On Thu, Nov 2, 2023 at 11:16 AM Song Liu <songliubraving@xxxxxxxx> wrote: > > > > > On 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. > > > >>> > >>> 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. > > By the way, the syntax and comment of bpf_dynptr_slice() is confusing: > > * @buffer__opt: User-provided buffer to copy contents into. May be NULL > * @buffer__szk: Size (in bytes) of the buffer if present. This is the > * length of the requested slice. This must be a constant. > > It reads (to me) as, "if buffer__opt is NULL, buffer__szk should be 0". > > Is this just my confusion, or is there a real issue? It's a bit confusing, but no, that size needs to be a "how much data do I want to read/write", so even if buffer is NULL, size has to be specified. "This is the length of the requested slice." is the most important part in that comment. > > Thanks, > Song > > > >> > >> 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. > > >