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?
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
>
