| Message ID | 20230101083403.332783-1-memxor@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Dynptr fixes | expand |
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? > 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 >
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 > >
On Wed, Jan 11, 2023 at 5:08 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> 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@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? 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 > > >