On Wed, 2024-11-27 at 13:20 -0800, Kumar Kartikeya Dwivedi wrote:
> Inside mark_stack_slot_misc, we should not upgrade STACK_INVALID to
> STACK_MISC when allow_ptr_leaks is false, since invalid contents
> shouldn't be read unless the program has the relevant capabilities.
> The relaxation only makes sense when env->allow_ptr_leaks is true.
>
> Currently, the condition is inverted (i.e. checking for true instead of
> false), simply invert it to restore correct behavior.
>
> Update error strings of selftests relying on current behavior's verifier
> output.
>
> Fixes: eaf18febd6eb ("bpf: preserve STACK_ZERO slots on partial reg spills")
> Reported-by: Tao Lyu <tao.lyu@xxxxxxx>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 2 +-
> .../selftests/bpf/progs/verifier_spill_fill.c | 18 +++++++++---------
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1c4ebb326785..f9791a001e25 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1209,7 +1209,7 @@ static void mark_stack_slot_misc(struct bpf_verifier_env *env, u8 *stype)
> {
> if (*stype == STACK_ZERO)
> return;
> - if (env->allow_ptr_leaks && *stype == STACK_INVALID)
> + if (!env->allow_ptr_leaks && *stype == STACK_INVALID)
This change makes sense, but it contradicts a few things:
- comment on top of this function;
- commit message for [0]
(there is my ack on that commit, but I have no memory of this place...).
Andrii, do you remember why STACK_INVALID had to be changed to STACK_MISC
for unprivileged case?
Kumar argues that the following program should be rejected when unprivileged:
0: (b7) r2 = 1 ; R2_w=1
1: (bf) r6 = r10 ; R6_w=fp0 R10=fp0
2: (07) r6 += -8 ; R6_w=fp-8
3: (73) *(u8 *)(r6 +0) = r2 ; R2_w=1 R6_w=fp-8 fp-8=???????1
4: (79) r2 = *(u64 *)(r6 +0)
invalid read from stack off -8+1 size 8
(which makes sense). But on master we have:
0: (b7) r2 = 1 ; R2_w=1
1: (bf) r6 = r10 ; R6_w=fp0 R10=fp0
2: (07) r6 += -8 ; R6_w=fp-8
3: (73) *(u8 *)(r6 +0) = r2 ; R2_w=1 R6_w=fp-8 fp-8=mmmmmmm1
4: (79) r2 = *(u64 *)(r6 +0) ; R2_w=scalar() R6_w=fp-8 fp-8=mmmmmmm1
5: (b7) r0 = 0 ; R0_w=0
6: (95) exit
(which makes much less sense).
Also, technically speaking, there is no longer a need in replacing
STACK_INVALID with STACK_MISC at all, only STACK_SPILL should be replaced.
(Because in privileged mode reads from STACK_INVALID are allowed).
[0] eaf18febd6eb ("bpf: preserve STACK_ZERO slots on partial reg spills")
> return;
> *stype = STACK_MISC;
> }
[...]
