On Mon, Dec 4, 2023 at 2:12 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> 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.
Hm... I wonder if the check was written like this deliberately to
prevent turning any spilled register into STACK_MISC?
> 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
What would happen if we have
4: *(u16 *)(r10 - 8) = 123; ?
and similarly
4: *(u16 *)(r10 - 6) = 123; ?
(16-bit overwrites of spilled 32-bit register)
It should be rejected, can you please quickly check if they will be
with the existing check?
So it makes me feel like the intent was to reject any partial writes
with spilled reg slots. We could probably improve that to just make
sure that we don't turn spilled pointers into STACK_MISC in unpriv,
but I'm not sure if it's worth doing that instead of keeping things
simple?
Alexei, do you remember what was the original intent?
>
> 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);
