On Thu, Mar 20, 2025 at 3:45 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Thu, Mar 20, 2025 at 2:41 PM Amery Hung <ameryhung@xxxxxxxxx> 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 <ameryhung@xxxxxxxxx>
> > ---
> > 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
> >
