Commit 10bf4e83 authored by Daniel Borkmann's avatar Daniel Borkmann
Browse files

bpf: Fix propagation of 32 bit unsigned bounds from 64 bit bounds 



Similarly as b0270958 ("bpf: Fix propagation of 32-bit signed bounds
from 64-bit bounds."), we also need to fix the propagation of 32 bit
unsigned bounds from 64 bit counterparts. That is, really only set the
u32_{min,max}_value when /both/ {umin,umax}_value safely fit in 32 bit
space. For example, the register with a umin_value == 1 does /not/ imply
that u32_min_value is also equal to 1, since umax_value could be much
larger than 32 bit subregister can hold, and thus u32_min_value is in
the interval [0,1] instead.

Before fix, invalid tracking result of R2_w=inv1:

  [...]
  5: R0_w=inv1337 R1=ctx(id=0,off=0,imm=0) R2_w=inv(id=0) R10=fp0
  5: (35) if r2 >= 0x1 goto pc+1
  [...] // goto path
  7: R0=inv1337 R1=ctx(id=0,off=0,imm=0) R2=inv(id=0,umin_value=1) R10=fp0
  7: (b6) if w2 <= 0x1 goto pc+1
  [...] // goto path
  9: R0=inv1337 R1=ctx(id=0,off=0,imm=0) R2=inv(id=0,smin_value=-9223372036854775807,smax_value=9223372032559808513,umin_value=1,umax_value=18446744069414584321,var_off=(0x1; 0xffffffff00000000),s32_min_value=1,s32_max_value=1,u32_max_value=1) R10=fp0
  9: (bc) w2 = w2
  10: R0=inv1337 R1=ctx(id=0,off=0,imm=0) R2_w=inv1 R10=fp0
  [...]

After fix, correct tracking result of R2_w=inv(id=0,umax_value=1,var_off=(0x0; 0x1)):

  [...]
  5: R0_w=inv1337 R1=ctx(id=0,off=0,imm=0) R2_w=inv(id=0) R10=fp0
  5: (35) if r2 >= 0x1 goto pc+1
  [...] // goto path
  7: R0=inv1337 R1=ctx(id=0,off=0,imm=0) R2=inv(id=0,umin_value=1) R10=fp0
  7: (b6) if w2 <= 0x1 goto pc+1
  [...] // goto path
  9: R0=inv1337 R1=ctx(id=0,off=0,imm=0) R2=inv(id=0,smax_value=9223372032559808513,umax_value=18446744069414584321,var_off=(0x0; 0xffffffff00000001),s32_min_value=0,s32_max_value=1,u32_max_value=1) R10=fp0
  9: (bc) w2 = w2
  10: R0=inv1337 R1=ctx(id=0,off=0,imm=0) R2_w=inv(id=0,umax_value=1,var_off=(0x0; 0x1)) R10=fp0
  [...]

Thus, same issue as in b0270958 holds for unsigned subregister tracking.
Also, align __reg64_bound_u32() similarly to __reg64_bound_s32() as done in
b0270958 to make them uniform again.

Fixes: 3f50f132 ("bpf: Verifier, do explicit ALU32 bounds tracking")
Reported-by: default avatarManfred Paul <(@_manfp)>
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Reviewed-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
Acked-by: default avatarAlexei Starovoitov <ast@kernel.org>
parent 38d26d89

kernel/bpf/verifier.c

+3 −5
Original line number Diff line number Diff line
@@ -1398,9 +1398,7 @@ static bool __reg64_bound_s32(s64 a) 


static bool __reg64_bound_u32(u64 a)

{

	if (a > U32_MIN && a < U32_MAX)

		return true;

	return false;

	return a > U32_MIN && a < U32_MAX;

}



static void __reg_combine_64_into_32(struct bpf_reg_state *reg)
@@ -1411,10 +1409,10 @@ static void __reg_combine_64_into_32(struct bpf_reg_state *reg) 
		reg->s32_min_value = (s32)reg->smin_value;

		reg->s32_max_value = (s32)reg->smax_value;

	}

	if (__reg64_bound_u32(reg->umin_value))

	if (__reg64_bound_u32(reg->umin_value) && __reg64_bound_u32(reg->umax_value)) {

		reg->u32_min_value = (u32)reg->umin_value;

	if (__reg64_bound_u32(reg->umax_value))

		reg->u32_max_value = (u32)reg->umax_value;

	}



	/* Intersecting with the old var_off might have improved our bounds

	 * slightly.  e.g. if umax was 0x7f...f and var_off was (0; 0xf...fc),

tools/testing/selftests/bpf/verifier/array_access.c

+1 −1
Original line number Diff line number Diff line
@@ -186,7 +186,7 @@ 
	},

	.fixup_map_hash_48b = { 3 },

	.errstr_unpriv = "R0 leaks addr",

	.errstr = "invalid access to map value, value_size=48 off=44 size=8",

	.errstr = "R0 unbounded memory access",

	.result_unpriv = REJECT,

	.result = REJECT,

	.flags = F_NEEDS_EFFICIENT_UNALIGNED_ACCESS,