Skip to content

Commit bc678fa

Browse files
pchaignogregkh
authored andcommitted
selftests/bpf: Fix reg_bounds to match new tnum-based refinement
[ Upstream commit 2fefa9c ] Commit efc11a6 ("bpf: Improve bounds when tnum has a single possible value") improved the bounds refinement to detect when the tnum and u64 range overlap in a single value (and the bounds can thus be set to that value). Eduard then noticed that it broke the slow-mode reg_bounds selftests because they don't have an equivalent logic and are therefore unable to refine the bounds as much as the verifier. The following test case illustrates this. ACTUAL TRUE1: scalar(u64=0xffffffff00000000,u32=0,s64=0xffffffff00000000,s32=0) EXPECTED TRUE1: scalar(u64=[0xfffffffe00000001; 0xffffffff00000000],u32=0,s64=[0xfffffffe00000001; 0xffffffff00000000],s32=0) [...] #323/1007 reg_bounds_gen_consts_s64_s32/(s64)[0xfffffffe00000001; 0xffffffff00000000] (s32)<op> S64_MIN:FAIL with the verifier logs: [...] 19: w0 = w6 ; R0=scalar(smin=0,smax=umax=0xffffffff, var_off=(0x0; 0xffffffff)) R6=scalar(smin=0xfffffffe00000001,smax=0xffffffff00000000, umin=0xfffffffe00000001,umax=0xffffffff00000000, var_off=(0xfffffffe00000000; 0x1ffffffff)) 20: w0 = w7 ; R0=0 R7=0x8000000000000000 21: if w6 == w7 goto pc+3 [...] from 21 to 25: [...] 25: w0 = w6 ; R0=0 R6=0xffffffff00000000 ; ^ ; unexpected refined value 26: w0 = w7 ; R0=0 R7=0x8000000000000000 27: exit When w6 == w7 is true, the verifier can deduce that the R6's tnum is equal to (0xfffffffe00000000; 0x100000000) and then use that information to refine the bounds: the tnum only overlap with the u64 range in 0xffffffff00000000. The reg_bounds selftest doesn't know about tnums and therefore fails to perform the same refinement. This issue happens when the tnum carries information that cannot be represented in the ranges, as otherwise the selftest could reach the same refined value using just the ranges. The tnum thus needs to represent non-contiguous values (ex., R6's tnum above, after the condition). The only way this can happen in the reg_bounds selftest is at the boundary between the 32 and 64bit ranges. We therefore only need to handle that case. This patch fixes the selftest refinement logic by checking if the u32 and u64 ranges overlap in a single value. If so, the ranges can be set to that value. We need to handle two cases: either they overlap in umin64... u64 values matching u32 range: xxx xxx xxx xxx |--------------------------------------| u64 range: 0 xxxxx UMAX64 or in umax64: u64 values matching u32 range: xxx xxx xxx xxx |--------------------------------------| u64 range: 0 xxxxx UMAX64 To detect the first case, we decrease umax64 to the maximum value that matches the u32 range. If that happens to be umin64, then umin64 is the only overlap. We proceed similarly for the second case, increasing umin64 to the minimum value that matches the u32 range. Note this is similar to how the verifier handles the general case using tnum, but we don't need to care about a single-value overlap in the middle of the range. That case is not possible when comparing two ranges. This patch also adds two test cases reproducing this bug as part of the normal test runs (without SLOW_TESTS=1). Fixes: efc11a6 ("bpf: Improve bounds when tnum has a single possible value") Reported-by: Eduard Zingerman <eddyz87@gmail.com> Closes: https://lore.kernel.org/bpf/4e6dd64a162b3cab3635706ae6abfdd0be4db5db.camel@gmail.com/ Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com> Link: https://lore.kernel.org/r/ada9UuSQi2SE2IfB@mail.gmail.com Signed-off-by: Alexei Starovoitov <ast@kernel.org> Signed-off-by: Sasha Levin <sashal@kernel.org>
1 parent cddf2bd commit bc678fa

1 file changed

Lines changed: 35 additions & 0 deletions

File tree

tools/testing/selftests/bpf/prog_tests/reg_bounds.c

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -500,6 +500,39 @@ static struct range range_refine(enum num_t x_t, struct range x, enum num_t y_t,
500500
(s64)x.a >= S32_MIN && (s64)x.b <= S32_MAX)
501501
return range_intersection(x_t, x, y_cast);
502502

503+
if (y_t == U32 && x_t == U64) {
504+
u64 xmin_swap, xmax_swap, xmin_lower32, xmax_lower32;
505+
506+
xmin_lower32 = x.a & 0xffffffff;
507+
xmax_lower32 = x.b & 0xffffffff;
508+
if (xmin_lower32 < y.a || xmin_lower32 > y.b) {
509+
/* The 32 lower bits of the umin64 are outside the u32
510+
* range. Let's update umin64 to match the u32 range.
511+
* We want to *increase* the umin64 to the *minimum*
512+
* value that matches the u32 range.
513+
*/
514+
xmin_swap = swap_low32(x.a, y.a);
515+
/* We should always only increase the minimum, so if
516+
* the new value is lower than before, we need to
517+
* increase the 32 upper bits by 1.
518+
*/
519+
if (xmin_swap < x.a)
520+
xmin_swap += 0x100000000;
521+
if (xmin_swap == x.b)
522+
return range(x_t, x.b, x.b);
523+
} else if (xmax_lower32 < y.a || xmax_lower32 > y.b) {
524+
/* Same for the umax64, but we want to *decrease*
525+
* umax64 to the *maximum* value that matches the u32
526+
* range.
527+
*/
528+
xmax_swap = swap_low32(x.b, y.b);
529+
if (xmax_swap > x.b)
530+
xmax_swap -= 0x100000000;
531+
if (xmax_swap == x.a)
532+
return range(x_t, x.a, x.a);
533+
}
534+
}
535+
503536
/* the case when new range knowledge, *y*, is a 32-bit subregister
504537
* range, while previous range knowledge, *x*, is a full register
505538
* 64-bit range, needs special treatment to take into account upper 32
@@ -2143,6 +2176,8 @@ static struct subtest_case crafted_cases[] = {
21432176
{U64, S64, {0x7fffffff00000001ULL, 0xffffffff00000000ULL}, {0, 0}},
21442177
{U64, S64, {0, 0xffffffffULL}, {1, 1}},
21452178
{U64, S64, {0, 0xffffffffULL}, {0x7fffffff, 0x7fffffff}},
2179+
{U64, S32, {0xfffffffe00000001, 0xffffffff00000000}, {S64_MIN, S64_MIN}},
2180+
{U64, U32, {0xfffffffe00000000, U64_MAX - 1}, {U64_MAX, U64_MAX}},
21462181

21472182
{U64, U32, {0, 0x100000000}, {0, 0}},
21482183
{U64, U32, {0xfffffffe, 0x300000000}, {0x80000000, 0x80000000}},

0 commit comments

Comments
 (0)