Re: [PATCH 2/3] bpf: Allow NULL buffers in bpf_dynptr_slice(_rw)

[Andrii Nakryiko]

On Thu, Apr 6, 2023 at 3:25 PM Daniel Rosenberg <drosen@xxxxxxxxxx> wrote:
>
> On Thu, Apr 6, 2023 at 2:09 PM Andrii Nakryiko
> <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > should we always reject NULL even for SKB/XDP or only when the buffer
> > *would be* required? If the latter, we could use bpf_dynptr_slice()
> > with NULL buf to say "only return pointer if no byte copying is
> > required". As opposed to bpf_dynptr_data(), where I think we always
> > fail for SKB/XDP, because we are not sure whether users are aware of
> > this need to copy bytes. Here, users are aware, but chose to prevent
> > copying.
> >
> > WDYT?
> >
>
> I think Passing NULL here should signal that you're quite okay with it
> failing instead of copying. We could limit this to just local/ringbuf
> types, but that seems like an unneeded restriction, particularly if
> you're operating on some section of an skb/xdp buffer that you know
> will always be contiguous.
> I adjusted xdp for that. The adjustment would be similar for skb, I
> just didn't do that since it was another layer of indirection deep and
> hadn't looked through all of those use cases. Though it should be fine
> to just reject when the buffer would have been needed, since all users
> currently provide one.
> I agree that allowing that same behavior for dnyptr_data would be more
> likely to cause confusion. Blocking the copy on dynprt_slice is much
> more explicit.
>
> >
> > would this work correctly if someone passes a non-null buffer with too
> > small size? Can you please add a test for this use case.
> >
>
> Yes, that's one of the tests that's missing there. Once I get my build
> situation sorted I'll add more tests. The behavior for a non-null
> buffer should be unchanged by this patch.

cool, sounds good

>
> > Also, I feel like for cases where we allow a NULL buffer, we need to
> > explicitly check that the register is a *known* NULL (SCALAR=0
> > basically). And also in that case the size of the buffer probably
> > should be enforced to zero, not just be allowed to be any value.
> >
>
> We absolutely should check that the pointer in question is NULL before
> ignoring the size check. I think I'm accomplishing that by ignoring
> __opt when reg->umax_value > 0 in is_kfunc_arg_optional. Is that the
> wrong check? Perhaps I should check var_off == tnum_const(0) instead.

yeah, umax_value is probably wrong check, use register_is_null()

but this approach, even if correct, is a bit too subtle. I'd code it explicitly:

  - if __opt, then we know it *can be NULL*
  - if so, we need to consider two situations
    - it is NULL, then don't enforce buffer size
    - it is not NULL (or may be not NULL), then enforce buffer size

Basically, conflating check whether argument is marked as opt with
enforcement of all the implied conditions seems very error-prone.

>
> We can't enforce the size being zero in this case because the size is
> doing double duty. It's both the length of the requested area of
> access into the dnyptr, and the size of the buffer that it might copy

yep, completely missed this double use of that constant, ignore my
point about enforcing sz==0 then

> that data into. If we don't provide a buffer, then it doesn't make
> sense to check that buffer's size. The size does still apply to the
> returned pointer though. Within the kfunc, it just needs to check for

yep

> null before copying dynptr data, as well as the regular enforcement of
> length against the dynprt/offset.
>
> > it's scary to just ignore some error, tbh, the number of error
> > conditions can grow overtime and we'll be masking them with this
> > is_kfunc_arg_optional() override. Let's be strict and explicit here.
> >
> It would probably make more sense to check is_kfunc_arg_optional and
> skip the size check altogether. Either way we're just relying on
> runtime checks against the dynptr at that point. If the buffer is
> known null and optional, we don't care what the relationship between
> the buffer and the size is, just that size and the dynptr. __szk
> already takes care of it being a constant. This doesn't affect the
> return buffer size.

yep, I agree about the logic, I'm concerned with the conflated
implementation, as I tried to explain above