Message-ID: <20230310211541.schh7iyrqgbgfaay@macbook-pro-6.dhcp.thefacebook.com>
Date:   Fri, 10 Mar 2023 13:15:41 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Joanne Koong <joannelkoong@...il.com>,
        bpf <bpf@...r.kernel.org>,
        Martin KaFai Lau <martin.lau@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        Toke Høiland-Jørgensen <toke@...nel.org>
Subject: Re: [PATCH v13 bpf-next 09/10] bpf: Add bpf_dynptr_slice and
 bpf_dynptr_slice_rdwr

On Tue, Mar 07, 2023 at 04:01:28PM -0800, Andrii Nakryiko wrote:
> > > >
> > > > I agree this is simpler, but I'm not sure it will work properly. Verifier won't
> > > > know when the lifetime of the buffer ends, so if we disallow spills until its
> > > > written over it's going to be a pain for users.
> > > >
> > > > Something like:
> > > >
> > > > for (...) {
> > > >         char buf[64];
> > > >         bpf_dynptr_slice_rdwr(..., buf, 64);
> > > >         ...
> > > > }
> > > >
> > > > .. and then compiler decides to spill something where buf was located on stack
> > > > outside the for loop. The verifier can't know when buf goes out of scope to
> > > > unpoison the slots.
> > >
> > > You're saying the "verifier doesn't know when buf ...".
> > > The same applies to the compiler. It has no visibility
> > > into what bpf_dynptr_slice_rdwr is doing.
> >
> > That is true, it can't assume anything about the side effects. But I am talking
> > about the point in the program when the buffer object no longer lives. Use of
> > the escaped pointer to such an object any longer is UB. The compiler is well
> > within its rights to reuse its stack storage at that point, including for
> > spilling registers. Which is why "outside the for loop" in my earlier reply.
> >
> > > So it never spills into a declared C array
> > > as I tried to explain in the previous reply.
> > > Spill/fill slots are always invisible to C.
> > > (unless of course you do pointer arithmetic asm style)
> >
> > When the declared array's lifetime ends, it can.
> > https://godbolt.org/z/Ez7v4xfnv
> >
> > The 2nd call to bar as part of unrolled loop happens with fp-8, then it calls
> > baz, spills r0 to fp-8, and calls bar again with fp-8.

Right. If user writes such program and does explicit store of spillable
pointer into a stack.
I was talking about compiler generated spill/fill and I still believe
that compiler will not be reusing variable's stack memory for them.

> >
> > If such a stack slot is STACK_POISON, verifier will reject this program.

Yes and I think it's an ok trade-off.
The user has to specifically code such program to hit this issue.
I don't think we will see this in practice.
If we do we can consider a more complex fix.

> >
> > >
> > > > > > +       *(void **)eth = (void *)0xdeadbeef;
> > > > > > +       ctx = *(void **)buffer;
> > > > > > +       eth_proto = eth->eth_proto + ctx->len;
> > > > > >         if (eth_proto == bpf_htons(ETH_P_IP))
> > > > > >                 err = process_packet(&ptr, eth, nh_off, false, ctx);
> > > > > >
> > > > > > I think the proper fix is to treat it as a separate return type distinct from
> > > > > > PTR_TO_MEM like PTR_TO_MEM_OR_PKT (or handle PTR_TO_MEM | DYNPTR_* specially),
> > > > > > fork verifier state whenever there is a write, so that one path verifies it as
> > > > > > PTR_TO_PACKET, while another as PTR_TO_STACK (if buffer was a stack ptr). I
> > > > > > think for the rest it's not a problem, but there are allow_ptr_leak checks
> > > > > > applied to PTR_TO_STACK and PTR_TO_MAP_VALUE, so that needs to be rechecked.
> > > > > > Then we ensure that program is safe in either path.
> > > > > >
> > > > > > Also we need to fix regsafe to not consider other PTR_TO_MEMs equivalent to such
> > > > > > a pointer. We could also fork verifier states on return, to verify either path
> > > > > > separately right from the point following the call instruction.
> > > > >
> > > > > This is too complex imo.
> > > >
> > > > A better way to phrase this is to verify with R0 = PTR_TO_PACKET in one path,
> > > > and push_stack with R0 = buffer's reg->type + size set to len in the other path
> > > > for exploration later. In terms of verifier infra everything is there already,
> > > > it just needs to analyze both cases which fall into the regular code handling
> > > > the reg->type's. Probably then no adjustments to regsafe are needed either. It's
> > > > like exploring branch instructions.
> > >
> > > I still don't like it. There is no reason to go a complex path
> > > when much simpler suffices.
> 
> This issue you are discussing is the reason we don't support
> bpf_dynptr_from_mem() taking PTR_TO_STACK (which is a pity, but we
> postponed it initially).
> 
> I've been thinking about something along the lines of STACK_POISON,
> but remembering associated id/ref_obj_id. When ref is released, turn
> STACK_POISON to STACK_MISC. If it's bpf_dynptr_slice_rdrw() or
> bpf_dynptr_from_mem(), which don't have ref_obj_id, they still have ID
> associated with returned pointer, so can we somehow incorporate that?

There is dynptr_id in PTR_TO_MEM that is used by destroy_if_dynptr_stack_slot(),
but I don't see how we can use it to help this case.
imo plain STACK_POISON that is overwriteable by STACK_MISC/STACK_ZERO
should be good enough in practice.

We can potentially do some liveness trick. When PTR_TO_MEM with dynptr_id becomes
REG_LIVE_DONE we can convert STACK_POISON. But I'd go with the simplest approach first.

Powered by blists - more mailing lists