On Thu, 29 Sept 2022 at 01:04, Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Wed, Sep 7, 2022 at 5:07 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
> >
> > Add a new helper function, bpf_dynptr_iterator:
> >
> > long bpf_dynptr_iterator(struct bpf_dynptr *ptr, void *callback_fn,
> > void *callback_ctx, u64 flags)
> >
> > where callback_fn is defined as:
> >
> > long (*callback_fn)(struct bpf_dynptr *ptr, void *ctx)
> >
> > and callback_fn returns the number of bytes to advance the
> > dynptr by (or an error code in the case of error). The iteration
> > will stop if the callback_fn returns 0 or an error or tries to
> > advance by more bytes than available.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> > include/uapi/linux/bpf.h | 20 ++++++++++++++
> > kernel/bpf/helpers.c | 48 +++++++++++++++++++++++++++++++---
> > kernel/bpf/verifier.c | 27 +++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 20 ++++++++++++++
> > 4 files changed, 111 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 16973fa4d073..ff78a94c262a 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -5531,6 +5531,25 @@ union bpf_attr {
> > * losing access to the original view of the dynptr.
> > * Return
> > * 0 on success, -EINVAL if the dynptr to clone is invalid.
> > + *
> > + * long bpf_dynptr_iterator(struct bpf_dynptr *ptr, void *callback_fn, void *callback_ctx, u64 flags)
> > + * Description
> > + * Iterate through the dynptr data, calling **callback_fn** on each
> > + * iteration with **callback_ctx** as the context parameter.
> > + * The **callback_fn** should be a static function and
> > + * the **callback_ctx** should be a pointer to the stack.
> > + * Currently **flags** is unused and must be 0.
> > + *
> > + * long (\*callback_fn)(struct bpf_dynptr \*ptr, void \*ctx);
> > + *
> > + * where **callback_fn** returns the number of bytes to advance
> > + * the dynptr by or an error. The iteration will stop if **callback_fn**
> > + * returns 0 or an error or tries to advance by more bytes than the
> > + * size of the dynptr.
> > + * Return
> > + * 0 on success, -EINVAL if the dynptr is invalid or **flags** is not 0,
> > + * -ERANGE if attempting to iterate more bytes than available, or other
> > + * negative error if **callback_fn** returns an error.
> > */
> > #define __BPF_FUNC_MAPPER(FN) \
> > FN(unspec), \
> > @@ -5752,6 +5771,7 @@ union bpf_attr {
> > FN(dynptr_get_size), \
> > FN(dynptr_get_offset), \
> > FN(dynptr_clone), \
> > + FN(dynptr_iterator), \
> > /* */
> >
> > /* 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 667f1e213a61..519b3da06d49 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1653,13 +1653,11 @@ static const struct bpf_func_proto bpf_dynptr_data_proto = {
> > .arg3_type = ARG_CONST_ALLOC_SIZE_OR_ZERO,
> > };
> >
> > -BPF_CALL_2(bpf_dynptr_advance, struct bpf_dynptr_kern *, ptr, u32, len)
> > +/* *ptr* should always be a valid dynptr */
> > +static int __bpf_dynptr_advance(struct bpf_dynptr_kern *ptr, u32 len)
> > {
> > u32 size;
> >
> > - if (!ptr->data)
> > - return -EINVAL;
> > -
> > size = __bpf_dynptr_get_size(ptr);
> >
> > if (len > size)
> > @@ -1672,6 +1670,14 @@ BPF_CALL_2(bpf_dynptr_advance, struct bpf_dynptr_kern *, ptr, u32, len)
> > return 0;
> > }
> >
> > +BPF_CALL_2(bpf_dynptr_advance, struct bpf_dynptr_kern *, ptr, u32, len)
> > +{
> > + if (!ptr->data)
> > + return -EINVAL;
> > +
> > + return __bpf_dynptr_advance(ptr, len);
> > +}
> > +
> > static const struct bpf_func_proto bpf_dynptr_advance_proto = {
> > .func = bpf_dynptr_advance,
> > .gpl_only = false,
> > @@ -1783,6 +1789,38 @@ static const struct bpf_func_proto bpf_dynptr_clone_proto = {
> > .arg2_type = ARG_PTR_TO_DYNPTR | MEM_UNINIT,
> > };
> >
> > +BPF_CALL_4(bpf_dynptr_iterator, struct bpf_dynptr_kern *, ptr, void *, callback_fn,
> > + void *, callback_ctx, u64, flags)
> > +{
> > + bpf_callback_t callback = (bpf_callback_t)callback_fn;
> > + int nr_bytes, err;
> > +
> > + if (!ptr->data || flags)
> > + return -EINVAL;
> > +
> > + while (ptr->size > 0) {
> > + nr_bytes = callback((u64)(long)ptr, (u64)(long)callback_ctx, 0, 0, 0);
> > + if (nr_bytes <= 0)
> > + return nr_bytes;
>
> callback is defined as returning long but here you are silently
> truncating it to int. Let's either stick to longs or to ints.
>
> > +
> > + err = __bpf_dynptr_advance(ptr, nr_bytes);
>
> as Kumar pointed out, you can't modify ptr in place, you have to
> create a local copy and bpf_dynptr_clone() it (so that for MALLOC
> you'll bump refcnt). Then advance and pass it to callback. David has
> such local dynptr use case in bpf_user_ringbuf_drain() helper.
>
My solution was a bit overcomplicated because just creating a local
copy doesn't fix this completely, there's still the hole of writing
through arg#0 (which now does not reflect runtime state, as writes at
runtime go to clone while verifier thinks you touched stack slots),
and still allows constructing the infinite loop (because well, you can
overwrite dynptr through it). The 'side channel' of writing to dynptr
slots through callback_ctx is still there as well.
Maybe the infinite loop _can_ be avoided if you clone inside each
iteration, that part isn't very clear.
My reasoning was that when you iterate, the container is always
immutable (to prevent iterator invalidation) while mutability of
elements remains unaffected from that. Setting it in spilled_ptr
covers all bases (PTR_TO_STACK arg#0, access to it through
callback_ctx, etc.).
But I totally agree with you that we should be working on a local copy
inside the helper and leave the original dynptr untouched.
I think then the first arg should not be PTR_TO_STACK but some other
pointer type. Maybe it should be its own register type, and
spilled_ptr should reflect the same register, which allows the dynptr
state that we track to be copied into the callback arg#0 easily.
For the case of writes to the original dynptr that is already broken
right now, we can't track the state of the stack across iterations
correctly anyway so I think that has to be left as is until your N
callbacks rework happens.
wdyt?
