[v1,bpf-next] bpf: Update kfunc __sz documentation

Commit Message

Joanne Koong Feb. 14, 2023, 4:33 a.m. UTC

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 <[e-mail]>
---
 Documentation/bpf/kfuncs.rst | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

Comments

Alexei Starovoitov Feb. 14, 2023, 8:57 p.m. UTC | #1

On Mon, Feb 13, 2023 at 8:35 PM Joanne Koong <[e-mail]> 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 <[e-mail]>
> ---
>  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/[e-mail]/

Eduard, is about to send those verifier patches.

In parallel we can relax __sz to accept uninit under allow_uninit_stack.

Joanne Koong Feb. 18, 2023, 1:24 a.m. UTC | #2

On Tue, Feb 14, 2023 at 12:57 PM Alexei Starovoitov
<[e-mail]> wrote:
>
> On Mon, Feb 13, 2023 at 8:35 PM Joanne Koong <[e-mail]> 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 <[e-mail]>
> > ---
> >  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/[e-mail]/
>
> 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

Patch

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));
+	...
+	}
 
 2.2.2 __k Annotation
 --------------------