| From cb454e724c48ed927e0d92405a896ccd82a385e9 Mon Sep 17 00:00:00 2001 |
| From: Sasha Levin <sashal@kernel.org> |
| Date: Fri, 23 Apr 2021 13:59:55 +0000 |
| Subject: bpf: Fix propagation of 32 bit unsigned bounds from 64 bit bounds |
| |
| From: Daniel Borkmann <daniel@iogearbox.net> |
| |
| [ Upstream commit 10bf4e83167cc68595b85fd73bb91e8f2c086e36 ] |
| |
| 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> |
| Signed-off-by: Sasha Levin <sashal@kernel.org> |
| --- |
| kernel/bpf/verifier.c | 8 +++----- |
| tools/testing/selftests/bpf/verifier/array_access.c | 2 +- |
| 2 files changed, 4 insertions(+), 6 deletions(-) |
| |
| diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c |
| index 4e4a844a68c3..3dd297203ab5 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, |
| -- |
| 2.30.2 |
| |