On Fri, Nov 3, 2023 at 2:46 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 | 10 +++++----
> 3 files changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index b4825d3cdb29..129c5a7c5982 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);
> +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..ddd1a5a81652 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.
> + */
> +void *__bpf_dynptr_data(const struct bpf_dynptr_kern *ptr, u32 len)
If it's read-only, then this should return `const void *`?
> +{
> + 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;
> + }
> +}
> +
> +/* 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 __bpf_dynptr_data(ptr, len);
and then here we can cast to (void *) because we checked is_rdonly
above, I think it would be ok to do that
> +}
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index df697c74d519..bfe6fb83e8d0 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,11 @@ __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_data(data_ptr, __bpf_dynptr_size(data_ptr));
> + sig = __bpf_dynptr_data(sig_ptr, __bpf_dynptr_size(sig_ptr));
> +
> + 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
>
