diff mbox series

[bpf-next,2/2] selftests/bpf: Tests for uninitialized stack reads

Message ID 20230216183606.2483834-3-eddyz87@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series Allow reads from uninit stack | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 9 maintainers not CCed: linux-kselftest@vger.kernel.org john.fastabend@gmail.com sdf@google.com shuah@kernel.org jolsa@kernel.org song@kernel.org mykolal@fb.com haoluo@google.com kpsingh@kernel.org
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: Avoid line continuations in quoted strings WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ${{ matrix.test }} on ${{ matrix.arch }} with ${{ matrix.toolchain }}
bpf/vmtest-bpf-next-VM_Test-2 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 fail Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-4 fail Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-5 fail Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-6 fail Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 fail Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-8 success Logs for llvm-toolchain
bpf/vmtest-bpf-next-VM_Test-9 success Logs for set-matrix

Commit Message

Eduard Zingerman Feb. 16, 2023, 6:36 p.m. UTC 
Two testcases to make sure that stack reads from uninitialized
locations are accepted by verifier when executed in privileged mode:
- read from a fixed offset;
- read from a variable offset.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 .../selftests/bpf/prog_tests/uninit_stack.c   |  9 +++
 .../selftests/bpf/progs/uninit_stack.c        | 55 +++++++++++++++++++
 2 files changed, 64 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/uninit_stack.c
 create mode 100644 tools/testing/selftests/bpf/progs/uninit_stack.c

Comments

Andrii Nakryiko Feb. 17, 2023, 12:55 a.m. UTC | #1 
On Thu, Feb 16, 2023 at 10:36 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> Two testcases to make sure that stack reads from uninitialized
> locations are accepted by verifier when executed in privileged mode:
> - read from a fixed offset;
> - read from a variable offset.
>
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  .../selftests/bpf/prog_tests/uninit_stack.c   |  9 +++
>  .../selftests/bpf/progs/uninit_stack.c        | 55 +++++++++++++++++++
>  2 files changed, 64 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/uninit_stack.c
>  create mode 100644 tools/testing/selftests/bpf/progs/uninit_stack.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/uninit_stack.c b/tools/testing/selftests/bpf/prog_tests/uninit_stack.c
> new file mode 100644
> index 000000000000..e64c71948491
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/uninit_stack.c
> @@ -0,0 +1,9 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <test_progs.h>
> +#include "uninit_stack.skel.h"
> +
> +void test_uninit_stack(void)
> +{
> +       RUN_TESTS(uninit_stack);
> +}
> diff --git a/tools/testing/selftests/bpf/progs/uninit_stack.c b/tools/testing/selftests/bpf/progs/uninit_stack.c
> new file mode 100644
> index 000000000000..20ff6a22c906
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/uninit_stack.c
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include "bpf_misc.h"
> +
> +/* Read an uninitialized value from stack at a fixed offset */
> +SEC("socket")
> +__naked int read_uninit_stack_fixed_off(void *ctx)
> +{
> +       asm volatile ("                         \
> +               // force stack depth to be 128  \
> +               *(u64*)(r10 - 128) = r1;        \
> +               r1 = *(u8 *)(r10 - 8 );         \
> +               r1 = *(u8 *)(r10 - 11);         \
> +               r1 = *(u8 *)(r10 - 13);         \
> +               r1 = *(u8 *)(r10 - 15);         \
> +               r1 = *(u16*)(r10 - 16);         \
> +               r1 = *(u32*)(r10 - 32);         \
> +               r1 = *(u64*)(r10 - 64);         \
> +               // read from a spill of a wrong size, it is a separate  \
> +               // branch in check_stack_read_fixed_off()               \
> +               *(u32*)(r10 - 72) = r1;         \
> +               r1 = *(u64*)(r10 - 72);         \
> +               r0 = 0;                         \
> +               exit;                           \

would it be better to

r0 = *(u64*)(r10 - 72);
exit;

to make sure that in the future verifier doesn't smartly optimize out
unused reads?


Either way, looks good to me:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> +"
> +                     ::: __clobber_all);
> +}
> +
> +/* Read an uninitialized value from stack at a variable offset */
> +SEC("socket")
> +__naked int read_uninit_stack_var_off(void *ctx)
> +{
> +       asm volatile ("                         \
> +               call %[bpf_get_prandom_u32];    \
> +               // force stack depth to be 64   \
> +               *(u64*)(r10 - 64) = r0;         \
> +               r0 = -r0;                       \
> +               // give r0 a range [-31, -1]    \
> +               if r0 s<= -32 goto exit_%=;     \
> +               if r0 s>= 0 goto exit_%=;       \
> +               // access stack using r0        \
> +               r1 = r10;                       \
> +               r1 += r0;                       \
> +               r2 = *(u8*)(r1 + 0);            \
> +exit_%=:       r0 = 0;                         \
> +               exit;                           \
> +"
> +                     :
> +                     : __imm(bpf_get_prandom_u32)
> +                     : __clobber_all);
> +}
> +
> +char _license[] SEC("license") = "GPL";
> --
> 2.39.1
>
Eduard Zingerman Feb. 17, 2023, 1:25 p.m. UTC | #2 
On Thu, 2023-02-16 at 16:55 -0800, Andrii Nakryiko wrote:
> On Thu, Feb 16, 2023 at 10:36 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > Two testcases to make sure that stack reads from uninitialized
> > locations are accepted by verifier when executed in privileged mode:
> > - read from a fixed offset;
> > - read from a variable offset.
> > 
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> >  .../selftests/bpf/prog_tests/uninit_stack.c   |  9 +++
> >  .../selftests/bpf/progs/uninit_stack.c        | 55 +++++++++++++++++++
> >  2 files changed, 64 insertions(+)
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/uninit_stack.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/uninit_stack.c
> > 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/uninit_stack.c b/tools/testing/selftests/bpf/prog_tests/uninit_stack.c
> > new file mode 100644
> > index 000000000000..e64c71948491
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/prog_tests/uninit_stack.c
> > @@ -0,0 +1,9 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <test_progs.h>
> > +#include "uninit_stack.skel.h"
> > +
> > +void test_uninit_stack(void)
> > +{
> > +       RUN_TESTS(uninit_stack);
> > +}
> > diff --git a/tools/testing/selftests/bpf/progs/uninit_stack.c b/tools/testing/selftests/bpf/progs/uninit_stack.c
> > new file mode 100644
> > index 000000000000..20ff6a22c906
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/uninit_stack.c
> > @@ -0,0 +1,55 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/bpf.h>
> > +#include <bpf/bpf_helpers.h>
> > +#include "bpf_misc.h"
> > +
> > +/* Read an uninitialized value from stack at a fixed offset */
> > +SEC("socket")
> > +__naked int read_uninit_stack_fixed_off(void *ctx)
> > +{
> > +       asm volatile ("                         \
> > +               // force stack depth to be 128  \
> > +               *(u64*)(r10 - 128) = r1;        \
> > +               r1 = *(u8 *)(r10 - 8 );         \
> > +               r1 = *(u8 *)(r10 - 11);         \
> > +               r1 = *(u8 *)(r10 - 13);         \
> > +               r1 = *(u8 *)(r10 - 15);         \
> > +               r1 = *(u16*)(r10 - 16);         \
> > +               r1 = *(u32*)(r10 - 32);         \
> > +               r1 = *(u64*)(r10 - 64);         \
> > +               // read from a spill of a wrong size, it is a separate  \
> > +               // branch in check_stack_read_fixed_off()               \
> > +               *(u32*)(r10 - 72) = r1;         \
> > +               r1 = *(u64*)(r10 - 72);         \
> > +               r0 = 0;                         \
> > +               exit;                           \
> 
> would it be better to
> 
> r0 = *(u64*)(r10 - 72);
> exit;
> 
> to make sure that in the future verifier doesn't smartly optimize out
> unused reads?

Are there plans for such optimizations? If there are, many tests might
be in trouble. I thought that this is delegated to the C compiler.

For this particular case the rewrite might look as:

	asm volatile ("					\
		r0 = 0;					\
		/* force stack depth to be 128 */	\
		*(u64*)(r10 - 128) = r1;		\
		r1 = *(u8 *)(r10 - 8 );			\
		r0 += r1;				\
		r1 = *(u8 *)(r10 - 11);			\
		r0 += r1;				\
		r1 = *(u8 *)(r10 - 13);			\
		r0 += r1;				\
		r1 = *(u8 *)(r10 - 15);			\
		r0 += r1;				\
		r1 = *(u16*)(r10 - 16);			\
		r0 += r1;				\
		r1 = *(u32*)(r10 - 32);			\
		r0 += r1;				\
		r1 = *(u64*)(r10 - 64);			\
		r0 += r1;				\
		/* read from a spill of a wrong size, it is a separate	\
		 * branch in check_stack_read_fixed_off()		\
		 */					\
		*(u32*)(r10 - 72) = r1;			\
		r1 = *(u64*)(r10 - 72);			\
		r0 += r1;				\
		exit;					\
"
		      ::: __clobber_all);
              
It works but is kinda ugly.

 ---

Orthogonal to the above issue, I found that use of the '//' comments
in the asm code w/o newlines is invalid, as it makes rest of the
string a comment. I changed '\n\' line endings to '\' just before
sending the patch and did not verify the change.
=> The patch-set would have to be resent.

> 
> 
> Either way, looks good to me:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> > +"
> > +                     ::: __clobber_all);
> > +}
> > +
> > +/* Read an uninitialized value from stack at a variable offset */
> > +SEC("socket")
> > +__naked int read_uninit_stack_var_off(void *ctx)
> > +{
> > +       asm volatile ("                         \
> > +               call %[bpf_get_prandom_u32];    \
> > +               // force stack depth to be 64   \
> > +               *(u64*)(r10 - 64) = r0;         \
> > +               r0 = -r0;                       \
> > +               // give r0 a range [-31, -1]    \
> > +               if r0 s<= -32 goto exit_%=;     \
> > +               if r0 s>= 0 goto exit_%=;       \
> > +               // access stack using r0        \
> > +               r1 = r10;                       \
> > +               r1 += r0;                       \
> > +               r2 = *(u8*)(r1 + 0);            \
> > +exit_%=:       r0 = 0;                         \
> > +               exit;                           \
> > +"
> > +                     :
> > +                     : __imm(bpf_get_prandom_u32)
> > +                     : __clobber_all);
> > +}
> > +
> > +char _license[] SEC("license") = "GPL";
> > --
> > 2.39.1
> >
Andrii Nakryiko Feb. 17, 2023, 10 p.m. UTC | #3 
On Fri, Feb 17, 2023 at 5:25 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2023-02-16 at 16:55 -0800, Andrii Nakryiko wrote:
> > On Thu, Feb 16, 2023 at 10:36 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > >
> > > Two testcases to make sure that stack reads from uninitialized
> > > locations are accepted by verifier when executed in privileged mode:
> > > - read from a fixed offset;
> > > - read from a variable offset.
> > >
> > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > > ---
> > >  .../selftests/bpf/prog_tests/uninit_stack.c   |  9 +++
> > >  .../selftests/bpf/progs/uninit_stack.c        | 55 +++++++++++++++++++
> > >  2 files changed, 64 insertions(+)
> > >  create mode 100644 tools/testing/selftests/bpf/prog_tests/uninit_stack.c
> > >  create mode 100644 tools/testing/selftests/bpf/progs/uninit_stack.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/uninit_stack.c b/tools/testing/selftests/bpf/prog_tests/uninit_stack.c
> > > new file mode 100644
> > > index 000000000000..e64c71948491
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/prog_tests/uninit_stack.c
> > > @@ -0,0 +1,9 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <test_progs.h>
> > > +#include "uninit_stack.skel.h"
> > > +
> > > +void test_uninit_stack(void)
> > > +{
> > > +       RUN_TESTS(uninit_stack);
> > > +}
> > > diff --git a/tools/testing/selftests/bpf/progs/uninit_stack.c b/tools/testing/selftests/bpf/progs/uninit_stack.c
> > > new file mode 100644
> > > index 000000000000..20ff6a22c906
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/bpf/progs/uninit_stack.c
> > > @@ -0,0 +1,55 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +
> > > +#include <linux/bpf.h>
> > > +#include <bpf/bpf_helpers.h>
> > > +#include "bpf_misc.h"
> > > +
> > > +/* Read an uninitialized value from stack at a fixed offset */
> > > +SEC("socket")
> > > +__naked int read_uninit_stack_fixed_off(void *ctx)
> > > +{
> > > +       asm volatile ("                         \
> > > +               // force stack depth to be 128  \
> > > +               *(u64*)(r10 - 128) = r1;        \
> > > +               r1 = *(u8 *)(r10 - 8 );         \
> > > +               r1 = *(u8 *)(r10 - 11);         \
> > > +               r1 = *(u8 *)(r10 - 13);         \
> > > +               r1 = *(u8 *)(r10 - 15);         \
> > > +               r1 = *(u16*)(r10 - 16);         \
> > > +               r1 = *(u32*)(r10 - 32);         \
> > > +               r1 = *(u64*)(r10 - 64);         \
> > > +               // read from a spill of a wrong size, it is a separate  \
> > > +               // branch in check_stack_read_fixed_off()               \
> > > +               *(u32*)(r10 - 72) = r1;         \
> > > +               r1 = *(u64*)(r10 - 72);         \
> > > +               r0 = 0;                         \
> > > +               exit;                           \
> >
> > would it be better to
> >
> > r0 = *(u64*)(r10 - 72);
> > exit;
> >
> > to make sure that in the future verifier doesn't smartly optimize out
> > unused reads?
>
> Are there plans for such optimizations? If there are, many tests might
> be in trouble. I thought that this is delegated to the C compiler.

I'm not aware of them, just hypothetical concern

>
> For this particular case the rewrite might look as:
>
>         asm volatile ("                                 \
>                 r0 = 0;                                 \
>                 /* force stack depth to be 128 */       \
>                 *(u64*)(r10 - 128) = r1;                \
>                 r1 = *(u8 *)(r10 - 8 );                 \
>                 r0 += r1;                               \
>                 r1 = *(u8 *)(r10 - 11);                 \
>                 r0 += r1;                               \
>                 r1 = *(u8 *)(r10 - 13);                 \
>                 r0 += r1;                               \
>                 r1 = *(u8 *)(r10 - 15);                 \
>                 r0 += r1;                               \
>                 r1 = *(u16*)(r10 - 16);                 \
>                 r0 += r1;                               \
>                 r1 = *(u32*)(r10 - 32);                 \
>                 r0 += r1;                               \
>                 r1 = *(u64*)(r10 - 64);                 \
>                 r0 += r1;                               \
>                 /* read from a spill of a wrong size, it is a separate  \
>                  * branch in check_stack_read_fixed_off()               \
>                  */                                     \
>                 *(u32*)(r10 - 72) = r1;                 \
>                 r1 = *(u64*)(r10 - 72);                 \
>                 r0 += r1;                               \
>                 exit;                                   \
> "
>                       ::: __clobber_all);
>
> It works but is kinda ugly.

nah, no need

>
>  ---
>
> Orthogonal to the above issue, I found that use of the '//' comments
> in the asm code w/o newlines is invalid, as it makes rest of the
> string a comment. I changed '\n\' line endings to '\' just before
> sending the patch and did not verify the change.
> => The patch-set would have to be resent.

I was wondering about that, but assumed you tested it ;) so yeah,
please fix and resend. (in that sense having each line separately
quoted allows much easier commenting, but we've decided on this style,
so let's stick to it

>
> >
> >
> > Either way, looks good to me:
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >
> > > +"
> > > +                     ::: __clobber_all);
> > > +}
> > > +
> > > +/* Read an uninitialized value from stack at a variable offset */
> > > +SEC("socket")
> > > +__naked int read_uninit_stack_var_off(void *ctx)
> > > +{
> > > +       asm volatile ("                         \
> > > +               call %[bpf_get_prandom_u32];    \
> > > +               // force stack depth to be 64   \
> > > +               *(u64*)(r10 - 64) = r0;         \
> > > +               r0 = -r0;                       \
> > > +               // give r0 a range [-31, -1]    \
> > > +               if r0 s<= -32 goto exit_%=;     \
> > > +               if r0 s>= 0 goto exit_%=;       \
> > > +               // access stack using r0        \
> > > +               r1 = r10;                       \
> > > +               r1 += r0;                       \
> > > +               r2 = *(u8*)(r1 + 0);            \
> > > +exit_%=:       r0 = 0;                         \
> > > +               exit;                           \
> > > +"
> > > +                     :
> > > +                     : __imm(bpf_get_prandom_u32)
> > > +                     : __clobber_all);
> > > +}
> > > +
> > > +char _license[] SEC("license") = "GPL";
> > > --
> > > 2.39.1
> > >
>
Eduard Zingerman Feb. 17, 2023, 10:06 p.m. UTC | #4 
On Fri, 2023-02-17 at 14:00 -0800, Andrii Nakryiko wrote:
[...]
> > Orthogonal to the above issue, I found that use of the '//' comments
> > in the asm code w/o newlines is invalid, as it makes rest of the
> > string a comment. I changed '\n\' line endings to '\' just before
> > sending the patch and did not verify the change.
> > => The patch-set would have to be resent.
> 
> I was wondering about that, but assumed you tested it ;) so yeah,
> please fix and resend. (in that sense having each line separately
> quoted allows much easier commenting, but we've decided on this style,
> so let's stick to it

And syntax highlighting for strings vs comments :'(

Thank you for the replies, I'll update the patch #1 and resend.

[...]
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/uninit_stack.c b/tools/testing/selftests/bpf/prog_tests/uninit_stack.c
new file mode 100644
index 000000000000..e64c71948491
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/uninit_stack.c
@@ -0,0 +1,9 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <test_progs.h>
+#include "uninit_stack.skel.h"
+
+void test_uninit_stack(void)
+{
+	RUN_TESTS(uninit_stack);
+}
diff --git a/tools/testing/selftests/bpf/progs/uninit_stack.c b/tools/testing/selftests/bpf/progs/uninit_stack.c
new file mode 100644
index 000000000000..20ff6a22c906
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/uninit_stack.c
@@ -0,0 +1,55 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include "bpf_misc.h"
+
+/* Read an uninitialized value from stack at a fixed offset */
+SEC("socket")
+__naked int read_uninit_stack_fixed_off(void *ctx)
+{
+	asm volatile ("				\
+		// force stack depth to be 128	\
+		*(u64*)(r10 - 128) = r1;	\
+		r1 = *(u8 *)(r10 - 8 );		\
+		r1 = *(u8 *)(r10 - 11);		\
+		r1 = *(u8 *)(r10 - 13);		\
+		r1 = *(u8 *)(r10 - 15);		\
+		r1 = *(u16*)(r10 - 16);		\
+		r1 = *(u32*)(r10 - 32);		\
+		r1 = *(u64*)(r10 - 64);		\
+		// read from a spill of a wrong size, it is a separate	\
+		// branch in check_stack_read_fixed_off()		\
+		*(u32*)(r10 - 72) = r1;		\
+		r1 = *(u64*)(r10 - 72);		\
+		r0 = 0;				\
+		exit;				\
+"
+		      ::: __clobber_all);
+}
+
+/* Read an uninitialized value from stack at a variable offset */
+SEC("socket")
+__naked int read_uninit_stack_var_off(void *ctx)
+{
+	asm volatile ("				\
+		call %[bpf_get_prandom_u32];	\
+		// force stack depth to be 64	\
+		*(u64*)(r10 - 64) = r0;		\
+		r0 = -r0;			\
+		// give r0 a range [-31, -1]	\
+		if r0 s<= -32 goto exit_%=;	\
+		if r0 s>= 0 goto exit_%=;	\
+		// access stack using r0	\
+		r1 = r10;			\
+		r1 += r0;			\
+		r2 = *(u8*)(r1 + 0);		\
+exit_%=:	r0 = 0;				\
+		exit;				\
+"
+		      :
+		      : __imm(bpf_get_prandom_u32)
+		      : __clobber_all);
+}
+
+char _license[] SEC("license") = "GPL";