From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Song Liu <songliubraving@meta.com>
Cc: Song Liu <song@kernel.org>, bpf <bpf@vger.kernel.org>,
"fsverity@lists.linux.dev" <fsverity@lists.linux.dev>,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Andrii Nakryiko <andrii@kernel.org>,
Martin KaFai Lau <martin.lau@kernel.org>,
Kernel Team <kernel-team@meta.com>,
Eric Biggers <ebiggers@kernel.org>,
"Theodore Ts'o" <tytso@mit.edu>,
"roberto.sassu@huaweicloud.com" <roberto.sassu@huaweicloud.com>
Subject: Re: [PATCH v6 bpf-next 1/9] bpf: Expose bpf_dynptr_slice* kfuncs for in kernel use
Date: Thu, 2 Nov 2023 12:06:50 -0700 [thread overview]
Message-ID: <CAEf4BzY_aW0-Ao9vaYoRJ=A3b2=VYqyAX--4UK6CYv7QGAjzgg@mail.gmail.com> (raw)
In-Reply-To: <B1DDF0D9-5365-4421-83AF-E7F6C0439422@fb.com>
On Thu, Nov 2, 2023 at 11:16 AM Song Liu <songliubraving@meta.com> wrote:
>
>
>
> > On Nov 2, 2023, at 11:09 AM, Song Liu <songliubraving@meta.com> wrote:
> >
> >
> >
> >> On Nov 2, 2023, at 10:16 AM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >>
> >> On Thu, Nov 2, 2023 at 10:09 AM Andrii Nakryiko
> >> <andrii.nakryiko@gmail.com> wrote:
> >>>
> >>> On Thu, Nov 2, 2023 at 9:56 AM Andrii Nakryiko
> >>> <andrii.nakryiko@gmail.com> wrote:
> >>>>
> >>>> On Tue, Oct 24, 2023 at 4:56 PM Song Liu <song@kernel.org> 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@kernel.org>
> >>>>> ---
> >>>>> 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.
> >
>
next prev parent reply other threads:[~2023-11-02 19:07 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-24 23:55 [PATCH v6 bpf-next 0/9] bpf: File verification with LSM and fsverity Song Liu
2023-10-24 23:55 ` [PATCH v6 bpf-next 1/9] bpf: Expose bpf_dynptr_slice* kfuncs for in kernel use Song Liu
2023-11-02 16:56 ` Andrii Nakryiko
2023-11-02 17:09 ` Andrii Nakryiko
2023-11-02 17:16 ` Andrii Nakryiko
2023-11-02 18:09 ` Song Liu
2023-11-02 18:12 ` Andrii Nakryiko
2023-11-02 18:16 ` Song Liu
2023-11-02 19:06 ` Andrii Nakryiko [this message]
2023-10-24 23:55 ` [PATCH v6 bpf-next 2/9] bpf: Factor out helper check_reg_const_str() Song Liu
2023-11-02 16:58 ` Andrii Nakryiko
2023-10-24 23:55 ` [PATCH v6 bpf-next 3/9] bpf: Introduce KF_ARG_PTR_TO_CONST_STR Song Liu
2023-11-02 5:58 ` Alexei Starovoitov
2023-11-02 6:09 ` Song Liu
2023-10-24 23:55 ` [PATCH v6 bpf-next 4/9] bpf: Add kfunc bpf_get_file_xattr Song Liu
2023-11-02 17:01 ` Andrii Nakryiko
2023-10-24 23:55 ` [PATCH v6 bpf-next 5/9] bpf, fsverity: Add kfunc bpf_get_fsverity_digest Song Liu
2023-11-02 3:11 ` Eric Biggers
2023-10-24 23:55 ` [PATCH v6 bpf-next 6/9] Documentation/bpf: Add documentation for filesystem kfuncs Song Liu
2023-10-24 23:55 ` [PATCH v6 bpf-next 7/9] selftests/bpf: Sort config in alphabetic order Song Liu
2023-10-24 23:55 ` [PATCH v6 bpf-next 8/9] selftests/bpf: Add tests for filesystem kfuncs Song Liu
2023-10-24 23:55 ` [PATCH v6 bpf-next 9/9] selftests/bpf: Add test that uses fsverity and xattr to sign a file Song Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEf4BzY_aW0-Ao9vaYoRJ=A3b2=VYqyAX--4UK6CYv7QGAjzgg@mail.gmail.com' \
--to=andrii.nakryiko@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=ebiggers@kernel.org \
--cc=fsverity@lists.linux.dev \
--cc=kernel-team@meta.com \
--cc=martin.lau@kernel.org \
--cc=roberto.sassu@huaweicloud.com \
--cc=song@kernel.org \
--cc=songliubraving@meta.com \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).