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

Eduard Zingerman
2023-11-09T15:20:56Z

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?

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"? …