On Sat, Jan 07, 2023 at 01:03:24AM IST, Joanne Koong wrote:
> On Wed, Jan 4, 2023 at 2:44 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi
> > <memxor@xxxxxxxxx> wrote:
> > >
> > > Consider a program like below:
> > >
> > > void prog(void)
> > > {
> > > {
> > > struct bpf_dynptr ptr;
> > > bpf_dynptr_from_mem(...);
> > > }
> > > ...
> > > {
> > > struct bpf_dynptr ptr;
> > > bpf_dynptr_from_mem(...);
> > > }
> > > }
> > >
> > > Here, the C compiler based on lifetime rules in the C standard would be
> > > well within in its rights to share stack storage for dynptr 'ptr' as
> > > their lifetimes do not overlap in the two distinct scopes. Currently,
> > > such an example would be rejected by the verifier, but this is too
> > > strict. Instead, we should allow reinitializing over dynptr stack slots
> > > and forget information about the old dynptr object.
> > >
> >
> > As mentioned in the previous patch, shouldn't we allow this only for
> > dynptrs that don't require OBJ_RELEASE, which would be those with
> > ref_obj_id == 0?
> >
>
> +1
>
Ack, I'll make this change.
> >
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > > ---
> > > kernel/bpf/verifier.c | 16 +++++++++-------
> > > 1 file changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index b985d90505cc..e85e8c4be00d 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -786,6 +786,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> > > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> > > return -EINVAL;
> > >
> > > + destroy_stack_slots_dynptr(env, state, spi);
> > > + destroy_stack_slots_dynptr(env, state, spi - 1);
>
> We don't need the 2nd call since destroy_slots_dynptr() destroys both slots
>
We do, I'll add a comment explaining this.
There are two cases.
[d1][d1][d2][d2]
spi 3 2 1 0
If initializing on spi = 3, it will destroy d1, spi - 1 will see no STACK_DYNPTR
and simply ignore the call.
But in case we initialize on spi = 2, it will destroy d1 but not d2, only the
next call will destroy the dynptr at d2.
So for the case where we initialize over slots of two adjacent dynptrs, we'll
miss that case without making the 2nd call.
The call simply means 'destroy any dynptr which spi belongs to'. So it needs to
be made for both, as both spi and spi-1 may belong to different dynptrs.
