From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Song Liu <song@kernel.org>
Cc: bpf@vger.kernel.org, fsverity@lists.linux.dev, ast@kernel.org,
daniel@iogearbox.net, andrii@kernel.org, martin.lau@kernel.org,
kernel-team@meta.com, ebiggers@kernel.org, tytso@mit.edu,
roberto.sassu@huaweicloud.com, kpsingh@kernel.org
Subject: Re: [PATCH v7 bpf-next 1/9] bpf: Expose bpf_dynptr_slice* kfuncs for in kernel use
Date: Thu, 2 Nov 2023 10:25:18 -0700 [thread overview]
Message-ID: <CAEf4BzYtfjZRq5xfAZJUmBS=bWVp6JP87Dv3dySyW_4hXwKoRQ@mail.gmail.com> (raw)
In-Reply-To: <20231102165432.1769965-2-song@kernel.org>
On Thu, Nov 2, 2023 at 9:54 AM 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(-)
>
Oh, what a bad timing, I was too late (or too early, depending on how
you look) to comment on issues I've found. Please take a look comments
on v6. I think your usage of dynptr needs fixing.
> 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
> + * 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
>
next prev parent reply other threads:[~2023-11-02 17:25 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-02 16:54 [PATCH v7 bpf-next 0/9] bpf: File verification with LSM and fsverity Song Liu
2023-11-02 16:54 ` [PATCH v7 bpf-next 1/9] bpf: Expose bpf_dynptr_slice* kfuncs for in kernel use Song Liu
2023-11-02 17:25 ` Andrii Nakryiko [this message]
2023-11-02 16:54 ` [PATCH v7 bpf-next 2/9] bpf: Factor out helper check_reg_const_str() Song Liu
2023-11-02 16:54 ` [PATCH v7 bpf-next 3/9] bpf: Introduce KF_ARG_PTR_TO_CONST_STR Song Liu
2023-11-02 16:54 ` [PATCH v7 bpf-next 4/9] bpf: Add kfunc bpf_get_file_xattr Song Liu
2023-11-02 16:54 ` [PATCH v7 bpf-next 5/9] bpf, fsverity: Add kfunc bpf_get_fsverity_digest Song Liu
2023-11-02 16:54 ` [PATCH v7 bpf-next 6/9] Documentation/bpf: Add documentation for filesystem kfuncs Song Liu
2023-11-02 16:54 ` [PATCH v7 bpf-next 7/9] selftests/bpf: Sort config in alphabetic order Song Liu
2023-11-02 16:54 ` [PATCH v7 bpf-next 8/9] selftests/bpf: Add tests for filesystem kfuncs Song Liu
2023-11-02 16:54 ` [PATCH v7 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='CAEf4BzYtfjZRq5xfAZJUmBS=bWVp6JP87Dv3dySyW_4hXwKoRQ@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=kpsingh@kernel.org \
--cc=martin.lau@kernel.org \
--cc=roberto.sassu@huaweicloud.com \
--cc=song@kernel.org \
--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).