On Wed, Jan 11, 2023 at 5:08 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote: > > 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@xxxxxxxxx> wrote: > > > > > > Happy New Year! > > > > > > This is part 2 of https://lore.kernel.org/bpf/20221018135920.726360-1-memxor@xxxxxxxxx. > > > > > > Changelog: > > > ---------- > > > Old v1 -> v1 > > > Old v1: https://lore.kernel.org/bpf/20221018135920.726360-1-memxor@xxxxxxxxx > > > > > > * 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? So I did a first quick pass and just changed is_spilled_reg to +static bool is_stack_slot_special(const struct bpf_stack_state *stack) +{ + enum bpf_stack_slot_type type = stack->slot_type[BPF_REG_SIZE - 1]; + + switch (type) { + case STACK_SPILL: + case STACK_DYNPTR: + case STACK_ITER: + return true; + case STACK_INVALID: + case STACK_MISC: + case STACK_ZERO: + return false; + default: + WARN_ONCE(1, "unknown stack slot type %d\n", type); + return true; + } +} + Which resulted in one or two tests failing due to the wrong verifier message. I haven't spent much time debugging this and I still have a few TODOs in the code to double-check everything in regards to this change. Thanks for the above analysis, I'll come back to it when I get to work on my code again. > > > > 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 > > >
