bpf: fix verification of indirect var-off stack access
Subsystems (4)
v5 cover bpf: fix verification of indirect var-off stack access
V4 to V5:
- split the test into a separate patch
V3 to V4:
- include a test per Eduard's request
- target bpf-next per Alexei's request (patches didn't change)
V2 to V3:
- simplify checks for max_off (don't call check_stack_slot_within_bounds for it)
- append a commit to protect against overflow in the addition of the register and the offset
V1 to V2:
- fix max_off calculation for access size = 0
Andrei Matei (3):
bpf: fix verification of indirect var-off stack access
bpf: add verifier regression test for previous patch
bpf: guard stack limits against 32bit overflow
kernel/bpf/verifier.c | 20 +++++-------- .../selftests/bpf/progs/verifier_var_off.c | 29 +++++++++++++++++++ 2 files changed, 36 insertions(+), 13 deletions(-)
Signature · 1 line
2.40.1
PATCH v5 1/3 bpf: fix verification of indirect var-off stack access
This patch fixes a bug around the verification of possibly-zero-sized stack accesses. When the access was done through a var-offset stack pointer, check_stack_access_within_bounds was incorrectly computing the maximum-offset of a zero-sized read to be the same as the register's min offset. Instead, we have to take in account the register's maximum possible value. The patch also simplifies how the max offset is checked; the check is now simpler than for min offset.
The bug was allowing accesses to erroneously pass the check_stack_access_within_bounds() checks, only to later crash in check_stack_range_initialized() when all the possibly-affected stack slots are iterated (this time with a correct max offset). check_stack_range_initialized() is relying on check_stack_access_within_bounds() for its accesses to the stack-tracking vector to be within bounds; in the case of zero-sized accesses, we were essentially only verifying that the lowest possible slot was within bounds. We would crash when the max-offset of the stack pointer was >= 0 (which shouldn't pass verification, and hopefully is not something anyone's code attempts to do in practice).
Thanks Hao for reporting!
--- kernel/bpf/verifier.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)
kernel/bpf/verifier.c +4 -10
@@ -6620,10 +6620,7 @@ static int check_stack_access_within_bounds(if (tnum_is_const (reg->var_off)) { min_off = reg->var_off.value + off; -if (access_size >0 ) - max_off = min_off + access_size -1 ; -else - max_off = min_off; + max_off = min_off + access_size; }else {if (reg->smax_value >=BPF_MAX_VAR_OFF || reg->smin_value <= -BPF_MAX_VAR_OFF ) {
@@ -6632,15 +6629,12 @@ static int check_stack_access_within_bounds(return -EACCES ; } min_off = reg->smin_value + off; -if (access_size >0 ) - max_off = reg->smax_value + off + access_size -1 ; -else - max_off = min_off; + max_off = reg->smax_value + off + access_size; } err =check_stack_slot_within_bounds (min_off, state, type); -if (!err) - err =check_stack_slot_within_bounds (max_off, state, type); +if (!err && max_off >0 ) + err = -EINVAL ;/* out of stack access into non-negative offsets */ if (err) {if (tnum_is_const (reg->var_off)) {
Signature · 1 line
2.40.1
PATCH v5 2/3 bpf: add verifier regression test for previous patch
In reply to Andrei MateiAdd a regression test for var-off zero-sized reads.
--- .../selftests/bpf/progs/verifier_var_off.c | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+)
tools/testing/selftests/bpf/progs/verifier_var_off.c +29 -0
@@ -224,6 +224,35 @@ __naked void access_max_out_of_bound(void) : __clobber_all); } +/* Similar to the test above, but this time check the special case of a +* zero-sized stack access. We used to have a bug causing crashes for zero-sized +* out-of-bounds accesses. +*/ +SEC("socket") +__description("indirect variable-offset stack access, zero-sized, max out of bound") +__failure __msg("invalid variable-offset indirect access to stack R1") +__naked void zero_sized_access_max_out_of_bound(void) +{ + asmvolatile (" \ +r0 = 0; \ +/* Fill some stack */ \ +*(u64*)(r10 - 16) = r0; \ +*(u64*)(r10 - 8) = r0; \ +/* Get an unknown value */ \ +r1 = *(u32*)(r1 + 0); \ +r1 &= 63; \ +r1 += -16; \ +/* r1 is now anywhere in [-16,48) */ \ +r1 += r10; \ +r2 = 0; \ +r3 = 0; \ +call %[bpf_probe_read_kernel]; \ +exit; \ +" : + : __imm(bpf_probe_read_kernel) + : __clobber_all); +} + SEC("lwt_in") __description("indirect variable-offset stack access, min out of bound") __failure __msg("invalid variable-offset indirect access to stack R2")
Signature · 1 line
2.40.1
PATCH v5 3/3 bpf: guard stack limits against 32bit overflow
In reply to Andrei MateiThis patch promotes the arithmetic around checking stack bounds to be done in the 64-bit domain, instead of the current 32bit. The arithmetic implies adding together a 64-bit register with a int offset. The register was checked to be below 1<<29 when it was variable, but not when it was fixed. The offset either comes from an instruction (in which case it is 16 bit), from another register (in which case the caller checked it to be below 1<<29 [1]), or from the size of an argument to a kfunc (in which case it can be a u32 [2]). Between the register being inconsistently checked to be below 1<<29, and the offset being up to an u32, it appears that we were open to overflowing the ints which were currently used for arithmetic.
[1] https://github.com/torvalds/linux/blob/815fb87b753055df2d9e50f6cd80eb10235fe3e9/kernel/bpf/verifier.c#L7494-L7498
[2] https://github.com/torvalds/linux/blob/815fb87b753055df2d9e50f6cd80eb10235fe3e9/kernel/bpf/verifier.c#L11904
--- kernel/bpf/verifier.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/bpf/verifier.c +3 -3
@@ -6577,7 +6577,7 @@ static int check_ptr_to_map_access(struct bpf_verifier_env *env,* The minimum valid offset is -MAX_BPF_STACK for writes, and * -state->allocated_stack for reads. */ -static int check_stack_slot_within_bounds (int off, +static int check_stack_slot_within_bounds (s64 off,struct bpf_func_state *state,enum bpf_access_type t) {
@@ -6606,7 +6606,7 @@ static int check_stack_access_within_bounds(struct bpf_reg_state *regs =cur_regs (env);struct bpf_reg_state *reg = regs + regno;struct bpf_func_state *state =func (env, reg); -int min_off, max_off; +s64 min_off, max_off;int err;char *err_extra;
@@ -6619,7 +6619,7 @@ static int check_stack_access_within_bounds( err_extra =" write to" ;if (tnum_is_const (reg->var_off)) { - min_off = reg->var_off.value + off; + min_off = (s64 )reg->var_off.value + off; max_off = min_off + access_size; }else { if (reg->smax_value >= BPF_MAX_VAR_OFF ||
Signature · 1 line
2.40.1
This series was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <[email protected]>:
Hello:
This series was applied to bpf/bpf-next.git (master) by Andrii Nakryiko <[email protected]>:
Quoted 8 lines · On Wed, 6 Dec 2023 23:11:47 -0500 you wrote:
V4 to V5:
- split the test into a separate patchV3 to V4:
- include a test per Eduard's request
- target bpf-next per Alexei's request (patches didn't change)[...]
Here is the summary with links:
- [bpf-next,v5,1/3] bpf: fix verification of indirect var-off stack access
https://git.kernel.org/bpf/bpf-next/c/4f114bc280bb - [bpf-next,v5,2/3] bpf: add verifier regression test for previous patch https://git.kernel.org/bpf/bpf-next/c/ec32ca301faa - [bpf-next,v5,3/3] bpf: guard stack limits against 32bit overflow https://git.kernel.org/bpf/bpf-next/c/755f82668d81
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html