Re: [PATCH bpf-next v4 4/6] bpf: Add bpf_dynptr_read and bpf_dynptr_write

[David Vernet <void@manifault.com>]

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@gmail.com>
> ---
>  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.