On Thu, Nov 9, 2023 at 7:20 AM Eduard Zingerman <eddyz87@xxxxxxxxx> 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@xxxxxxxxxx> > > Could you please add a test case? > sure > Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx> > > [...] > > > @@ -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 > > [...] > >
