Re: [PATCH bpf-next 5/7] bpf: preserve STACK_ZERO slots on partial reg spills

Andrii Nakryiko
2023-11-09T17:37:23Z

On Thu, Nov 9, 2023 at 7:20 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Mon, 2023-10-30 at 22:03 -0700, Andrii Nakryiko wrote:
> > Instead of always forcing STACK_ZERO slots to STACK_MISC, preserve it in
> > situations where this is possible. E.g., when spilling register as
> > 1/2/4-byte subslots on the stack, all the remaining bytes in the stack
> > slot do not automatically become unknown. If we knew they contained
> > zeroes, we can preserve those STACK_ZERO markers.
> >
> > Add a helper mark_stack_slot_misc(), similar to scrub_spilled_slot(),
> > but that doesn't overwrite either STACK_INVALID nor STACK_ZERO. Note
> > that we need to take into account possibility of being in unprivileged
> > mode, in which case STACK_INVALID is forced to STACK_MISC for correctness,
> > as treating STACK_INVALID as equivalent STACK_MISC is only enabled in
> > privileged mode.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
>
> Could you please add a test case?
>

sure

> Acked-by: Eduard Zingerman <eddyz87@gmail.com>
>
> [...]
>
> > @@ -1355,6 +1355,21 @@ static void scrub_spilled_slot(u8 *stype)
> > *stype = STACK_MISC;
> > }
> >
> > +/* 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.
> > + */
> > +static void mark_stack_slot_misc(struct bpf_verifier_env *env, u8 *stype)
>
> Nitpick: I find this name misleading, maybe something like "remove_spill_mark"?

remove_spill_mark is even more misleading, no? there is also DYNPTR
and ITER stack slots?

maybe mark_stack_slot_scalar (though that's a bit misleading as well,
as can be understood as marking slot as spilled SCALAR_VALUE
register)? not sure, I think "slot_misc" is close enough as an
approximation of what it's doing, modulo ZERO/INVALID

>
> [...]
>
>