Message-ID: <CAEf4BzbnYM+=W3N9mun24v0QbCDTbm5rnpGAwxD4xat297+XZg@mail.gmail.com>
Date:   Thu, 6 Apr 2023 16:54:22 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Daniel Rosenberg <drosen@...gle.com>
Cc:     bpf@...r.kernel.org, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        John Fastabend <john.fastabend@...il.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        KP Singh <kpsingh@...nel.org>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Shuah Khan <shuah@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Joanne Koong <joannelkoong@...il.com>,
        Mykola Lysenko <mykolal@...com>, linux-kernel@...r.kernel.org,
        linux-kselftest@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH 2/3] bpf: Allow NULL buffers in bpf_dynptr_slice(_rw)

On Thu, Apr 6, 2023 at 3:25 PM Daniel Rosenberg <drosen@...gle.com> wrote:
>
> On Thu, Apr 6, 2023 at 2:09 PM Andrii Nakryiko
> <andrii.nakryiko@...il.com> 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

Powered by blists - more mailing lists