Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230406004018.1439952-1-drosen@google.com> <20230406004018.1439952-3-drosen@google.com>
 <CAEf4BzakRfffU9+wLBNfhBi1dKxs03ibopJsMyEF6JAM-QJWjw@mail.gmail.com> <CA+PiJmRZHKn4GyDu7NbWc612=ovgM7jODKSD76CSW0gbjzDfzg@mail.gmail.com>
In-Reply-To: <CA+PiJmRZHKn4GyDu7NbWc612=ovgM7jODKSD76CSW0gbjzDfzg@mail.gmail.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Thu, 6 Apr 2023 16:54:22 -0700
Message-ID: <CAEf4BzbnYM+=W3N9mun24v0QbCDTbm5rnpGAwxD4xat297+XZg@mail.gmail.com>
Subject: Re: [PATCH 2/3] bpf: Allow NULL buffers in bpf_dynptr_slice(_rw)
To:     Daniel Rosenberg <drosen@google.com>
Cc:     bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        John Fastabend <john.fastabend@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        KP Singh <kpsingh@kernel.org>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        Shuah Khan <shuah@kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Joanne Koong <joannelkoong@gmail.com>,
        Mykola Lysenko <mykolal@fb.com>, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org, kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Apr 6, 2023 at 3:25=E2=80=AFPM Daniel Rosenberg <drosen@google.com>=
 wrote:
>
> On Thu, Apr 6, 2023 at 2:09=E2=80=AFPM Andrii Nakryiko
> <andrii.nakryiko@gmail.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=3D0
> > 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 =3D=3D 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 explic=
itly:

  - 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=3D=3D0 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