On Thu, 2024-11-28 at 05:39 +0100, Kumar Kartikeya Dwivedi wrote:
[...]
> > > +static bool is_irq_flag_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > > +{
> > > + struct bpf_func_state *state = func(env, reg);
> > > + struct bpf_stack_state *slot;
> > > + int spi, i;
> > > +
> > > + /* For -ERANGE (i.e. spi not falling into allocated stack slots), we
> > > + * will do check_mem_access to check and update stack bounds later, so
> > > + * return true for that case.
> > > + */
> > > + spi = irq_flag_get_spi(env, reg);
> > > + if (spi == -ERANGE)
> > > + return true;
> >
> > Nit: is it possible to swap is_irq_flag_reg_valid_uninit() and
> > check_mem_access(), so that ERANGE special case would be not needed?
> >
>
> I don't think so. For dynptr, iter, irq, ERANGE indicates stack needs
> to be grown, so check_mem_access will naturally do that when writing.
> When not ERANGE, we need to catch cases where we have a bad slot_type.
> If we overwrote it with check_mem_access, then it would scrub the slot
> type as well.
>
> When I fixed this stuff for dynptr, we had to additionally
> destroy_if_dynptr_stack_slot because it wasn't required to 'release' a
> dynptr when overwriting it.
> Andrii made sure this was necessary for iters so now slot_type ==
> STACK_ITER is just rejected instead of overwrite without a destroy
> operation.
> Similar idea is followed for irq flag.
>
> Just paging in context for all this, but I may be missing if you have
> something in mind.
I see, makes sense. And is_dynptr_reg_valid_uninit() has the same check.
Thank you for explaining.
> > > + if (spi < 0)
> > > + return false;
> > > +
> > > + slot = &state->stack[spi];
> > > +
> > > + for (i = 0; i < BPF_REG_SIZE; i++)
> > > + if (slot->slot_type[i] == STACK_IRQ_FLAG)
> > > + return false;
> > > + return true;
> > > +}
> >
> > [...]
> >
