On Thu, Nov 2, 2023 at 11:09 AM Song Liu <songliubraving@xxxxxxxx> wrote:
>
>
>
> > On Nov 2, 2023, at 10:16 AM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
> >
> > On Thu, Nov 2, 2023 at 10:09 AM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> >>
> >> On Thu, Nov 2, 2023 at 9:56 AM Andrii Nakryiko
> >> <andrii.nakryiko@xxxxxxxxx> wrote:
> >>>
> >>> On Tue, Oct 24, 2023 at 4:56 PM Song Liu <song@xxxxxxxxxx> wrote:
> >>>>
> >>>> kfuncs bpf_dynptr_slice and bpf_dynptr_slice_rdwr are used by BPF programs
> >>>> to access the dynptr data. They are also useful for in kernel functions
> >>>> that access dynptr data, for example, bpf_verify_pkcs7_signature.
> >>>>
> >>>> Add bpf_dynptr_slice and bpf_dynptr_slice_rdwr to bpf.h so that kernel
> >>>> functions can use them instead of accessing dynptr->data directly.
> >>>>
> >>>> Update bpf_verify_pkcs7_signature to use bpf_dynptr_slice instead of
> >>>> dynptr->data.
> >>>>
> >>>> Also, update the comments for bpf_dynptr_slice and bpf_dynptr_slice_rdwr
> >>>> that they may return error pointers for BPF_DYNPTR_TYPE_XDP.
> >>>>
> >>>> Signed-off-by: Song Liu <song@xxxxxxxxxx>
> >>>> ---
> >>>> include/linux/bpf.h | 4 ++++
> >>>> kernel/bpf/helpers.c | 16 ++++++++--------
> >>>> kernel/trace/bpf_trace.c | 15 +++++++++++----
> >>>> 3 files changed, 23 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> >>>> index b4825d3cdb29..3ed3ae37cbdf 100644
> >>>> --- a/include/linux/bpf.h
> >>>> +++ b/include/linux/bpf.h
> >>>> @@ -1222,6 +1222,10 @@ enum bpf_dynptr_type {
> >>>>
> >>>> int bpf_dynptr_check_size(u32 size);
> >>>> u32 __bpf_dynptr_size(const struct bpf_dynptr_kern *ptr);
> >>>> +void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset,
> >>>> + void *buffer__opt, u32 buffer__szk);
> >>>> +void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 offset,
> >>>> + void *buffer__opt, u32 buffer__szk);
> >>>>
> >>>> #ifdef CONFIG_BPF_JIT
> >>>> int bpf_trampoline_link_prog(struct bpf_tramp_link *link, struct bpf_trampoline *tr);
> >>>> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> >>>> index e46ac288a108..af5059f11e83 100644
> >>>> --- a/kernel/bpf/helpers.c
> >>>> +++ b/kernel/bpf/helpers.c
> >>>> @@ -2270,10 +2270,10 @@ __bpf_kfunc struct task_struct *bpf_task_from_pid(s32 pid)
> >>>> * bpf_dynptr_slice will not invalidate any ctx->data/data_end pointers in
> >>>> * the bpf program.
> >>>> *
> >>>> - * Return: NULL if the call failed (eg invalid dynptr), pointer to a read-only
> >>>> - * data slice (can be either direct pointer to the data or a pointer to the user
> >>>> - * provided buffer, with its contents containing the data, if unable to obtain
> >>>> - * direct pointer)
> >>>> + * Return: NULL or error pointer if the call failed (eg invalid dynptr), pointer
> >>>
> >>> Hold on, nope, this one shouldn't return error pointer because it's
> >>> used from BPF program side and BPF program is checking for NULL only.
> >>> Does it actually return error pointer, though?
>
> Right. kfunc should not return error pointer.
Turns out it doesn't, see discussion in [0]. Maybe you can sneak in
that comment in your next revision as a separate lightweight patch :)
[0] https://lore.kernel.org/bpf/CAEf4Bzb4VbH56S2D_5Sc3u9V=OXOy20JTr4wsObBOiUA32Md2Q@xxxxxxxxxxxxxx/
>
> >>
> >> So I just checked the code (should have done it before replying,
> >> sorry). It is a bug that slipped through when adding bpf_xdp_pointer()
> >> usage. We should always return NULL from this kfunc on error
> >> conditions. Let's fix it separately, but please don't change the
> >> comments.
> >>
> >>>
> >>> I'm generally skeptical of allowing to call kfuncs directly from
> >>> internal kernel code, tbh, and concerns like this are one reason why.
> >>> BPF verifier sets up various conditions that kfuncs have to follow,
> >>> and it seems error-prone to mix this up with internal kernel usage.
> >>>
> >>
> >> Reading bpf_dynptr_slice_rdwr code, it does look exactly like what you
> >> want, despite the confusingly-looking 0, NULL, 0 arguments. So I guess
> >> I'm fine exposing it directly, but it still feels like it will bite us
> >> at some point later.
> >
> > Ok, now I'm at patch #5. Think about what you are doing here. You are
> > asking bpf_dynptr_slice_rdrw() if you can get a directly writable
> > pointer to a data area of length *zero*. So if it's SKB, for example,
> > then yeah, you'll be granted a pointer. But then you are proceeding to
> > write up to sizeof(struct fsverity_digest) bytes, and that can cross
> > into non-contiguous memory.
> >
> > So I'll take it back, let's not expose this kfunc directly to kernel
> > code. Let's have a separate internal helper that will return either
> > valid pointer or NULL for a given dynptr, but will require valid
> > non-zero max size. Something with the signature like below
> >
> > void * __bpf_dynptr_data_rw(const struct bpf_dynptr_kern *ptr, u32 len);
> >
> > If ptr can provide a direct pointer to memory of length *len*, great.
> > If not, return NULL. This will be an appropriate internal API for all
> > the use cases you are adding where we will be writing back into dynptr
> > from other kernel APIs with the assumption of contiguous memory
> > region.
>
> Sounds good. Fixing this in the next version.
>
> Thanks,
> Song
>
