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?
>
> 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.
>
>
> > > + * 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)
> > > */
> > > __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset,
> > > void *buffer__opt, u32 buffer__szk)
> > > @@ -2354,10 +2354,10 @@ __bpf_kfunc void *bpf_dynptr_slice(const struct bpf_dynptr_kern *ptr, u32 offset
> > > * bpf_dynptr_slice_rdwr 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
> > > - * 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
> > > + * to a 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)
> > > */
> > > __bpf_kfunc void *bpf_dynptr_slice_rdwr(const struct bpf_dynptr_kern *ptr, u32 offset,
> > > void *buffer__opt, u32 buffer__szk)
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index df697c74d519..2626706b6387 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -1378,6 +1378,7 @@ __bpf_kfunc int bpf_verify_pkcs7_signature(struct bpf_dynptr_kern *data_ptr,
> > > struct bpf_dynptr_kern *sig_ptr,
> > > struct bpf_key *trusted_keyring)
> > > {
> > > + void *data, *sig;
> > > int ret;
> > >
> > > if (trusted_keyring->has_ref) {
> > > @@ -1394,10 +1395,16 @@ __bpf_kfunc int bpf_verify_pkcs7_signature(struct bpf_dynptr_kern *data_ptr,
> > > return ret;
> > > }
> > >
> > > - return verify_pkcs7_signature(data_ptr->data,
> > > - __bpf_dynptr_size(data_ptr),
> > > - sig_ptr->data,
> > > - __bpf_dynptr_size(sig_ptr),
> > > + data = bpf_dynptr_slice(data_ptr, 0, NULL, 0);
> > > + if (IS_ERR(data))
> > > + return PTR_ERR(data);
> > > +
> > > + sig = bpf_dynptr_slice(sig_ptr, 0, NULL, 0);
> > > + if (IS_ERR(sig))
> > > + return PTR_ERR(sig);
> > > +
> > > + return verify_pkcs7_signature(data, __bpf_dynptr_size(data_ptr),
> > > + sig, __bpf_dynptr_size(sig_ptr),
> > > trusted_keyring->key,
> > > VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
> > > NULL);
> > > --
> > > 2.34.1
> > >
> > >
