On Fri, Apr 5, 2024 at 1:26 PM Tao Lyu <tao.lyu@xxxxxxx> wrote:
>
> When CAP_PERFMON and CAP_SYS_ADMIN (allow_ptr_leaks) are disabled,
> the verifier previously aimed to reject partial overwrite on an 8-byte stack slot
> that contains a spilled pointer.
>
> However, it rejects all partial stack overwrites
> as long as the targeted stack slot is a spilled register in that cap setting,
> because it does not check if the stack slot is a spilled pointer.
>
> Finally, 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.
>
> Note that only when CAP_BPF or CAP_SYS_ADMIN are enabled,
> are 64-bit scalars stored onto stacks marked as spilled scalars.
>
> Thus, to reproduce this issue,
> we can only assign CAP_BPF capability to the test.
>
> However, the existing bpf selftest doesn't support setting specific caps.
>
> So, I modified the test_loader.c and some related header files to
> enabled set specific capabilities on a test with the following attributes:
>
> __msg_caps(msg)
> __failure_caps(caps)
> __success_caps(caps)
> __retval_caps(val)
>
> Here, caps can be any combination of capabilities, like "CAP_BPF | CAP_PERFMON".
>
> Finally, the issue is fixed by checking the spilled register type of targeted slots.
>
> Fixes: ab125ed3ec1c ("bpf: fix check for attempt to corrupt spilled pointer")
> Signed-off-by: Tao Lyu <tao.lyu@xxxxxxx>
> ---
> kernel/bpf/verifier.c | 1 +
> tools/testing/selftests/bpf/progs/bpf_misc.h | 13 ++
> .../selftests/bpf/progs/verifier_spill_fill.c | 18 ++
> tools/testing/selftests/bpf/test_loader.c | 154 ++++++++++++++++--
> 4 files changed, 169 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 63749ad5ac6b..da575e295d53 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -4493,6 +4493,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;
ack for this part:
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
but I'm not sure if we need all the *_caps logic for selftests just to
test this, tbh. I'll let other decide, though.
> diff --git a/tools/testing/selftests/bpf/progs/bpf_misc.h b/tools/testing/selftests/bpf/progs/bpf_misc.h
> index fb2f5513e29e..b8dc0d73932f 100644
> --- a/tools/testing/selftests/bpf/progs/bpf_misc.h
> +++ b/tools/testing/selftests/bpf/progs/bpf_misc.h
> @@ -2,6 +2,10 @@
> #ifndef __BPF_MISC_H__
> #define __BPF_MISC_H__
>
[...]
