Re: [PATCH v3 bpf-next 03/10] bpf: fix check for attempt to corrupt spilled pointer

[Eduard Zingerman <eddyz87@xxxxxxxxx>]

On Mon, 2023-12-04 at 11:25 -0800, Andrii Nakryiko wrote:
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 4f8a3c77eb80..73315e2f20d9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4431,7 +4431,7 @@ static int check_stack_write_fixed_off(struct bpf_verifier_env *env,
>  	 * so it's aligned access and [off, off + size) are within stack limits
>  	 */
>  	if (!env->allow_ptr_leaks &&
> -	    state->stack[spi].slot_type[0] == STACK_SPILL &&
> +	    is_spilled_reg(&state->stack[spi]) &&
>  	    size != BPF_REG_SIZE) {
>  		verbose(env, "attempt to corrupt spilled pointer on stack\n");
>  		return -EACCES;

I think there is a small detail here.
slot_type[0] == STACK_SPILL actually checks if a spill is 64-bit.
Thus, with this patch applied the test below does not pass.
Log fragment:

    1: (57) r0 &= 65535                   ; R0_w=scalar(...,var_off=(0x0; 0xffff))
    2: (63) *(u32 *)(r10 -8) = r0
    3: R0_w=scalar(...,var_off=(0x0; 0xffff)) R10=fp0 fp-8=mmmmscalar(...,var_off=(0x0; 0xffff))
    3: (b7) r0 = 42                       ; R0_w=42
    4: (63) *(u32 *)(r10 -4) = r0
    attempt to corrupt spilled pointer on stack

Admittedly, this happens only when the only capability is CAP_BPF and
we don't test this configuration.

---

iff --git a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
index 359df865a8f3..61ada86e84df 100644
--- a/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
+++ b/tools/testing/selftests/bpf/progs/verifier_basic_stack.c
@@ -97,4 +97,20 @@ __naked void misaligned_read_from_stack(void)
 "      ::: __clobber_all);
 }
 
+SEC("socket")
+__success_unpriv
+__naked void spill_lo32_write_hi32(void)
+{
+       asm volatile ("                                 \
+       call %[bpf_get_prandom_u32];                    \
+       r0 &= 0xffff;                                   \
+       *(u32*)(r10 - 8) = r0;                          \
+       r0 = 42;                                        \
+       *(u32*)(r10 - 4) = r0;                          \
+       exit;                                           \
+"      :
+       : __imm(bpf_get_prandom_u32)
+       : __clobber_all);
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_loader.c b/tools/testing/selftests/bpf/test_loader.c
index a350ecdfba4a..a5ad6b01175e 100644
--- a/tools/testing/selftests/bpf/test_loader.c
+++ b/tools/testing/selftests/bpf/test_loader.c
@@ -430,7 +430,7 @@ struct cap_state {
 static int drop_capabilities(struct cap_state *caps)
 {
        const __u64 caps_to_drop = (1ULL << CAP_SYS_ADMIN | 1ULL << CAP_NET_ADMIN |
-                                   1ULL << CAP_PERFMON   | 1ULL << CAP_BPF);
+                                   1ULL << CAP_PERFMON /*| 1ULL << CAP_BPF */);
        int err;
 
        err = cap_disable_effective(caps_to_drop, &caps->old_caps);