On Mon, Dec 2, 2024 at 12:38 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> 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.
>
> However, such conversion in privileged mode becomes unnecessary, as
> invalid slots can be read without being upgraded to STACK_MISC.
>
> Currently, the condition is inverted (i.e. checking for true instead of
> false), simply remove it to restore correct behavior.
>
> 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 | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1c4ebb326785..c6a5c431495c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -1202,14 +1202,17 @@ static bool is_spilled_scalar_reg64(const struct bpf_stack_state *stack)
> /* Mark stack slot as STACK_MISC, unless it is already STACK_INVALID, in which
> * case they are equivalent, or it's STACK_ZERO, in which case we preserve
> * more precise STACK_ZERO.
> - * Note, in uprivileged mode leaving STACK_INVALID is wrong, so we take
> - * env->allow_ptr_leaks into account and force STACK_MISC, if necessary.
> + * Regardless of allow_ptr_leaks setting (i.e., privileged or unprivileged
> + * mode), we won't promote STACK_INVALID to STACK_MISC. In privileged case it is
> + * unnecessary as both are considered equivalent when loading data and pruning,
> + * in case of unprivileged mode it will be incorrect to allow reads of invalid
> + * slots.
> */
> 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 (*stype == STACK_INVALID)
It's a bit worrying that my original comment explicitly states that in
unpriv mode we *have* to set STACK_MISC, but I can't recall why.
Looking at this now, it looks good, so:
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> return;
> *stype = STACK_MISC;
> }
> --
> 2.43.5
>
