On Fri, Nov 3, 2023 at 5:13 PM Song Liu <song@xxxxxxxxxx> wrote:
>
> Different types of bpf dynptr have different internal data storage.
> Specifically, SKB and XDP type of dynptr may have non-continuous data.
> Therefore, it is not always safe to directly access dynptr->data.
>
> Add __bpf_dynptr_data and __bpf_dynptr_data_rw to replace direct access to
> dynptr->data.
>
> Update bpf_verify_pkcs7_signature to use __bpf_dynptr_data instead of
> dynptr->data.
>
> Signed-off-by: Song Liu <song@xxxxxxxxxx>
> ---
> include/linux/bpf.h | 2 ++
> kernel/bpf/helpers.c | 47 ++++++++++++++++++++++++++++++++++++++++
> kernel/trace/bpf_trace.c | 12 ++++++----
> 3 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b4825d3cdb29..eb84caf133df 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -1222,6 +1222,8 @@ enum bpf_dynptr_type {
>
> int bpf_dynptr_check_size(u32 size);
> u32 __bpf_dynptr_size(const struct bpf_dynptr_kern *ptr);
> +const void *__bpf_dynptr_data(const struct bpf_dynptr_kern *ptr, u32 len);
> +void *__bpf_dynptr_data_rw(const struct bpf_dynptr_kern *ptr, u32 len);
>
> #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..c569c4c43bde 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -2611,3 +2611,50 @@ static int __init kfunc_init(void)
> }
>
> late_initcall(kfunc_init);
> +
> +/* Get a pointer to dynptr data up to len bytes for read only access. If
> + * the dynptr doesn't have continuous data up to len bytes, return NULL.
> + */
> +const void *__bpf_dynptr_data(const struct bpf_dynptr_kern *ptr, u32 len)
> +{
> + enum bpf_dynptr_type type;
> + int err;
> +
> + if (!ptr->data)
> + return NULL;
> +
> + err = bpf_dynptr_check_off_len(ptr, 0, len);
> + if (err)
> + return NULL;
> + type = bpf_dynptr_get_type(ptr);
> +
> + switch (type) {
> + case BPF_DYNPTR_TYPE_LOCAL:
> + case BPF_DYNPTR_TYPE_RINGBUF:
> + return ptr->data + ptr->offset;
> + case BPF_DYNPTR_TYPE_SKB:
> + return skb_pointer_if_linear(ptr->data, ptr->offset, len);
> + case BPF_DYNPTR_TYPE_XDP:
> + {
> + void *xdp_ptr = bpf_xdp_pointer(ptr->data, ptr->offset, len);
> +
> + if (IS_ERR_OR_NULL(xdp_ptr))
> + return NULL;
> + return xdp_ptr;
> + }
> + default:
> + WARN_ONCE(true, "unknown dynptr type %d\n", type);
> + return NULL;
> + }
> +}
> +
Song, you basically reimplemented bpf_dynptr_slice() but didn't unify
the code. Now we have two almost identical non-trivial functions we'd
need to update every time someone adds a new type of dynptr. Why not
have common helper that does everything both bpf_dynptr_slice() kfunc
needs and __bpf_dynptr_data() needs. And then call into it from both,
keeping all the LOCAL vs RINGBUF vs SKB vs XDP logic in one place?
Is there some problem unifying them?
> +/* Get a pointer to dynptr data up to len bytes for read write access. If
> + * the dynptr doesn't have continuous data up to len bytes, or the dynptr
> + * is read only, return NULL.
> + */
> +void *__bpf_dynptr_data_rw(const struct bpf_dynptr_kern *ptr, u32 len)
> +{
> + if (__bpf_dynptr_is_rdonly(ptr))
> + return NULL;
> + return (void *)__bpf_dynptr_data(ptr, len);
> +}
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index df697c74d519..d525a22b8d56 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1378,6 +1378,8 @@ __bpf_kfunc int bpf_verify_pkcs7_signature(struct bpf_dynptr_kern *data_ptr,
> struct bpf_dynptr_kern *sig_ptr,
> struct bpf_key *trusted_keyring)
> {
> + const void *data, *sig;
> + u32 data_len, sig_len;
> int ret;
>
> if (trusted_keyring->has_ref) {
> @@ -1394,10 +1396,12 @@ __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_len = __bpf_dynptr_size(data_ptr);
> + data = __bpf_dynptr_data(data_ptr, data_len);
> + sig_len = __bpf_dynptr_size(sig_ptr);
> + sig = __bpf_dynptr_data(sig_ptr, sig_len);
> +
> + return verify_pkcs7_signature(data, data_len, sig, sig_len,
> trusted_keyring->key,
> VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
> NULL);
> --
> 2.34.1
>
