Skip to content

Commit 3844d15

Browse files
borkmannanakryiko
borkmann
authored and
anakryiko
committed
bpf: Fix insufficient bounds propagation from adjust_scalar_min_max_vals
Kuee reported a corner case where the tnum becomes constant after the call to __reg_bound_offset(), but the register's bounds are not, that is, its min bounds are still not equal to the register's max bounds. This in turn allows to leak pointers through turning a pointer register as is into an unknown scalar via adjust_ptr_min_max_vals(). Before: func#0 @0 0: R1=ctx(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) R10=fp(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) 0: (b7) r0 = 1 ; R0_w=scalar(imm=1,umin=1,umax=1,var_off=(0x1; 0x0)) 1: (b7) r3 = 0 ; R3_w=scalar(imm=0,umax=0,var_off=(0x0; 0x0)) 2: (87) r3 = -r3 ; R3_w=scalar() 3: (87) r3 = -r3 ; R3_w=scalar() 4: (47) r3 |= 32767 ; R3_w=scalar(smin=-9223372036854743041,umin=32767,var_off=(0x7fff; 0xffffffffffff8000),s32_min=-2147450881) 5: (75) if r3 s>= 0x0 goto pc+1 ; R3_w=scalar(umin=9223372036854808575,var_off=(0x8000000000007fff; 0x7fffffffffff8000),s32_min=-2147450881,u32_min=32767) 6: (95) exit from 5 to 7: R0=scalar(imm=1,umin=1,umax=1,var_off=(0x1; 0x0)) R1=ctx(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) R3=scalar(umin=32767,umax=9223372036854775807,var_off=(0x7fff; 0x7fffffffffff8000),s32_min=-2147450881) R10=fp(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) 7: (d5) if r3 s<= 0x8000 goto pc+1 ; R3=scalar(umin=32769,umax=9223372036854775807,var_off=(0x7fff; 0x7fffffffffff8000),s32_min=-2147450881,u32_min=32767) 8: (95) exit from 7 to 9: R0=scalar(imm=1,umin=1,umax=1,var_off=(0x1; 0x0)) R1=ctx(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) R3=scalar(umin=32767,umax=32768,var_off=(0x7fff; 0x8000)) R10=fp(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) 9: (07) r3 += -32767 ; R3_w=scalar(imm=0,umax=1,var_off=(0x0; 0x0)) <--- [*] 10: (95) exit What can be seen here is that R3=scalar(umin=32767,umax=32768,var_off=(0x7fff; 0x8000)) after the operation R3 += -32767 results in a 'malformed' constant, that is, R3_w=scalar(imm=0,umax=1,var_off=(0x0; 0x0)). Intersecting with var_off has not been done at that point via __update_reg_bounds(), which would have improved the umax to be equal to umin. Refactor the tnum <> min/max bounds information flow into a reg_bounds_sync() helper and use it consistently everywhere. After the fix, bounds have been corrected to R3_w=scalar(imm=0,umax=0,var_off=(0x0; 0x0)) and thus the register is regarded as a 'proper' constant scalar of 0. After: func#0 @0 0: R1=ctx(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) R10=fp(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) 0: (b7) r0 = 1 ; R0_w=scalar(imm=1,umin=1,umax=1,var_off=(0x1; 0x0)) 1: (b7) r3 = 0 ; R3_w=scalar(imm=0,umax=0,var_off=(0x0; 0x0)) 2: (87) r3 = -r3 ; R3_w=scalar() 3: (87) r3 = -r3 ; R3_w=scalar() 4: (47) r3 |= 32767 ; R3_w=scalar(smin=-9223372036854743041,umin=32767,var_off=(0x7fff; 0xffffffffffff8000),s32_min=-2147450881) 5: (75) if r3 s>= 0x0 goto pc+1 ; R3_w=scalar(umin=9223372036854808575,var_off=(0x8000000000007fff; 0x7fffffffffff8000),s32_min=-2147450881,u32_min=32767) 6: (95) exit from 5 to 7: R0=scalar(imm=1,umin=1,umax=1,var_off=(0x1; 0x0)) R1=ctx(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) R3=scalar(umin=32767,umax=9223372036854775807,var_off=(0x7fff; 0x7fffffffffff8000),s32_min=-2147450881) R10=fp(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) 7: (d5) if r3 s<= 0x8000 goto pc+1 ; R3=scalar(umin=32769,umax=9223372036854775807,var_off=(0x7fff; 0x7fffffffffff8000),s32_min=-2147450881,u32_min=32767) 8: (95) exit from 7 to 9: R0=scalar(imm=1,umin=1,umax=1,var_off=(0x1; 0x0)) R1=ctx(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) R3=scalar(umin=32767,umax=32768,var_off=(0x7fff; 0x8000)) R10=fp(off=0,imm=0,umax=0,var_off=(0x0; 0x0)) 9: (07) r3 += -32767 ; R3_w=scalar(imm=0,umax=0,var_off=(0x0; 0x0)) <--- [*] 10: (95) exit Fixes: b03c9f9 ("bpf/verifier: track signed and unsigned min/max values") Reported-by: Kuee K1r0a <liulin063@gmail.com> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Signed-off-by: Andrii Nakryiko <andrii@kernel.org> Acked-by: John Fastabend <john.fastabend@gmail.com> Link: https://lore.kernel.org/bpf/20220701124727.11153-2-daniel@iogearbox.net
1 parent a12ca62 commit 3844d15

1 file changed

Lines changed: 23 additions & 49 deletions

File tree

‎kernel/bpf/verifier.c‎

Lines changed: 23 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1562,6 +1562,21 @@ static void __reg_bound_offset(struct bpf_reg_state *reg)
15621562
reg->var_off = tnum_or(tnum_clear_subreg(var64_off), var32_off);
15631563
}
15641564

1565+
static void reg_bounds_sync(struct bpf_reg_state *reg)
1566+
{
1567+
/* We might have learned new bounds from the var_off. */
1568+
__update_reg_bounds(reg);
1569+
/* We might have learned something about the sign bit. */
1570+
__reg_deduce_bounds(reg);
1571+
/* We might have learned some bits from the bounds. */
1572+
__reg_bound_offset(reg);
1573+
/* Intersecting with the old var_off might have improved our bounds
1574+
* slightly, e.g. if umax was 0x7f...f and var_off was (0; 0xf...fc),
1575+
* then new var_off is (0; 0x7f...fc) which improves our umax.
1576+
*/
1577+
__update_reg_bounds(reg);
1578+
}
1579+
15651580
static bool __reg32_bound_s64(s32 a)
15661581
{
15671582
return a >= 0 && a <= S32_MAX;
@@ -1603,16 +1618,8 @@ static void __reg_combine_32_into_64(struct bpf_reg_state *reg)
16031618
* so they do not impact tnum bounds calculation.
16041619
*/
16051620
__mark_reg64_unbounded(reg);
1606-
__update_reg_bounds(reg);
16071621
}
1608-
1609-
/* Intersecting with the old var_off might have improved our bounds
1610-
* slightly. e.g. if umax was 0x7f...f and var_off was (0; 0xf...fc),
1611-
* then new var_off is (0; 0x7f...fc) which improves our umax.
1612-
*/
1613-
__reg_deduce_bounds(reg);
1614-
__reg_bound_offset(reg);
1615-
__update_reg_bounds(reg);
1622+
reg_bounds_sync(reg);
16161623
}
16171624

16181625
static bool __reg64_bound_s32(s64 a)
@@ -1628,7 +1635,6 @@ static bool __reg64_bound_u32(u64 a)
16281635
static void __reg_combine_64_into_32(struct bpf_reg_state *reg)
16291636
{
16301637
__mark_reg32_unbounded(reg);
1631-
16321638
if (__reg64_bound_s32(reg->smin_value) && __reg64_bound_s32(reg->smax_value)) {
16331639
reg->s32_min_value = (s32)reg->smin_value;
16341640
reg->s32_max_value = (s32)reg->smax_value;
@@ -1637,14 +1643,7 @@ static void __reg_combine_64_into_32(struct bpf_reg_state *reg)
16371643
reg->u32_min_value = (u32)reg->umin_value;
16381644
reg->u32_max_value = (u32)reg->umax_value;
16391645
}
1640-
1641-
/* Intersecting with the old var_off might have improved our bounds
1642-
* slightly. e.g. if umax was 0x7f...f and var_off was (0; 0xf...fc),
1643-
* then new var_off is (0; 0x7f...fc) which improves our umax.
1644-
*/
1645-
__reg_deduce_bounds(reg);
1646-
__reg_bound_offset(reg);
1647-
__update_reg_bounds(reg);
1646+
reg_bounds_sync(reg);
16481647
}
16491648

16501649
/* Mark a register as having a completely unknown (scalar) value. */
@@ -6943,9 +6942,7 @@ static void do_refine_retval_range(struct bpf_reg_state *regs, int ret_type,
69436942
ret_reg->s32_max_value = meta->msize_max_value;
69446943
ret_reg->smin_value = -MAX_ERRNO;
69456944
ret_reg->s32_min_value = -MAX_ERRNO;
6946-
__reg_deduce_bounds(ret_reg);
6947-
__reg_bound_offset(ret_reg);
6948-
__update_reg_bounds(ret_reg);
6945+
reg_bounds_sync(ret_reg);
69496946
}
69506947

69516948
static int
@@ -8202,11 +8199,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
82028199

82038200
if (!check_reg_sane_offset(env, dst_reg, ptr_reg->type))
82048201
return -EINVAL;
8205-
8206-
__update_reg_bounds(dst_reg);
8207-
__reg_deduce_bounds(dst_reg);
8208-
__reg_bound_offset(dst_reg);
8209-
8202+
reg_bounds_sync(dst_reg);
82108203
if (sanitize_check_bounds(env, insn, dst_reg) < 0)
82118204
return -EACCES;
82128205
if (sanitize_needed(opcode)) {
@@ -8944,10 +8937,7 @@ static int adjust_scalar_min_max_vals(struct bpf_verifier_env *env,
89448937
/* ALU32 ops are zero extended into 64bit register */
89458938
if (alu32)
89468939
zext_32_to_64(dst_reg);
8947-
8948-
__update_reg_bounds(dst_reg);
8949-
__reg_deduce_bounds(dst_reg);
8950-
__reg_bound_offset(dst_reg);
8940+
reg_bounds_sync(dst_reg);
89518941
return 0;
89528942
}
89538943

@@ -9136,10 +9126,7 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
91369126
insn->dst_reg);
91379127
}
91389128
zext_32_to_64(dst_reg);
9139-
9140-
__update_reg_bounds(dst_reg);
9141-
__reg_deduce_bounds(dst_reg);
9142-
__reg_bound_offset(dst_reg);
9129+
reg_bounds_sync(dst_reg);
91439130
}
91449131
} else {
91459132
/* case: R = imm
@@ -9742,21 +9729,8 @@ static void __reg_combine_min_max(struct bpf_reg_state *src_reg,
97429729
dst_reg->smax_value);
97439730
src_reg->var_off = dst_reg->var_off = tnum_intersect(src_reg->var_off,
97449731
dst_reg->var_off);
9745-
/* We might have learned new bounds from the var_off. */
9746-
__update_reg_bounds(src_reg);
9747-
__update_reg_bounds(dst_reg);
9748-
/* We might have learned something about the sign bit. */
9749-
__reg_deduce_bounds(src_reg);
9750-
__reg_deduce_bounds(dst_reg);
9751-
/* We might have learned some bits from the bounds. */
9752-
__reg_bound_offset(src_reg);
9753-
__reg_bound_offset(dst_reg);
9754-
/* Intersecting with the old var_off might have improved our bounds
9755-
* slightly. e.g. if umax was 0x7f...f and var_off was (0; 0xf...fc),
9756-
* then new var_off is (0; 0x7f...fc) which improves our umax.
9757-
*/
9758-
__update_reg_bounds(src_reg);
9759-
__update_reg_bounds(dst_reg);
9732+
reg_bounds_sync(src_reg);
9733+
reg_bounds_sync(dst_reg);
97609734
}
97619735

97629736
static void reg_combine_min_max(struct bpf_reg_state *true_src,

0 commit comments

Comments
 (0)