Re: [PATCH bpf-next v2 1/4] bpf: Don't relax STACK_INVALID to STACK_MISC when not allow_ptr_leaks

[Eduard Zingerman <eddyz87@gmail.com>]

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@epfl.ch>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  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;
>  }

[...]