Re: [PATCH bpf-next v4 1/3] bpf: Add skb dynptrs

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Thu, 25 Aug 2022 at 23:02, Joanne Koong <joannelkoong@gmail.com> wrote:
> [...]
> >
> > Related question, it seems we know statically if dynptr is read only
> > or not, so why even do all this hidden parameter passing and instead
> > just reject writes directly? You only need to be able to set
> > MEM_RDONLY on dynptr_data returned PTR_TO_PACKETs, and reject
> > dynptr_write when dynptr type is xdp/skb (and ctx is only one). That
> > seems simpler than checking it at runtime. Verifier already handles
> > MEM_RDONLY generically, you only need to add the guard for
> > check_packet_acces (and check_helper_mem_access for meta->raw_mode
> > under pkt case), and rejecting dynptr_write seems like a if condition.
>
> There will be other helper functions that do writes (eg memcpy to
> dynptrs, strncpy to dynptrs, probe read user to dynptrs, hashing
> dynptrs, ...) so it's more scalable if we reject these at runtime
> rather than enforce these at the verifier level. I also think it's
> cleaner to keep the verifier logic as simple as possible and do the
> checking in the helper.

I won't be pushing this further, since you know what you plan to add
in the future better, but I still disagree.

I'm guessing there might be dynptrs where this read only property is
set dynamically at runtime, which is why you want to go this route?
I.e. you might not know statically whether dynptr is read only or not?

My main confusion is the inconsistency here.

Right now the patch implicitly relies on may_access_direct_pkt_data to
protect slices returned from dynptr_data, instead of setting
MEM_RDONLY on the returned PTR_TO_PACKET. Which is fine, it's not
needed. So indirectly, you are relying on knowing statically whether
the dynptr is read only or not. But then you also set this bit at
runtime.

So you reject some cases at load time, and the rest of them only at
runtime. Direct writes to dynptr slice fails load, writes through
helper does not (only fails at runtime).

Also, dynptr_data needs to know whether dynptr is read only
statically, to protect writes to its returned pointer, unless you
decide to introduce another helper for the dynamic rdonly bit case
(like dynptr_data_rdonly). Then you have a mismatch, where dynptr_data
works for some rdonly dynptrs (known to be rdonly statically, like
this skb one), but not for others.

I also don't agree about the complexity or scalability part, all the
infra and precedence is already there. We already have similar checks
for meta->raw_mode where we reject writes to read only pointers in
check_helper_mem_access.