On Tue, Feb 14, 2023 at 12:57 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, Feb 13, 2023 at 8:35 PM Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
> >
> > A bpf program calling a kfunc with a __sz-annotated arg must explicitly
> > initialize the stack themselves if the pointer to the memory region is
> > a pointer to the stack. This is because in the verifier, we do not
> > explicitly initialize the stack space for reg type PTR_TO_STACK
> > kfunc args. Thus, the verifier will reject the program with:
> >
> > invalid indirect read from stack
> > arg#0 arg#1 memory, len pair leads to invalid memory access
> >
> > Alternatively, the verifier could support initializing the stack
> > space on behalf of the program for KF_ARG_PTR_TO_MEM_SIZE args,
> > but this has some drawbacks. For example this would not allow the
> > verifier to reject a program for passing in an uninitialized
> > PTR_TO_STACK for an arg that should have valid data. Another example is
> > that since there's no current way in a kfunc to differentiate between
> > whether the arg should be treated as uninitialized or not, additional
> > check_mem_access calls would need to be called even on PTR_TO_STACKs
> > that have been initialized, which is inefficient. Please note
> > that non-kfuncs don't have this problem because of the MEM_UNINIT tag;
> > only if the arg is tagged as MEM_UNINIT, then do we call
> > check_mem_access byte-by-byte for the size of the buffer.
> >
> > Signed-off-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > ---
> > Documentation/bpf/kfuncs.rst | 35 +++++++++++++++++++++++++++++++----
> > 1 file changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/bpf/kfuncs.rst b/Documentation/bpf/kfuncs.rst
> > index ca96ef3f6896..97497a7879d6 100644
> > --- a/Documentation/bpf/kfuncs.rst
> > +++ b/Documentation/bpf/kfuncs.rst
> > @@ -71,10 +71,37 @@ An example is given below::
> > ...
> > }
> >
> > -Here, the verifier will treat first argument as a PTR_TO_MEM, and second
> > -argument as its size. By default, without __sz annotation, the size of the type
> > -of the pointer is used. Without __sz annotation, a kfunc cannot accept a void
> > -pointer.
> > +Here, the verifier will treat first argument (KF_ARG_PTR_TO_MEM_SIZE) as a
> > +pointer to the memory region and second argument as its size. By default,
> > +without __sz annotation, the size of the type of the pointer is used. Without
> > +__sz annotation, a kfunc cannot accept a void pointer.
> > +
> > +Please note that if the memory is on the stack, the stack space must be
> > +explicitly initialized by the program. For example:
> > +
> > +.. code-block:: c
> > +
> > + SEC("tc")
> > + int prog(struct __sk_buff *skb)
> > + {
> > + char buf[8];
> > +
> > + bpf_memzero(buf, sizeof(buf));
> > + ...
> > + }
> > +
> > +should be
> > +
> > +.. code-block:: c
> > +
> > + SEC("tc")
> > + int prog(struct __sk_buff *skb)
> > + {
> > + char buf[8] = {};
> > +
> > + bpf_memzero(buf, sizeof(buf));
>
> Actually we might go the other way.
> Instead of asking users to explicitly init things
> we will allow uninit memory.
> See this discussion:
> https://lore.kernel.org/bpf/082fd8451321a832f334882a1872b5cee240d811.camel@xxxxxxxxx/
>
> Eduard, is about to send those verifier patches.
>
> In parallel we can relax __sz to accept uninit under allow_uninit_stack.
in that case, for kfuncs it needs to grow the stack state if the
buffer + __sz is beyond the current allocated stack, or else
check_stack_range_initialized() will automatically fail if the user
tries to pass in an uninitialized buffer. i have a local patch for
this in my tree, I'll tidy it up and submit it next week
