| Message ID | 20210524225619.26662-2-cascardo@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | CVE-2021-31440 | expand |
On 24/05/2021 18:56, Thadeu Lima de Souza Cascardo wrote: > From: Daniel Borkmann <daniel@iogearbox.net> > > Similarly as b02709587ea3 ("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 b02709587ea3 holds for unsigned subregister tracking. > Also, align __reg64_bound_u32() similarly to __reg64_bound_s32() as done in > b02709587ea3 to make them uniform again. > > Fixes: 3f50f132d840 ("bpf: Verifier, do explicit ALU32 bounds tracking") > Reported-by: Manfred Paul (@_manfp) > Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> > Reviewed-by: John Fastabend <john.fastabend@gmail.com> > Acked-by: Alexei Starovoitov <ast@kernel.org> > (cherry picked from commit 10bf4e83167cc68595b85fd73bb91e8f2c086e36) > CVE-2021-31440 > Signed-off-by: Thadeu Lima de Souza Cascardo <cascardo@canonical.com> > --- > kernel/bpf/verifier.c | 8 +++----- > tools/testing/selftests/bpf/verifier/array_access.c | 2 +- > 2 files changed, 4 insertions(+), 6 deletions(-) > Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Best regards, Krzysztof
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 39e475dd0f7b..8a5f88a19024 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -1304,9 +1304,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) @@ -1317,10 +1315,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), diff --git a/tools/testing/selftests/bpf/verifier/array_access.c b/tools/testing/selftests/bpf/verifier/array_access.c index 1b138cd2b187..1b1c798e9248 100644 --- a/tools/testing/selftests/bpf/verifier/array_access.c +++ b/tools/testing/selftests/bpf/verifier/array_access.c @@ -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,