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?
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.
> + * 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
>
>
