On Mon, May 09, 2022 at 03:42:55PM -0700, Joanne Koong wrote:
> This patch adds two helper functions, bpf_dynptr_read and
> bpf_dynptr_write:
>
> long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset);
>
> long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len);
>
> The dynptr passed into these functions must be valid dynptrs that have
> been initialized.
>
> Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> ---
> include/linux/bpf.h | 16 ++++++++++
> include/uapi/linux/bpf.h | 19 ++++++++++++
> kernel/bpf/helpers.c | 56 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 19 ++++++++++++
> 4 files changed, 110 insertions(+)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index 8fbe739b0dec..6f4fa0627620 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -2391,6 +2391,12 @@ enum bpf_dynptr_type {
> #define DYNPTR_SIZE_MASK 0xFFFFFF
> #define DYNPTR_TYPE_SHIFT 28
> #define DYNPTR_TYPE_MASK 0x7
> +#define DYNPTR_RDONLY_BIT BIT(31)
> +
> +static inline bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr)
> +{
> + return ptr->size & DYNPTR_RDONLY_BIT;
> +}
>
> static inline enum bpf_dynptr_type bpf_dynptr_get_type(struct bpf_dynptr_kern *ptr)
> {
> @@ -2412,6 +2418,16 @@ static inline int bpf_dynptr_check_size(u32 size)
> return size > DYNPTR_MAX_SIZE ? -E2BIG : 0;
> }
>
> +static inline int bpf_dynptr_check_off_len(struct bpf_dynptr_kern *ptr, u32 offset, u32 len)
> +{
> + u32 size = bpf_dynptr_get_size(ptr);
> +
> + if (len > size || offset > size - len)
> + return -E2BIG;
> +
> + return 0;
> +}
Does this need to be in bpf.h? Or could it be brought into helpers.c as a
static function? I don't think there's any harm in leaving it here, but at
first glance it seems like a helper function that doesn't really need to be
exported.
> +
> void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data, enum bpf_dynptr_type type,
> u32 offset, u32 size);
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 679f960d2514..f0c5ca220d8e 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5209,6 +5209,23 @@ union bpf_attr {
> * 'bpf_ringbuf_discard'.
> * Return
> * Nothing. Always succeeds.
> + *
> + * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset)
> + * Description
> + * Read *len* bytes from *src* into *dst*, starting from *offset*
> + * into *src*.
> + * Return
> + * 0 on success, -E2BIG if *offset* + *len* exceeds the length
> + * of *src*'s data, -EINVAL if *src* is an invalid dynptr.
> + *
> + * long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len)
> + * Description
> + * Write *len* bytes from *src* into *dst*, starting from *offset*
> + * into *dst*.
> + * Return
> + * 0 on success, -E2BIG if *offset* + *len* exceeds the length
> + * of *dst*'s data, -EINVAL if *dst* is an invalid dynptr or if *dst*
> + * is a read-only dynptr.
> */
> #define __BPF_FUNC_MAPPER(FN) \
> FN(unspec), \
> @@ -5411,6 +5428,8 @@ union bpf_attr {
> FN(ringbuf_reserve_dynptr), \
> FN(ringbuf_submit_dynptr), \
> FN(ringbuf_discard_dynptr), \
> + FN(dynptr_read), \
> + FN(dynptr_write), \
> /* */
>
> /* integer value in 'imm' field of BPF_CALL instruction selects which helper
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 2d6f2e28b580..7206b9e5322f 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1467,6 +1467,58 @@ const struct bpf_func_proto bpf_dynptr_put_proto = {
> .arg1_type = ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_MALLOC | OBJ_RELEASE,
> };
>
> +BPF_CALL_4(bpf_dynptr_read, void *, dst, u32, len, struct bpf_dynptr_kern *, src, u32, offset)
> +{
> + int err;
> +
> + if (!src->data)
> + return -EINVAL;
> +
> + err = bpf_dynptr_check_off_len(src, offset, len);
> + if (err)
> + return err;
> +
> + memcpy(dst, src->data + src->offset + offset, len);
> +
> + return 0;
> +}
> +
> +const struct bpf_func_proto bpf_dynptr_read_proto = {
> + .func = bpf_dynptr_read,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_UNINIT_MEM,
> + .arg2_type = ARG_CONST_SIZE_OR_ZERO,
> + .arg3_type = ARG_PTR_TO_DYNPTR,
> + .arg4_type = ARG_ANYTHING,
I think what you have now is safe / correct, but is there a reason that we
don't use ARG_CONST_SIZE_OR_ZERO for both the len and the offset, given
that they're both bound by the size of a memory region? Same question
applies to the function proto for bpf_dynptr_write() as well.
> +};
> +
> +BPF_CALL_4(bpf_dynptr_write, struct bpf_dynptr_kern *, dst, u32, offset, void *, src, u32, len)
> +{
> + int err;
> +
> + if (!dst->data || bpf_dynptr_is_rdonly(dst))
> + return -EINVAL;
> +
> + err = bpf_dynptr_check_off_len(dst, offset, len);
> + if (err)
> + return err;
> +
> + memcpy(dst->data + dst->offset + offset, src, len);
> +
> + return 0;
> +}
> +
> +const struct bpf_func_proto bpf_dynptr_write_proto = {
> + .func = bpf_dynptr_write,
> + .gpl_only = false,
> + .ret_type = RET_INTEGER,
> + .arg1_type = ARG_PTR_TO_DYNPTR,
> + .arg2_type = ARG_ANYTHING,
> + .arg3_type = ARG_PTR_TO_MEM | MEM_RDONLY,
> + .arg4_type = ARG_CONST_SIZE_OR_ZERO,
> +};
> +
[...]
Overall looks great.
