On Fri, Feb 17, 2023 at 5:25 AM Eduard Zingerman <eddyz87@xxxxxxxxx> 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@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.
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@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
> > >
>
