On Wed, May 11, 2022 at 5:05 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
>
> On 5/10/22 12:42 AM, Joanne Koong wrote:
> [...]
> > @@ -6498,6 +6523,11 @@ struct bpf_timer {
> > __u64 :64;
> > } __attribute__((aligned(8)));
> >
> > +struct bpf_dynptr {
> > + __u64 :64;
> > + __u64 :64;
> > +} __attribute__((aligned(8)));
> > +
> > struct bpf_sysctl {
> > __u32 write; /* Sysctl is being read (= 0) or written (= 1).
> > * Allows 1,2,4-byte read, but no write.
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 8a2398ac14c2..a4272e9239ea 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1396,6 +1396,77 @@ const struct bpf_func_proto bpf_kptr_xchg_proto = {
> > .arg2_btf_id = BPF_PTR_POISON,
> > };
> >
> > +void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data, enum bpf_dynptr_type type,
> > + u32 offset, u32 size)
> > +{
> > + ptr->data = data;
> > + ptr->offset = offset;
> > + ptr->size = size;
> > + bpf_dynptr_set_type(ptr, type);
> > +}
> > +
> > +void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr)
> > +{
> > + memset(ptr, 0, sizeof(*ptr));
> > +}
> > +
> > +BPF_CALL_3(bpf_dynptr_alloc, u32, size, u64, flags, struct bpf_dynptr_kern *, ptr)
> > +{
> > + gfp_t gfp_flags = GFP_ATOMIC;
>
> nit: should also have __GFP_NOWARN
I will add this in to v5
>
> I presume mem accounting cannot be done on this one given there is no real "ownership"
> of this piece of mem?
I'm not too familiar with memory accounting, but I think the ownership
can get ambiguous given that the memory can be persisted in a map and
"owned" by different bpf programs (eg the one that frees it may not be
the same one that allocated it)
>
> Was planning to run some more local tests tomorrow, but from glance at selftest side
> I haven't seen sanity checks like these:
>
> bpf_dynptr_alloc(8, 0, &ptr);
> data = bpf_dynptr_data(&ptr, 0, 0);
> bpf_dynptr_put(&ptr);
> *(__u8 *)data = 23;
>
> How is this prevented? I think you do a ptr id check in the is_dynptr_ref_function
> check on the acquire function, but with above use, would our data pointer escape, or
> get invalidated via last put?
There's a subtest inside the dynptr_fail.c file called
"data_slice_use_after_put" that does:
bpf_dynptr_alloc(8, 0, &ptr);
data =bpf_dynptr_data(&ptr, 0, 8);
bpf_dynptr_put(&ptr);
val = *(__u8 *)data;
and checks that trying to dereference the data slice in that last line
fails the verifier (with error msg "invalid mem access 'scalar'")
In the verifier, the call to bpf_dynptr_put will invalidate any data
slices associated with the dyntpr. This happens in
unmark_stack_slots_dynptr() which calls release_reference() which
marks the data slice reg as an unknown scalar value. When you try to
then dereference the data slice, the verifier rejects it with an
"invalid mem access 'scalar'" message.
Thanks for your comments.
>
> Thanks,
> Daniel
