[RFC,1/4] bpf: Allow creating dynptr from uptr

Commit Message

Amery Hung March 20, 2025, 9:40 p.m. UTC

Currently, bpf_dynptr_from_mem() only allows creating dynptr from local
memory of reg type PTR_TO_MAP_VALUE, specifically ringbuf. This patch
futher supports PTR_TO_MEM as a valid source of data.

For a reg to be PTR_TO_MEM in the verifier:
 - read map value with special field BPF_UPTR
 - ld_imm64 kfunc (MEM_RDONLY)
 - ld_imm64 other non-struct ksyms (MEM_RDONLY)
 - return from helper with RET_PTR_TO_MEM: ringbuf_reserve (MEM_RINGBUF)
   and dynptr_from_data
 - return from helper with RET_PTR_TO_MEM_OR_BTF_ID: this_cpu_ptr,
   per_cpu_ptr and the return type is not struct (both MEM_RDONLY)
 - return from special kfunc: dynptr_slice (MEM_RDONLY), dynptr_slice_rdwr
 - return from non-special kfunc that returns non-struct pointer:
   hid_bpf_get_data

Since this patch only allows PTR_TO_MEM without any flags, so only uptr,
global subprog argument, non-special kfunc that returns non-struct ptr,
return of bpf_dynptr_slice_rdwr() and bpf_dynptr_data() will be allowed
additionally.

The last two will allow creating dynptr from dynptr data. Will they create
any problem?

Signed-off-by: Amery Hung <[e-mail]>
---
 include/uapi/linux/bpf.h | 4 +++-
 kernel/bpf/verifier.c    | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

Comments

Andrii Nakryiko March 20, 2025, 10:45 p.m. UTC | #1

On Thu, Mar 20, 2025 at 2:41 PM Amery Hung <[e-mail]> wrote:
>
> Currently, bpf_dynptr_from_mem() only allows creating dynptr from local
> memory of reg type PTR_TO_MAP_VALUE, specifically ringbuf. This patch
> futher supports PTR_TO_MEM as a valid source of data.
>
> For a reg to be PTR_TO_MEM in the verifier:
>  - read map value with special field BPF_UPTR
>  - ld_imm64 kfunc (MEM_RDONLY)
>  - ld_imm64 other non-struct ksyms (MEM_RDONLY)
>  - return from helper with RET_PTR_TO_MEM: ringbuf_reserve (MEM_RINGBUF)
>    and dynptr_from_data
>  - return from helper with RET_PTR_TO_MEM_OR_BTF_ID: this_cpu_ptr,
>    per_cpu_ptr and the return type is not struct (both MEM_RDONLY)
>  - return from special kfunc: dynptr_slice (MEM_RDONLY), dynptr_slice_rdwr
>  - return from non-special kfunc that returns non-struct pointer:
>    hid_bpf_get_data
>
> Since this patch only allows PTR_TO_MEM without any flags, so only uptr,
> global subprog argument, non-special kfunc that returns non-struct ptr,
> return of bpf_dynptr_slice_rdwr() and bpf_dynptr_data() will be allowed
> additionally.
>
> The last two will allow creating dynptr from dynptr data. Will they create
> any problem?

Yes, I think so. You need to make sure that dynptr you created from
that PTR_TO_MEM is invalidated if that memory "goes away". E.g., for
ringbuf case:

void *r = bpf_ringbuf_reserve(..., 100);

struct dynptr d;
bpf_dynptr_from_mem(r, 100, 0, &d);

void *p = bpf_dynptr_data(&d, 0, 100);
if (!p) return 0; /* can't happen */

bpf_ringbuf_submit(r, 0);


*(char *)p = '\0'; /* bad things happen */


Do you handle that situation? With PTR_TO_MAP_VALUE "bad things" can't
happen even if value is actually deleted/reused (besides overwriting
some other element's value, which we can do without dynptrs anyways),
because that memory won't go away due to RCU and it doesn't contain
any information important for correctness (ringbuf data area does have
it).


>
> Signed-off-by: Amery Hung <[e-mail]>
> ---
>  include/uapi/linux/bpf.h | 4 +++-
>  kernel/bpf/verifier.c    | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index beac5cdf2d2c..2b1335fa1173 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5562,7 +5562,9 @@ union bpf_attr {
>   *     Description
>   *             Get a dynptr to local memory *data*.
>   *
> - *             *data* must be a ptr to a map value.
> + *             *data* must be a ptr to valid local memory such as a map value, a uptr,
> + *             a null-checked non-void pointer pass to a global subprogram, and allocated
> + *             memory returned by a kfunc such as hid_bpf_get_data(),
>   *             The maximum *size* supported is DYNPTR_MAX_SIZE.
>   *             *flags* is currently unused.
>   *     Return
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 22c4edc8695c..d22310d1642c 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -11307,7 +11307,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>                 }
>                 break;
>         case BPF_FUNC_dynptr_from_mem:
> -               if (regs[BPF_REG_1].type != PTR_TO_MAP_VALUE) {
> +               if (regs[BPF_REG_1].type != PTR_TO_MAP_VALUE &&
> +                   regs[BPF_REG_1].type != PTR_TO_MEM) {
>                         verbose(env, "Unsupported reg type %s for bpf_dynptr_from_mem data\n",
>                                 reg_type_str(env, regs[BPF_REG_1].type));
>                         return -EACCES;
> --
> 2.47.1
>

Amery Hung March 20, 2025, 11:20 p.m. UTC | #2

On Thu, Mar 20, 2025 at 3:45 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Thu, Mar 20, 2025 at 2:41 PM Amery Hung <[e-mail]> wrote:
> >
> > Currently, bpf_dynptr_from_mem() only allows creating dynptr from local
> > memory of reg type PTR_TO_MAP_VALUE, specifically ringbuf. This patch
> > futher supports PTR_TO_MEM as a valid source of data.
> >
> > For a reg to be PTR_TO_MEM in the verifier:
> >  - read map value with special field BPF_UPTR
> >  - ld_imm64 kfunc (MEM_RDONLY)
> >  - ld_imm64 other non-struct ksyms (MEM_RDONLY)
> >  - return from helper with RET_PTR_TO_MEM: ringbuf_reserve (MEM_RINGBUF)
> >    and dynptr_from_data
> >  - return from helper with RET_PTR_TO_MEM_OR_BTF_ID: this_cpu_ptr,
> >    per_cpu_ptr and the return type is not struct (both MEM_RDONLY)
> >  - return from special kfunc: dynptr_slice (MEM_RDONLY), dynptr_slice_rdwr
> >  - return from non-special kfunc that returns non-struct pointer:
> >    hid_bpf_get_data
> >
> > Since this patch only allows PTR_TO_MEM without any flags, so only uptr,
> > global subprog argument, non-special kfunc that returns non-struct ptr,
> > return of bpf_dynptr_slice_rdwr() and bpf_dynptr_slice_rdwr() will be allowed
> > additionally.
> >
> > The last two will allow creating dynptr from dynptr data. Will they create
> > any problem?
>
> Yes, I think so. You need to make sure that dynptr you created from
> that PTR_TO_MEM is invalidated if that memory "goes away". E.g., for
> ringbuf case:
>
> void *r = bpf_ringbuf_reserve(..., 100);
>
> struct dynptr d;
> bpf_dynptr_from_mem(r, 100, 0, &d);
>

^ This will fail during verification because "r" will be PTR_TO_MEM |
MEM_RINGBUF.

Only five of the listed PTR_TO_MEM cases will be allowed with this
patch additionally: uptr, global subprog argument, hid_bpf_get_data,
bpf_dynptr_ptr_data and bpf_dynptr_slice_rdwr. For the former three,
the memory seems to be valid all the time. For the last two, IIUC,
bpf_dynptr_data or bpf_dynptr_slice_rdwr should be valid if null
checks pass. I am just so not sure about the nested situation (i.e.,
creating another dynptr from data behind a dynptr).

Thanks,
Amery

> void *p = bpf_dynptr_data(&d, 0, 100);
> if (!p) return 0; /* can't happen */
>
> bpf_ringbuf_submit(r, 0);
>
>
> *(char *)p = '\0'; /* bad things happen */
>
>
> Do you handle that situation? With PTR_TO_MAP_VALUE "bad things" can't
> happen even if value is actually deleted/reused (besides overwriting
> some other element's value, which we can do without dynptrs anyways),
> because that memory won't go away due to RCU and it doesn't contain
> any information important for correctness (ringbuf data area does have
> it).
>
>
> >
> > Signed-off-by: Amery Hung <[e-mail]>
> > ---
> >  include/uapi/linux/bpf.h | 4 +++-
> >  kernel/bpf/verifier.c    | 3 ++-
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index beac5cdf2d2c..2b1335fa1173 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5562,7 +5562,9 @@ union bpf_attr {
> >   *     Description
> >   *             Get a dynptr to local memory *data*.
> >   *
> > - *             *data* must be a ptr to a map value.
> > + *             *data* must be a ptr to valid local memory such as a map value, a uptr,
> > + *             a null-checked non-void pointer pass to a global subprogram, and allocated
> > + *             memory returned by a kfunc such as hid_bpf_get_data(),
> >   *             The maximum *size* supported is DYNPTR_MAX_SIZE.
> >   *             *flags* is currently unused.
> >   *     Return
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 22c4edc8695c..d22310d1642c 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -11307,7 +11307,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> >                 }
> >                 break;
> >         case BPF_FUNC_dynptr_from_mem:
> > -               if (regs[BPF_REG_1].type != PTR_TO_MAP_VALUE) {
> > +               if (regs[BPF_REG_1].type != PTR_TO_MAP_VALUE &&
> > +                   regs[BPF_REG_1].type != PTR_TO_MEM) {
> >                         verbose(env, "Unsupported reg type %s for bpf_dynptr_from_mem data\n",
> >                                 reg_type_str(env, regs[BPF_REG_1].type));
> >                         return -EACCES;
> > --
> > 2.47.1
> >

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index beac5cdf2d2c..2b1335fa1173 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5562,7 +5562,9 @@  union bpf_attr {
  *	Description
  *		Get a dynptr to local memory *data*.
  *
- *		*data* must be a ptr to a map value.
+ *		*data* must be a ptr to valid local memory such as a map value, a uptr,
+ *		a null-checked non-void pointer pass to a global subprogram, and allocated
+ *		memory returned by a kfunc such as hid_bpf_get_data(),
  *		The maximum *size* supported is DYNPTR_MAX_SIZE.
  *		*flags* is currently unused.
  *	Return
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 22c4edc8695c..d22310d1642c 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11307,7 +11307,8 @@  static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
 		}
 		break;
 	case BPF_FUNC_dynptr_from_mem:
-		if (regs[BPF_REG_1].type != PTR_TO_MAP_VALUE) {
+		if (regs[BPF_REG_1].type != PTR_TO_MAP_VALUE &&
+		    regs[BPF_REG_1].type != PTR_TO_MEM) {
 			verbose(env, "Unsupported reg type %s for bpf_dynptr_from_mem data\n",
 				reg_type_str(env, regs[BPF_REG_1].type));
 			return -EACCES;