[bpf-next,v2,2/4] bpf: Fix narrow scalar spill onto 64-bit spilled scalar slots

Commit Message

Kumar Kartikeya Dwivedi Nov. 27, 2024, 9:20 p.m. UTC

From: Tao Lyu <[e-mail]>

When CAP_PERFMON and CAP_SYS_ADMIN (allow_ptr_leaks) are disabled, the
verifier aims to reject partial overwrite on an 8-byte stack slot that
contains a spilled pointer.

However, in such a scenario, it rejects all partial stack overwrites as
long as the targeted stack slot is a spilled register, because it does
not check if the stack slot is a spilled pointer.

Incomplete checks will result in the rejection of valid programs, which
spill narrower scalar values onto scalar slots, as shown below.

0: R1=ctx() R10=fp0
; asm volatile ( @ repro.bpf.c:679
0: (7a) *(u64 *)(r10 -8) = 1          ; R10=fp0 fp-8_w=1
1: (62) *(u32 *)(r10 -8) = 1
attempt to corrupt spilled pointer on stack
processed 2 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0.

Fix this by expanding the check to not consider spilled scalar registers
when rejecting the write into the stack.

Previous discussion on this patch is at link [0].

  [0]: https://lore.kernel.org/bpf/[e-mail]

Fixes: ab125ed3ec1c ("bpf: fix check for attempt to corrupt spilled pointer")
Signed-off-by: Tao Lyu <[e-mail]>
Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
---
 kernel/bpf/verifier.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Eduard Zingerman Nov. 28, 2024, 1:21 a.m. UTC | #1

On Wed, 2024-11-27 at 13:20 -0800, Kumar Kartikeya Dwivedi wrote:
> From: Tao Lyu <[e-mail]>
> 
> When CAP_PERFMON and CAP_SYS_ADMIN (allow_ptr_leaks) are disabled, the
> verifier aims to reject partial overwrite on an 8-byte stack slot that
> contains a spilled pointer.
> 
> However, in such a scenario, it rejects all partial stack overwrites as
> long as the targeted stack slot is a spilled register, because it does
> not check if the stack slot is a spilled pointer.
> 
> Incomplete checks will result in the rejection of valid programs, which
> spill narrower scalar values onto scalar slots, as shown below.
> 
> 0: R1=ctx() R10=fp0
> ; asm volatile ( @ repro.bpf.c:679
> 0: (7a) *(u64 *)(r10 -8) = 1          ; R10=fp0 fp-8_w=1
> 1: (62) *(u32 *)(r10 -8) = 1
> attempt to corrupt spilled pointer on stack
> processed 2 insns (limit 1000000) max_states_per_insn 0 total_states 0 peak_states 0 mark_read 0.
> 
> Fix this by expanding the check to not consider spilled scalar registers
> when rejecting the write into the stack.
> 
> Previous discussion on this patch is at link [0].
> 
>   [0]: https://lore.kernel.org/bpf/[e-mail]
> 
> Fixes: ab125ed3ec1c ("bpf: fix check for attempt to corrupt spilled pointer")
> Signed-off-by: Tao Lyu <[e-mail]>
> Signed-off-by: Kumar Kartikeya Dwivedi <[e-mail]>
> ---

Acked-by: Eduard Zingerman <[e-mail]>

[...]

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f9791a001e25..7fb3aa6561f5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4700,6 +4700,7 @@  static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
 	 */
 	if (!env->allow_ptr_leaks &&
 	    is_spilled_reg(&state->stack[spi]) &&
+	    !is_spilled_scalar_reg(&state->stack[spi]) &&
 	    size != BPF_REG_SIZE) {
 		verbose(env, "attempt to corrupt spilled pointer on stack\n");
 		return -EACCES;