Re: [PATCH bpf-next v1 0/8] Dynptr fixes

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Andrii Nakryiko <andrii@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Martin KaFai Lau <martin.lau@kernel.org>,
	Joanne Koong <joannelkoong@gmail.com>,
	David Vernet <void@manifault.com>,
	Eduard Zingerman <eddyz87@gmail.com>
Subject: Re: [PATCH bpf-next v1 0/8] Dynptr fixes
Date: Thu, 12 Jan 2023 06:38:21 +0530	[thread overview]
Message-ID: <20230112010821.bous4eprkyaikkzu@apollo> (raw)
In-Reply-To: <CAEf4BzaZuCWq5KrO-NPZjAya1etM4_zCFxWgva4zVDYaWJ89iw@mail.gmail.com>

On Thu, Jan 05, 2023 at 04:21:27AM IST, Andrii Nakryiko wrote:
> On Sun, Jan 1, 2023 at 12:34 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > Happy New Year!
> >
> > This is part 2 of https://lore.kernel.org/bpf/20221018135920.726360-1-memxor@gmail.com.
> >
> > Changelog:
> > ----------
> > Old v1 -> v1
> > Old v1: https://lore.kernel.org/bpf/20221018135920.726360-1-memxor@gmail.com
> >
> >  * Allow overwriting dynptr stack slots from dynptr init helpers
> >  * Fix a bug in alignment check where reg->var_off.value was still not included
> >  * Address other minor nits
> >
> > Kumar Kartikeya Dwivedi (8):
> >   bpf: Fix state pruning for STACK_DYNPTR stack slots
> >   bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR
> >   bpf: Fix partial dynptr stack slot reads/writes
> >   bpf: Allow reinitializing unreferenced dynptr stack slots
> >   selftests/bpf: Add dynptr pruning tests
> >   selftests/bpf: Add dynptr var_off tests
> >   selftests/bpf: Add dynptr partial slot overwrite tests
> >   selftests/bpf: Add dynptr helper tests
> >
>
> Hey Kumar, thanks for fixes! Left few comments, but I was also
> wondering if you thought about current is_spilled_reg() usage in the
> code? It makes an assumption that stack slots can be either a scalar
> (MISC/ZERO/INVALID) or STACK_SPILL. With STACK_DYNPTR it's not the
> case anymore, so it feels like we need to audit all the places where
> we assume stack spill and see if anything should be fixed. Was just
> wondering if you already looked at this?
>

I did look at its usage (once again), here are some comments:

For check_stack_read_fixed_off, the else branch for !is_spilled_reg treats
anything apart from STACK_MISC and STACK_ZERO as an error, so it would include
STACK_INVALID, STACK_DYNPTR, and your upcoming STACK_ITER.
For check_stack_read_var_off and its underlying check_stack_range_initialized,
situation is the same, anything apart from STACK_MISC, STACK_ZERO and
STACK_SPILL becomes an error.

Coming to check_stack_write_fixed_off and check_stack_write_var_off, they will
no longer see STACK_DYNPTR, as we destroy all dynptr for the stack slots being
written to, so it falls back to the handling for the case of STACK_INVALID.
With your upcoming STACK_ITER I assume dynptr/iter/list_head all such objects on
the stack will follow consistent lifetime rules so stray writes should lead to
their destruction in verifier state.

The rest of the cases to me seem to be about checking for spilled reg to be a
SCALAR_VALUE, and one case in stacksafe which looks ok as well.

Are there any particular cases that you are worried about?

> >  kernel/bpf/verifier.c                         | 243 ++++++++++++++++--
> >  .../bpf/prog_tests/kfunc_dynptr_param.c       |   2 +-
> >  .../testing/selftests/bpf/progs/dynptr_fail.c |  68 ++++-
> >  tools/testing/selftests/bpf/verifier/dynptr.c | 182 +++++++++++++
> >  4 files changed, 464 insertions(+), 31 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/verifier/dynptr.c
> >
> >
> > base-commit: bb5747cfbc4b7fe29621ca6cd4a695d2723bf2e8
> > --
> > 2.39.0
> >