On Thu, 2023-02-16 at 16:55 -0800, Andrii Nakryiko wrote:
> On Thu, Feb 16, 2023 at 10:36 AM Eduard Zingerman <eddyz87@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > .../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@xxxxxxxxxx>
>
> > +"
> > + ::: __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
> >
