[v7,bpf-next,1/9] bpf: Expose bpf_dynptr_slice* kfuncs for in kernel use

Commit Message

Song Liu Nov. 2, 2023, 4:54 p.m. UTC

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 <[e-mail]>
---
 include/linux/bpf.h      |  4 ++++
 kernel/bpf/helpers.c     | 16 ++++++++--------
 kernel/trace/bpf_trace.c | 15 +++++++++++----
 3 files changed, 23 insertions(+), 12 deletions(-)

Comments

Andrii Nakryiko Nov. 2, 2023, 5:25 p.m. UTC | #1

On Thu, Nov 2, 2023 at 9:54 AM Song Liu <[e-mail]> 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 <[e-mail]>
> ---
>  include/linux/bpf.h      |  4 ++++
>  kernel/bpf/helpers.c     | 16 ++++++++--------
>  kernel/trace/bpf_trace.c | 15 +++++++++++----
>  3 files changed, 23 insertions(+), 12 deletions(-)
>

Oh, what a bad timing, I was too late (or too early, depending on how
you look) to comment on issues I've found. Please take a look comments
on v6. I think your usage of dynptr needs fixing.


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

Patch

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
+ * 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);