| Message ID | 20221018135920.726360-7-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Fixes for dynptr | expand |
On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > Currently, the dynptr function is not checking the variable offset part > of PTR_TO_STACK that it needs to check. The fixed offset is considered > when computing the stack pointer index, but if the variable offset was > not a constant (such that it could not be accumulated in reg->off), we > will end up a discrepency where runtime pointer does not point to the > actual stack slot we mark as STACK_DYNPTR. > > It is impossible to precisely track dynptr state when variable offset is > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > simply reject the case where reg->var_off is not constant. Then, > consider both reg->off and reg->var_off.value when computing the stack > pointer index. > > A new helper dynptr_get_spi is introduced to hide over these details > since the dynptr needs to be located in multiple places outside the > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > need to enforce these checks in all places. > > Note that it is disallowed for unprivileged users to have a non-constant > var_off, so this problem should only be possible to trigger from > programs having CAP_PERFMON. However, its effects can vary. > > Without the fix, it is possible to replace the contents of the dynptr > arbitrarily by making verifier mark different stack slots than actual > location and then doing writes to the actual stack address of dynptr at > runtime. > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > --- > kernel/bpf/verifier.c | 80 +++++++++++++++---- > .../testing/selftests/bpf/prog_tests/dynptr.c | 6 +- > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > 3 files changed, 67 insertions(+), 21 deletions(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 8f667180f70f..0fd73f96c5e2 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -610,11 +610,34 @@ static void print_liveness(struct bpf_verifier_env *env, > verbose(env, "D"); > } > > -static int get_spi(s32 off) > +static int __get_spi(s32 off) > { > return (-off - 1) / BPF_REG_SIZE; > } > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > +{ > + int spi; > + > + if (reg->off % BPF_REG_SIZE) { > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > + return -EINVAL; > + } I think this cannot happen. > + if (!tnum_is_const(reg->var_off)) { > + verbose(env, "dynptr has to be at the constant offset\n"); > + return -EINVAL; > + } This part can. > + spi = __get_spi(reg->off + reg->var_off.value); > + if (spi < 1) { > + verbose(env, "cannot pass in dynptr at an offset=%d\n", > + (int)(reg->off + reg->var_off.value)); > + return -EINVAL; > + } > + return spi; > +} This one is a more conservative (read: redundant) check. The is_spi_bounds_valid() is doing it better. How about keeping get_spi(reg) as error free and use it directly in places where it cannot fail without defensive WARN_ON_ONCE. int get_spi(reg) { return (-reg->off - reg->var_off.value - 1) / BPF_REG_SIZE; } While moving tnum_is_const() check into is_spi_bounds_valid() ? Like is_spi_bounds_valid(state, reg, spi) ? We should probably remove BPF_DYNPTR_NR_SLOTS since there are so many other places where dynptr is assumed to be 16-bytes. That macro doesn't help at all. It only causes confusion. I guess we can replace is_spi_bounds_valid() with a differnet helper that checks and computes spi. Like get_spi_and_check(state, reg, &spi) and use it in places where we have get_spi + is_spi_bounds_valid while using unchecked get_spi where it cannot fail? If we only have get_spi_and_check() we'd have to add WARN_ON_ONCE in a few places and that bothers me... due to defensive programming... If code is so complex that we cannot think it through we have to refactor it. Sprinkling WARN_ON_ONCE (just to be sure) doesn't inspire confidence. > + > static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots) > { > int allocated_slots = state->allocated_stack / BPF_REG_SIZE; > @@ -725,7 +748,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ > enum bpf_dynptr_type type; > int spi, i, id; > > - spi = get_spi(reg->off); > + spi = dynptr_get_spi(env, reg); > + if (spi < 0) > + return spi; > > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) > return -EINVAL;
On Thu, Oct 20, 2022 at 12:22:56AM IST, Alexei Starovoitov wrote: > On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > Currently, the dynptr function is not checking the variable offset part > > of PTR_TO_STACK that it needs to check. The fixed offset is considered > > when computing the stack pointer index, but if the variable offset was > > not a constant (such that it could not be accumulated in reg->off), we > > will end up a discrepency where runtime pointer does not point to the > > actual stack slot we mark as STACK_DYNPTR. > > > > It is impossible to precisely track dynptr state when variable offset is > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > > simply reject the case where reg->var_off is not constant. Then, > > consider both reg->off and reg->var_off.value when computing the stack > > pointer index. > > > > A new helper dynptr_get_spi is introduced to hide over these details > > since the dynptr needs to be located in multiple places outside the > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > > need to enforce these checks in all places. > > > > Note that it is disallowed for unprivileged users to have a non-constant > > var_off, so this problem should only be possible to trigger from > > programs having CAP_PERFMON. However, its effects can vary. > > > > Without the fix, it is possible to replace the contents of the dynptr > > arbitrarily by making verifier mark different stack slots than actual > > location and then doing writes to the actual stack address of dynptr at > > runtime. > > > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > --- > > kernel/bpf/verifier.c | 80 +++++++++++++++---- > > .../testing/selftests/bpf/prog_tests/dynptr.c | 6 +- > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > > 3 files changed, 67 insertions(+), 21 deletions(-) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index 8f667180f70f..0fd73f96c5e2 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -610,11 +610,34 @@ static void print_liveness(struct bpf_verifier_env *env, > > verbose(env, "D"); > > } > > > > -static int get_spi(s32 off) > > +static int __get_spi(s32 off) > > { > > return (-off - 1) / BPF_REG_SIZE; > > } > > > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > +{ > > + int spi; > > + > > + if (reg->off % BPF_REG_SIZE) { > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > + return -EINVAL; > > + } > > I think this cannot happen. > There are existing selftests that trigger this. Or do you mean it cannot happen anymore? If so, why? > > + if (!tnum_is_const(reg->var_off)) { > > + verbose(env, "dynptr has to be at the constant offset\n"); > > + return -EINVAL; > > + } > > This part can. > > > + spi = __get_spi(reg->off + reg->var_off.value); > > + if (spi < 1) { > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", > > + (int)(reg->off + reg->var_off.value)); > > + return -EINVAL; > > + } > > + return spi; > > +} > > This one is a more conservative (read: redundant) check. > The is_spi_bounds_valid() is doing it better. The problem is, is_spi_bounds_valid returning an error is not always a problem. See how in is_dynptr_reg_valid_uninit we just return true on invalid bounds, then later simulate two 8-byte accesses for uninit_dynptr_regno and rely on it to grow the stack depth and do MAX_BPF_STACK check. > How about keeping get_spi(reg) as error free and use it > directly in places where it cannot fail without > defensive WARN_ON_ONCE. > int get_spi(reg) > { return (-reg->off - reg->var_off.value - 1) / BPF_REG_SIZE; } > > While moving tnum_is_const() check into is_spi_bounds_valid() ? > > Like is_spi_bounds_valid(state, reg, spi) ? > > We should probably remove BPF_DYNPTR_NR_SLOTS since > there are so many other places where dynptr is assumed > to be 16-bytes. That macro doesn't help at all. > It only causes confusion. > > I guess we can replace is_spi_bounds_valid() with a differnet > helper that checks and computes spi. > Like get_spi_and_check(state, reg, &spi) > and use it in places where we have get_spi + is_spi_bounds_valid > while using unchecked get_spi where it cannot fail? > > If we only have get_spi_and_check() we'd have to add > WARN_ON_ONCE in a few places and that bothers me... > due to defensive programming... > If code is so complex that we cannot think it through > we have to refactor it. Sprinkling WARN_ON_ONCE (just to be sure) > doesn't inspire confidence. > I will think about this and reply later today.
On Wed, Oct 19, 2022 at 6:04 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Thu, Oct 20, 2022 at 12:22:56AM IST, Alexei Starovoitov wrote: > > On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > > > > Currently, the dynptr function is not checking the variable offset part > > > of PTR_TO_STACK that it needs to check. The fixed offset is considered > > > when computing the stack pointer index, but if the variable offset was > > > not a constant (such that it could not be accumulated in reg->off), we > > > will end up a discrepency where runtime pointer does not point to the > > > actual stack slot we mark as STACK_DYNPTR. > > > > > > It is impossible to precisely track dynptr state when variable offset is > > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > > > simply reject the case where reg->var_off is not constant. Then, > > > consider both reg->off and reg->var_off.value when computing the stack > > > pointer index. > > > > > > A new helper dynptr_get_spi is introduced to hide over these details > > > since the dynptr needs to be located in multiple places outside the > > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > > > need to enforce these checks in all places. > > > > > > Note that it is disallowed for unprivileged users to have a non-constant > > > var_off, so this problem should only be possible to trigger from > > > programs having CAP_PERFMON. However, its effects can vary. > > > > > > Without the fix, it is possible to replace the contents of the dynptr > > > arbitrarily by making verifier mark different stack slots than actual > > > location and then doing writes to the actual stack address of dynptr at > > > runtime. > > > > > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > --- > > > kernel/bpf/verifier.c | 80 +++++++++++++++---- > > > .../testing/selftests/bpf/prog_tests/dynptr.c | 6 +- > > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > > > 3 files changed, 67 insertions(+), 21 deletions(-) > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index 8f667180f70f..0fd73f96c5e2 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -610,11 +610,34 @@ static void print_liveness(struct bpf_verifier_env *env, > > > verbose(env, "D"); > > > } > > > > > > -static int get_spi(s32 off) > > > +static int __get_spi(s32 off) > > > { > > > return (-off - 1) / BPF_REG_SIZE; > > > } > > > > > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > > +{ > > > + int spi; > > > + > > > + if (reg->off % BPF_REG_SIZE) { > > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > > + return -EINVAL; > > > + } > > > > I think this cannot happen. > > > > There are existing selftests that trigger this. Really. Which one is that? Those that you've modified in this patch are hitting "cannot pass in dynptr..." message from the check below, no? > Or do you mean it cannot happen anymore? If so, why? Why would it? There is an alignment check earlier. > > > + if (!tnum_is_const(reg->var_off)) { > > > + verbose(env, "dynptr has to be at the constant offset\n"); > > > + return -EINVAL; > > > + } > > > > This part can. > > > > > + spi = __get_spi(reg->off + reg->var_off.value); > > > + if (spi < 1) { > > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", > > > + (int)(reg->off + reg->var_off.value)); > > > + return -EINVAL; > > > + } > > > + return spi; > > > +} > > > > This one is a more conservative (read: redundant) check. > > The is_spi_bounds_valid() is doing it better. > > The problem is, is_spi_bounds_valid returning an error is not always a problem. > See how in is_dynptr_reg_valid_uninit we just return true on invalid bounds, > then later simulate two 8-byte accesses for uninit_dynptr_regno and rely on it > to grow the stack depth and do MAX_BPF_STACK check. It's a weird one. I'm not sure it's actually correct to do it this way. > > How about keeping get_spi(reg) as error free and use it > > directly in places where it cannot fail without > > defensive WARN_ON_ONCE. > > int get_spi(reg) > > { return (-reg->off - reg->var_off.value - 1) / BPF_REG_SIZE; } > > > > While moving tnum_is_const() check into is_spi_bounds_valid() ? > > > > Like is_spi_bounds_valid(state, reg, spi) ? > > > > We should probably remove BPF_DYNPTR_NR_SLOTS since > > there are so many other places where dynptr is assumed > > to be 16-bytes. That macro doesn't help at all. > > It only causes confusion. > > > > I guess we can replace is_spi_bounds_valid() with a differnet > > helper that checks and computes spi. > > Like get_spi_and_check(state, reg, &spi) > > and use it in places where we have get_spi + is_spi_bounds_valid > > while using unchecked get_spi where it cannot fail? > > > > If we only have get_spi_and_check() we'd have to add > > WARN_ON_ONCE in a few places and that bothers me... > > due to defensive programming... > > If code is so complex that we cannot think it through > > we have to refactor it. Sprinkling WARN_ON_ONCE (just to be sure) > > doesn't inspire confidence. > > > > I will think about this and reply later today.
On Thu, Oct 20, 2022 at 07:43:16AM IST, Alexei Starovoitov wrote: > On Wed, Oct 19, 2022 at 6:04 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > On Thu, Oct 20, 2022 at 12:22:56AM IST, Alexei Starovoitov wrote: > > > On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi > > > <memxor@gmail.com> wrote: > > > > > > > > Currently, the dynptr function is not checking the variable offset part > > > > of PTR_TO_STACK that it needs to check. The fixed offset is considered > > > > when computing the stack pointer index, but if the variable offset was > > > > not a constant (such that it could not be accumulated in reg->off), we > > > > will end up a discrepency where runtime pointer does not point to the > > > > actual stack slot we mark as STACK_DYNPTR. > > > > > > > > It is impossible to precisely track dynptr state when variable offset is > > > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > > > > simply reject the case where reg->var_off is not constant. Then, > > > > consider both reg->off and reg->var_off.value when computing the stack > > > > pointer index. > > > > > > > > A new helper dynptr_get_spi is introduced to hide over these details > > > > since the dynptr needs to be located in multiple places outside the > > > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > > > > need to enforce these checks in all places. > > > > > > > > Note that it is disallowed for unprivileged users to have a non-constant > > > > var_off, so this problem should only be possible to trigger from > > > > programs having CAP_PERFMON. However, its effects can vary. > > > > > > > > Without the fix, it is possible to replace the contents of the dynptr > > > > arbitrarily by making verifier mark different stack slots than actual > > > > location and then doing writes to the actual stack address of dynptr at > > > > runtime. > > > > > > > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > --- > > > > kernel/bpf/verifier.c | 80 +++++++++++++++---- > > > > .../testing/selftests/bpf/prog_tests/dynptr.c | 6 +- > > > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > > > > 3 files changed, 67 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > index 8f667180f70f..0fd73f96c5e2 100644 > > > > --- a/kernel/bpf/verifier.c > > > > +++ b/kernel/bpf/verifier.c > > > > @@ -610,11 +610,34 @@ static void print_liveness(struct bpf_verifier_env *env, > > > > verbose(env, "D"); > > > > } > > > > > > > > -static int get_spi(s32 off) > > > > +static int __get_spi(s32 off) > > > > { > > > > return (-off - 1) / BPF_REG_SIZE; > > > > } > > > > > > > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > > > +{ > > > > + int spi; > > > > + > > > > + if (reg->off % BPF_REG_SIZE) { > > > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > > > + return -EINVAL; > > > > + } > > > > > > I think this cannot happen. > > > > > > > There are existing selftests that trigger this. > > Really. Which one is that? > Those that you've modified in this patch are hitting > "cannot pass in dynptr..." message from the check below, no? > Just taking one example, invalid_read2 which does: bpf_dynptr_read(read_data, sizeof(read_data), (void *)&ptr + 1, 0, 0); does hit this one, it passes fp-15, no var_off. Same with invalid_helper2 that was updated. Same with invalid_offset that was updated. invalid_write3 gained coverage from this patch, earlier it was probably just being rejected because of arg_type_is_release checking spilled_ptr.id. not_valid_dynptr is also hitting this one, not the one below. The others now started hitting this error as the order of checks was changed in the verifier. Since arg_type_is_release checking happens before process_dynptr_func, it uses dynptr_get_spi to check ref_obj_id of spilled_ptr. At that point no checks have been made of the dynptr argument, so dynptr_get_spi is required to ensure spi is in bounds. The reg->off % BPF_REG_SIZE was earlier in check_func_arg_reg_off but that alone is not sufficient. This is why I wrapped everything into dynptr_get_spi. > > Or do you mean it cannot happen anymore? If so, why? > > Why would it? There is an alignment check earlier. > I removed the one in check_func_arg_reg_off. So this is the only place now where this alignment check happens. > > > > + if (!tnum_is_const(reg->var_off)) { > > > > + verbose(env, "dynptr has to be at the constant offset\n"); > > > > + return -EINVAL; > > > > + } > > > > > > This part can. > > > > > > > + spi = __get_spi(reg->off + reg->var_off.value); > > > > + if (spi < 1) { > > > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", > > > > + (int)(reg->off + reg->var_off.value)); > > > > + return -EINVAL; > > > > + } > > > > + return spi; > > > > +} > > > > > > This one is a more conservative (read: redundant) check. > > > The is_spi_bounds_valid() is doing it better. > > > > The problem is, is_spi_bounds_valid returning an error is not always a problem. > > See how in is_dynptr_reg_valid_uninit we just return true on invalid bounds, > > then later simulate two 8-byte accesses for uninit_dynptr_regno and rely on it > > to grow the stack depth and do MAX_BPF_STACK check. > > It's a weird one. I'm not sure it's actually correct to do it this way. > Yeah, when looking at this I was actually surprised by that return true, thinking that was by accident and the stack depth was not being updated, but it later happens using check_mem_access in that if block. I'm open to other ideas, like separating out code in check_stack_write_fixed_off, but the only issue is code divergence and we miss checks we need to in both places due to duplication. Let me know what you think. But however you do it, it has to be done after check_func_arg. The stack depth should not be updated until all other arguments have been checked. If you consider meta.access_size handling, that happens in a similar way.
On Wed, Oct 19, 2022 at 7:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote: > > On Thu, Oct 20, 2022 at 07:43:16AM IST, Alexei Starovoitov wrote: > > On Wed, Oct 19, 2022 at 6:04 PM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > > > > On Thu, Oct 20, 2022 at 12:22:56AM IST, Alexei Starovoitov wrote: > > > > On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi > > > > <memxor@gmail.com> wrote: > > > > > > > > > > Currently, the dynptr function is not checking the variable offset part > > > > > of PTR_TO_STACK that it needs to check. The fixed offset is considered > > > > > when computing the stack pointer index, but if the variable offset was > > > > > not a constant (such that it could not be accumulated in reg->off), we > > > > > will end up a discrepency where runtime pointer does not point to the > > > > > actual stack slot we mark as STACK_DYNPTR. > > > > > > > > > > It is impossible to precisely track dynptr state when variable offset is > > > > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > > > > > simply reject the case where reg->var_off is not constant. Then, > > > > > consider both reg->off and reg->var_off.value when computing the stack > > > > > pointer index. > > > > > > > > > > A new helper dynptr_get_spi is introduced to hide over these details > > > > > since the dynptr needs to be located in multiple places outside the > > > > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > > > > > need to enforce these checks in all places. > > > > > > > > > > Note that it is disallowed for unprivileged users to have a non-constant > > > > > var_off, so this problem should only be possible to trigger from > > > > > programs having CAP_PERFMON. However, its effects can vary. > > > > > > > > > > Without the fix, it is possible to replace the contents of the dynptr > > > > > arbitrarily by making verifier mark different stack slots than actual > > > > > location and then doing writes to the actual stack address of dynptr at > > > > > runtime. > > > > > > > > > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > > --- > > > > > kernel/bpf/verifier.c | 80 +++++++++++++++---- > > > > > .../testing/selftests/bpf/prog_tests/dynptr.c | 6 +- > > > > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > > > > > 3 files changed, 67 insertions(+), 21 deletions(-) > > > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > > index 8f667180f70f..0fd73f96c5e2 100644 > > > > > --- a/kernel/bpf/verifier.c > > > > > +++ b/kernel/bpf/verifier.c > > > > > @@ -610,11 +610,34 @@ static void print_liveness(struct bpf_verifier_env *env, > > > > > verbose(env, "D"); > > > > > } > > > > > > > > > > -static int get_spi(s32 off) > > > > > +static int __get_spi(s32 off) > > > > > { > > > > > return (-off - 1) / BPF_REG_SIZE; > > > > > } > > > > > > > > > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > > > > +{ > > > > > + int spi; > > > > > + > > > > > + if (reg->off % BPF_REG_SIZE) { > > > > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > > > > + return -EINVAL; > > > > > + } > > > > > > > > I think this cannot happen. > > > > > > > > > > There are existing selftests that trigger this. > > > > Really. Which one is that? > > Those that you've modified in this patch are hitting > > "cannot pass in dynptr..." message from the check below, no? > > > > Just taking one example, invalid_read2 which does: > > bpf_dynptr_read(read_data, sizeof(read_data), (void *)&ptr + 1, 0, 0); > > does hit this one, it passes fp-15, no var_off. > > Same with invalid_helper2 that was updated. > Same with invalid_offset that was updated. > invalid_write3 gained coverage from this patch, earlier it was probably just > being rejected because of arg_type_is_release checking spilled_ptr.id. > not_valid_dynptr is also hitting this one, not the one below. > > The others now started hitting this error as the order of checks was changed in > the verifier. Since arg_type_is_release checking happens before > process_dynptr_func, it uses dynptr_get_spi to check ref_obj_id of spilled_ptr. > At that point no checks have been made of the dynptr argument, so dynptr_get_spi > is required to ensure spi is in bounds. > > The reg->off % BPF_REG_SIZE was earlier in check_func_arg_reg_off but that alone > is not sufficient. This is why I wrapped everything into dynptr_get_spi. I see. That was not obvious at all that some other patch is removing that check from check_func_arg_reg_off. Why is the check there not sufficient? > > > Or do you mean it cannot happen anymore? If so, why? > > > > Why would it? There is an alignment check earlier. > > > > I removed the one in check_func_arg_reg_off. So this is the only place now where > this alignment check happens. > > > > > > + if (!tnum_is_const(reg->var_off)) { > > > > > + verbose(env, "dynptr has to be at the constant offset\n"); > > > > > + return -EINVAL; > > > > > + } > > > > > > > > This part can. > > > > > > > > > + spi = __get_spi(reg->off + reg->var_off.value); > > > > > + if (spi < 1) { > > > > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", > > > > > + (int)(reg->off + reg->var_off.value)); > > > > > + return -EINVAL; > > > > > + } > > > > > + return spi; > > > > > +} > > > > > > > > This one is a more conservative (read: redundant) check. > > > > The is_spi_bounds_valid() is doing it better. > > > > > > The problem is, is_spi_bounds_valid returning an error is not always a problem. > > > See how in is_dynptr_reg_valid_uninit we just return true on invalid bounds, > > > then later simulate two 8-byte accesses for uninit_dynptr_regno and rely on it > > > to grow the stack depth and do MAX_BPF_STACK check. > > > > It's a weird one. I'm not sure it's actually correct to do it this way. > > > > Yeah, when looking at this I was actually surprised by that return true, > thinking that was by accident and the stack depth was not being updated, but it > later happens using check_mem_access in that if block. > > I'm open to other ideas, like separating out code in > check_stack_write_fixed_off, but the only issue is code divergence and we miss > checks we need to in both places due to duplication. Let me know what you think. Not following. Why check_stack_write_fixed_off has to do with any of that? The bug you're fixing is missing tnum_is_const(reg->var_off), right? All other changes make it hard to understand what is going on. > But however you do it, it has to be done after check_func_arg. The stack depth > should not be updated until all other arguments have been checked. If you > consider meta.access_size handling, that happens in a similar way. Not following.
On Thu, Oct 20, 2022 at 08:26:44AM IST, Alexei Starovoitov wrote: > On Wed, Oct 19, 2022 at 7:40 PM Kumar Kartikeya Dwivedi > <memxor@gmail.com> wrote: > > > > On Thu, Oct 20, 2022 at 07:43:16AM IST, Alexei Starovoitov wrote: > > > On Wed, Oct 19, 2022 at 6:04 PM Kumar Kartikeya Dwivedi > > > <memxor@gmail.com> wrote: > > > > > > > > On Thu, Oct 20, 2022 at 12:22:56AM IST, Alexei Starovoitov wrote: > > > > > On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi > > > > > <memxor@gmail.com> wrote: > > > > > > > > > > > > Currently, the dynptr function is not checking the variable offset part > > > > > > of PTR_TO_STACK that it needs to check. The fixed offset is considered > > > > > > when computing the stack pointer index, but if the variable offset was > > > > > > not a constant (such that it could not be accumulated in reg->off), we > > > > > > will end up a discrepency where runtime pointer does not point to the > > > > > > actual stack slot we mark as STACK_DYNPTR. > > > > > > > > > > > > It is impossible to precisely track dynptr state when variable offset is > > > > > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > > > > > > simply reject the case where reg->var_off is not constant. Then, > > > > > > consider both reg->off and reg->var_off.value when computing the stack > > > > > > pointer index. > > > > > > > > > > > > A new helper dynptr_get_spi is introduced to hide over these details > > > > > > since the dynptr needs to be located in multiple places outside the > > > > > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > > > > > > need to enforce these checks in all places. > > > > > > > > > > > > Note that it is disallowed for unprivileged users to have a non-constant > > > > > > var_off, so this problem should only be possible to trigger from > > > > > > programs having CAP_PERFMON. However, its effects can vary. > > > > > > > > > > > > Without the fix, it is possible to replace the contents of the dynptr > > > > > > arbitrarily by making verifier mark different stack slots than actual > > > > > > location and then doing writes to the actual stack address of dynptr at > > > > > > runtime. > > > > > > > > > > > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > > > --- > > > > > > kernel/bpf/verifier.c | 80 +++++++++++++++---- > > > > > > .../testing/selftests/bpf/prog_tests/dynptr.c | 6 +- > > > > > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > > > > > > 3 files changed, 67 insertions(+), 21 deletions(-) > > > > > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > > > index 8f667180f70f..0fd73f96c5e2 100644 > > > > > > --- a/kernel/bpf/verifier.c > > > > > > +++ b/kernel/bpf/verifier.c > > > > > > @@ -610,11 +610,34 @@ static void print_liveness(struct bpf_verifier_env *env, > > > > > > verbose(env, "D"); > > > > > > } > > > > > > > > > > > > -static int get_spi(s32 off) > > > > > > +static int __get_spi(s32 off) > > > > > > { > > > > > > return (-off - 1) / BPF_REG_SIZE; > > > > > > } > > > > > > > > > > > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > > > > > +{ > > > > > > + int spi; > > > > > > + > > > > > > + if (reg->off % BPF_REG_SIZE) { > > > > > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > > > > > + return -EINVAL; > > > > > > + } > > > > > > > > > > I think this cannot happen. > > > > > > > > > > > > > There are existing selftests that trigger this. > > > > > > Really. Which one is that? > > > Those that you've modified in this patch are hitting > > > "cannot pass in dynptr..." message from the check below, no? > > > > > > > Just taking one example, invalid_read2 which does: > > > > bpf_dynptr_read(read_data, sizeof(read_data), (void *)&ptr + 1, 0, 0); > > > > does hit this one, it passes fp-15, no var_off. > > > > Same with invalid_helper2 that was updated. > > Same with invalid_offset that was updated. > > invalid_write3 gained coverage from this patch, earlier it was probably just > > being rejected because of arg_type_is_release checking spilled_ptr.id. > > not_valid_dynptr is also hitting this one, not the one below. > > > > The others now started hitting this error as the order of checks was changed in > > the verifier. Since arg_type_is_release checking happens before > > process_dynptr_func, it uses dynptr_get_spi to check ref_obj_id of spilled_ptr. > > At that point no checks have been made of the dynptr argument, so dynptr_get_spi > > is required to ensure spi is in bounds. > > > > The reg->off % BPF_REG_SIZE was earlier in check_func_arg_reg_off but that alone > > is not sufficient. This is why I wrapped everything into dynptr_get_spi. > > I see. That was not obvious at all that some other patch > is removing that check from check_func_arg_reg_off. > It is done in patch 4. There I move that check from the check_func_arg_reg_off to process_dynptr_func. > Why is the check there not sufficient? > I wanted to keep check_func_arg_reg_off free of assumptions for helper specific checks. It just ensures a few rules: When OBJ_RELEASE, offsets (fixed and var are 0) Otherwise, for some specific register types, allow fixed and var_off. For PTR_TO_BTF_ID, allow fixed but not var_off. Reject any fixed or var_off for all other cases. Everything else is handled on top of that. > > > > Or do you mean it cannot happen anymore? If so, why? > > > > > > Why would it? There is an alignment check earlier. > > > > > > > I removed the one in check_func_arg_reg_off. So this is the only place now where > > this alignment check happens. > > > > > > > > + if (!tnum_is_const(reg->var_off)) { > > > > > > + verbose(env, "dynptr has to be at the constant offset\n"); > > > > > > + return -EINVAL; > > > > > > + } > > > > > > > > > > This part can. > > > > > > > > > > > + spi = __get_spi(reg->off + reg->var_off.value); > > > > > > + if (spi < 1) { > > > > > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", > > > > > > + (int)(reg->off + reg->var_off.value)); > > > > > > + return -EINVAL; > > > > > > + } > > > > > > + return spi; > > > > > > +} > > > > > > > > > > This one is a more conservative (read: redundant) check. > > > > > The is_spi_bounds_valid() is doing it better. > > > > > > > > The problem is, is_spi_bounds_valid returning an error is not always a problem. > > > > See how in is_dynptr_reg_valid_uninit we just return true on invalid bounds, > > > > then later simulate two 8-byte accesses for uninit_dynptr_regno and rely on it > > > > to grow the stack depth and do MAX_BPF_STACK check. > > > > > > It's a weird one. I'm not sure it's actually correct to do it this way. > > > > > > > Yeah, when looking at this I was actually surprised by that return true, > > thinking that was by accident and the stack depth was not being updated, but it > > later happens using check_mem_access in that if block. > > > > I'm open to other ideas, like separating out code in > > check_stack_write_fixed_off, but the only issue is code divergence and we miss > > checks we need to in both places due to duplication. Let me know what you think. > > Not following. Why check_stack_write_fixed_off has to do with any of that? > Well, I thought you didn't consider check_mem_access based simulation of writes to grow stack bounds to be clean, so I was soliciting opinions on how it could be done otherwise. It ends up calling check_stack_write_fixed_off internally. per > > > It's a weird one. I'm not sure it's actually correct to do it this way. but maybe I misunderstood and you meant it for is_spi_bounds_valid only. > The bug you're fixing is missing tnum_is_const(reg->var_off), right? > All other changes make it hard to understand what is going on. > In this patch, there is no other change. Every site that used get_spi(reg->off) now uses get_spi(reg->off + reg->var_off.value) essentially. For dynptr, only spi 1 and above are valid values. The main ugliness comes because it needs to get ref_obj_id earlier before argument processing begins in arg_type_is_release block. Maybe that step should be moved later below, I don't see anything using meta->ref_obj_id inside functions called by the switch case. Also, going back to what you said earlier: > If we only have get_spi_and_check() we'd have to add > WARN_ON_ONCE in a few places and that bothers me... > due to defensive programming... > If code is so complex that we cannot think it through > we have to refactor it. Sprinkling WARN_ON_ONCE (just to be sure) > doesn't inspire confidence. > Once we are done with process_dynptr_func, the rest of code can assume it points to a valid stack location where dynptr needs to be marked/unmarked, so the rest of the code doesn't do any checking of the spi etc.
On Thu, Oct 20, 2022 at 08:53:45AM +0530, Kumar Kartikeya Dwivedi wrote: > On Thu, Oct 20, 2022 at 08:26:44AM IST, Alexei Starovoitov wrote: > > On Wed, Oct 19, 2022 at 7:40 PM Kumar Kartikeya Dwivedi > > <memxor@gmail.com> wrote: > > > > > > On Thu, Oct 20, 2022 at 07:43:16AM IST, Alexei Starovoitov wrote: > > > > On Wed, Oct 19, 2022 at 6:04 PM Kumar Kartikeya Dwivedi > > > > <memxor@gmail.com> wrote: > > > > > > > > > > On Thu, Oct 20, 2022 at 12:22:56AM IST, Alexei Starovoitov wrote: > > > > > > On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi > > > > > > <memxor@gmail.com> wrote: > > > > > > > > > > > > > > Currently, the dynptr function is not checking the variable offset part > > > > > > > of PTR_TO_STACK that it needs to check. The fixed offset is considered > > > > > > > when computing the stack pointer index, but if the variable offset was > > > > > > > not a constant (such that it could not be accumulated in reg->off), we > > > > > > > will end up a discrepency where runtime pointer does not point to the > > > > > > > actual stack slot we mark as STACK_DYNPTR. > > > > > > > > > > > > > > It is impossible to precisely track dynptr state when variable offset is > > > > > > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > > > > > > > simply reject the case where reg->var_off is not constant. Then, > > > > > > > consider both reg->off and reg->var_off.value when computing the stack > > > > > > > pointer index. > > > > > > > > > > > > > > A new helper dynptr_get_spi is introduced to hide over these details > > > > > > > since the dynptr needs to be located in multiple places outside the > > > > > > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > > > > > > > need to enforce these checks in all places. > > > > > > > > > > > > > > Note that it is disallowed for unprivileged users to have a non-constant > > > > > > > var_off, so this problem should only be possible to trigger from > > > > > > > programs having CAP_PERFMON. However, its effects can vary. > > > > > > > > > > > > > > Without the fix, it is possible to replace the contents of the dynptr > > > > > > > arbitrarily by making verifier mark different stack slots than actual > > > > > > > location and then doing writes to the actual stack address of dynptr at > > > > > > > runtime. > > > > > > > > > > > > > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > > > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > > > > --- > > > > > > > kernel/bpf/verifier.c | 80 +++++++++++++++---- > > > > > > > .../testing/selftests/bpf/prog_tests/dynptr.c | 6 +- > > > > > > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > > > > > > > 3 files changed, 67 insertions(+), 21 deletions(-) > > > > > > > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > > > > index 8f667180f70f..0fd73f96c5e2 100644 > > > > > > > --- a/kernel/bpf/verifier.c > > > > > > > +++ b/kernel/bpf/verifier.c > > > > > > > @@ -610,11 +610,34 @@ static void print_liveness(struct bpf_verifier_env *env, > > > > > > > verbose(env, "D"); > > > > > > > } > > > > > > > > > > > > > > -static int get_spi(s32 off) > > > > > > > +static int __get_spi(s32 off) > > > > > > > { > > > > > > > return (-off - 1) / BPF_REG_SIZE; > > > > > > > } > > > > > > > > > > > > > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > > > > > > +{ > > > > > > > + int spi; > > > > > > > + > > > > > > > + if (reg->off % BPF_REG_SIZE) { > > > > > > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > > > > > > I think this cannot happen. > > > > > > > > > > > > > > > > There are existing selftests that trigger this. > > > > > > > > Really. Which one is that? > > > > Those that you've modified in this patch are hitting > > > > "cannot pass in dynptr..." message from the check below, no? > > > > > > > > > > Just taking one example, invalid_read2 which does: > > > > > > bpf_dynptr_read(read_data, sizeof(read_data), (void *)&ptr + 1, 0, 0); > > > > > > does hit this one, it passes fp-15, no var_off. > > > > > > Same with invalid_helper2 that was updated. > > > Same with invalid_offset that was updated. > > > invalid_write3 gained coverage from this patch, earlier it was probably just > > > being rejected because of arg_type_is_release checking spilled_ptr.id. > > > not_valid_dynptr is also hitting this one, not the one below. > > > > > > The others now started hitting this error as the order of checks was changed in > > > the verifier. Since arg_type_is_release checking happens before > > > process_dynptr_func, it uses dynptr_get_spi to check ref_obj_id of spilled_ptr. > > > At that point no checks have been made of the dynptr argument, so dynptr_get_spi > > > is required to ensure spi is in bounds. > > > > > > The reg->off % BPF_REG_SIZE was earlier in check_func_arg_reg_off but that alone > > > is not sufficient. This is why I wrapped everything into dynptr_get_spi. > > > > I see. That was not obvious at all that some other patch > > is removing that check from check_func_arg_reg_off. > > > > It is done in patch 4. There I move that check from the check_func_arg_reg_off > to process_dynptr_func. "Finally, since check_func_arg_reg_off is meant to be generic, move dynptr specific check into process_dynptr_func." It's a sign that patch 4 is doing too much. It should be at least two patches. > > > Why is the check there not sufficient? > > > > I wanted to keep check_func_arg_reg_off free of assumptions for helper specific > checks. It just ensures a few rules: Currently it's case PTR_TO_STACK: if (arg_type_is_dynptr(arg_type) && reg->off % BPF_REG_SIZE) { it's not really helper specific. process_dynptr_func may be the right palce to check for alignment, but imo the patch set is doing way too much. Instead of fixing dynptr specific issues it goes into massive refactoring. Please do one or the other. One patch set for refactoring only with no functional changes. Another patch set with fixes. Either order is fine.
On Fri, Oct 21, 2022 at 06:16:27AM IST, Alexei Starovoitov wrote: > On Thu, Oct 20, 2022 at 08:53:45AM +0530, Kumar Kartikeya Dwivedi wrote: > > On Thu, Oct 20, 2022 at 08:26:44AM IST, Alexei Starovoitov wrote: > > > On Wed, Oct 19, 2022 at 7:40 PM Kumar Kartikeya Dwivedi > > > <memxor@gmail.com> wrote: > > > > > > > > On Thu, Oct 20, 2022 at 07:43:16AM IST, Alexei Starovoitov wrote: > > > > > On Wed, Oct 19, 2022 at 6:04 PM Kumar Kartikeya Dwivedi > > > > > <memxor@gmail.com> wrote: > > > > > > > > > > > > On Thu, Oct 20, 2022 at 12:22:56AM IST, Alexei Starovoitov wrote: > > > > > > > On Tue, Oct 18, 2022 at 6:59 AM Kumar Kartikeya Dwivedi > > > > > > > <memxor@gmail.com> wrote: > > > > > > > > > > > > > > > > Currently, the dynptr function is not checking the variable offset part > > > > > > > > of PTR_TO_STACK that it needs to check. The fixed offset is considered > > > > > > > > when computing the stack pointer index, but if the variable offset was > > > > > > > > not a constant (such that it could not be accumulated in reg->off), we > > > > > > > > will end up a discrepency where runtime pointer does not point to the > > > > > > > > actual stack slot we mark as STACK_DYNPTR. > > > > > > > > > > > > > > > > It is impossible to precisely track dynptr state when variable offset is > > > > > > > > not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. > > > > > > > > simply reject the case where reg->var_off is not constant. Then, > > > > > > > > consider both reg->off and reg->var_off.value when computing the stack > > > > > > > > pointer index. > > > > > > > > > > > > > > > > A new helper dynptr_get_spi is introduced to hide over these details > > > > > > > > since the dynptr needs to be located in multiple places outside the > > > > > > > > process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we > > > > > > > > need to enforce these checks in all places. > > > > > > > > > > > > > > > > Note that it is disallowed for unprivileged users to have a non-constant > > > > > > > > var_off, so this problem should only be possible to trigger from > > > > > > > > programs having CAP_PERFMON. However, its effects can vary. > > > > > > > > > > > > > > > > Without the fix, it is possible to replace the contents of the dynptr > > > > > > > > arbitrarily by making verifier mark different stack slots than actual > > > > > > > > location and then doing writes to the actual stack address of dynptr at > > > > > > > > runtime. > > > > > > > > > > > > > > > > Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") > > > > > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> > > > > > > > > --- > > > > > > > > kernel/bpf/verifier.c | 80 +++++++++++++++---- > > > > > > > > .../testing/selftests/bpf/prog_tests/dynptr.c | 6 +- > > > > > > > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- > > > > > > > > 3 files changed, 67 insertions(+), 21 deletions(-) > > > > > > > > > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > > > > > index 8f667180f70f..0fd73f96c5e2 100644 > > > > > > > > --- a/kernel/bpf/verifier.c > > > > > > > > +++ b/kernel/bpf/verifier.c > > > > > > > > @@ -610,11 +610,34 @@ static void print_liveness(struct bpf_verifier_env *env, > > > > > > > > verbose(env, "D"); > > > > > > > > } > > > > > > > > > > > > > > > > -static int get_spi(s32 off) > > > > > > > > +static int __get_spi(s32 off) > > > > > > > > { > > > > > > > > return (-off - 1) / BPF_REG_SIZE; > > > > > > > > } > > > > > > > > > > > > > > > > +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) > > > > > > > > +{ > > > > > > > > + int spi; > > > > > > > > + > > > > > > > > + if (reg->off % BPF_REG_SIZE) { > > > > > > > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); > > > > > > > > + return -EINVAL; > > > > > > > > + } > > > > > > > > > > > > > > I think this cannot happen. > > > > > > > > > > > > > > > > > > > There are existing selftests that trigger this. > > > > > > > > > > Really. Which one is that? > > > > > Those that you've modified in this patch are hitting > > > > > "cannot pass in dynptr..." message from the check below, no? > > > > > > > > > > > > > Just taking one example, invalid_read2 which does: > > > > > > > > bpf_dynptr_read(read_data, sizeof(read_data), (void *)&ptr + 1, 0, 0); > > > > > > > > does hit this one, it passes fp-15, no var_off. > > > > > > > > Same with invalid_helper2 that was updated. > > > > Same with invalid_offset that was updated. > > > > invalid_write3 gained coverage from this patch, earlier it was probably just > > > > being rejected because of arg_type_is_release checking spilled_ptr.id. > > > > not_valid_dynptr is also hitting this one, not the one below. > > > > > > > > The others now started hitting this error as the order of checks was changed in > > > > the verifier. Since arg_type_is_release checking happens before > > > > process_dynptr_func, it uses dynptr_get_spi to check ref_obj_id of spilled_ptr. > > > > At that point no checks have been made of the dynptr argument, so dynptr_get_spi > > > > is required to ensure spi is in bounds. > > > > > > > > The reg->off % BPF_REG_SIZE was earlier in check_func_arg_reg_off but that alone > > > > is not sufficient. This is why I wrapped everything into dynptr_get_spi. > > > > > > I see. That was not obvious at all that some other patch > > > is removing that check from check_func_arg_reg_off. > > > > > > > It is done in patch 4. There I move that check from the check_func_arg_reg_off > > to process_dynptr_func. > > "Finally, since check_func_arg_reg_off is meant to be generic, move > dynptr specific check into process_dynptr_func." > > It's a sign that patch 4 is doing too much. It should be at least two patches. > > > > > > Why is the check there not sufficient? > > > > > > > I wanted to keep check_func_arg_reg_off free of assumptions for helper specific > > checks. It just ensures a few rules: > > Currently it's > case PTR_TO_STACK: > if (arg_type_is_dynptr(arg_type) && reg->off % BPF_REG_SIZE) { > it's not really helper specific. > > process_dynptr_func may be the right palce to check for alignment, > but imo the patch set is doing way too much. > Instead of fixing dynptr specific issues it goes into massive refactoring. > Please do one or the other. > One patch set for refactoring only with no functional changes. > Another patch set with fixes. > Either order is fine. Ok, I will split it into two. First send the refactorings (and incorporate feedback based on the discussion), and then the fixes on top of that. Thanks.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 8f667180f70f..0fd73f96c5e2 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -610,11 +610,34 @@ static void print_liveness(struct bpf_verifier_env *env, verbose(env, "D"); } -static int get_spi(s32 off) +static int __get_spi(s32 off) { return (-off - 1) / BPF_REG_SIZE; } +static int dynptr_get_spi(struct bpf_verifier_env *env, struct bpf_reg_state *reg) +{ + int spi; + + if (reg->off % BPF_REG_SIZE) { + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); + return -EINVAL; + } + + if (!tnum_is_const(reg->var_off)) { + verbose(env, "dynptr has to be at the constant offset\n"); + return -EINVAL; + } + + spi = __get_spi(reg->off + reg->var_off.value); + if (spi < 1) { + verbose(env, "cannot pass in dynptr at an offset=%d\n", + (int)(reg->off + reg->var_off.value)); + return -EINVAL; + } + return spi; +} + static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots) { int allocated_slots = state->allocated_stack / BPF_REG_SIZE; @@ -725,7 +748,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ enum bpf_dynptr_type type; int spi, i, id; - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (spi < 0) + return spi; if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) return -EINVAL; @@ -763,7 +788,9 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re struct bpf_func_state *state = func(env, reg); int spi, i; - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (spi < 0) + return spi; if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) return -EINVAL; @@ -810,7 +837,11 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ if (reg->type == CONST_PTR_TO_DYNPTR) return false; - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (spi < 0) + return spi; + + /* We will do check_mem_access to check and update stack bounds later */ if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) return true; @@ -826,14 +857,15 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg) { struct bpf_func_state *state = func(env, reg); - int spi; - int i; + int spi, i; /* This already represents first slot of initialized bpf_dynptr */ if (reg->type == CONST_PTR_TO_DYNPTR) return true; - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (spi < 0) + return false; if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) || !state->stack[spi].spilled_ptr.dynptr.first_slot) return false; @@ -864,7 +896,9 @@ static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg if (reg->type == CONST_PTR_TO_DYNPTR) { return reg->dynptr.type == dynptr_type; } else { - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (WARN_ON_ONCE(spi < 0)) + return false; return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type; } } @@ -2388,7 +2422,9 @@ static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state * */ if (reg->type == CONST_PTR_TO_DYNPTR) return 0; - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (WARN_ON_ONCE(spi < 0)) + return spi; /* Caller ensures dynptr is valid and initialized, which means spi is in * bounds and spi is the first dynptr slot. Simply mark stack slot as * read. @@ -5663,6 +5699,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, return 0; } +static bool arg_type_is_release(enum bpf_arg_type type) +{ + return type & OBJ_RELEASE; +} + /* Implementation details: * * There are two register types representing a bpf_dynptr, one is PTR_TO_STACK @@ -5710,6 +5751,13 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, return -EINVAL; } + /* Additional check for PTR_TO_STACK offset */ + if (reg->type == PTR_TO_STACK) { + err = dynptr_get_spi(env, reg); + if (err < 0) + return err; + } + /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to a * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): * @@ -5728,7 +5776,6 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, * destroyed, including mutation of the memory it points * to. */ - if (arg_type & MEM_UNINIT) { if (!is_dynptr_reg_valid_uninit(env, reg)) { verbose(env, "Dynptr has to be an uninitialized dynptr\n"); @@ -5791,11 +5838,6 @@ static bool arg_type_is_mem_size(enum bpf_arg_type type) type == ARG_CONST_SIZE_OR_ZERO; } -static bool arg_type_is_release(enum bpf_arg_type type) -{ - return type & OBJ_RELEASE; -} - static bool arg_type_is_dynptr(enum bpf_arg_type type) { return base_type(type) == ARG_PTR_TO_DYNPTR; @@ -6122,7 +6164,9 @@ static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state if (reg->type == CONST_PTR_TO_DYNPTR) return reg->ref_obj_id; - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (WARN_ON_ONCE(spi < 0)) + return U32_MAX; return state->stack[spi].spilled_ptr.ref_obj_id; } @@ -6190,7 +6234,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, int spi; if (reg->type == PTR_TO_STACK) { - spi = get_spi(reg->off); + spi = dynptr_get_spi(env, reg); + if (spi < 0) + return spi; if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) || !state->stack[spi].spilled_ptr.ref_obj_id) { verbose(env, "arg %d is an unacquired reference\n", regno); diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c index 8fc4e6c02bfd..947126d217bd 100644 --- a/tools/testing/selftests/bpf/prog_tests/dynptr.c +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c @@ -27,16 +27,16 @@ static struct { {"data_slice_missing_null_check1", "invalid mem access 'mem_or_null'"}, {"data_slice_missing_null_check2", "invalid mem access 'mem_or_null'"}, {"invalid_helper1", "invalid indirect read from stack"}, - {"invalid_helper2", "Expected an initialized dynptr as arg #3"}, + {"invalid_helper2", "cannot pass in dynptr at an offset=-8"}, {"invalid_write1", "Expected an initialized dynptr as arg #1"}, {"invalid_write2", "Expected an initialized dynptr as arg #3"}, - {"invalid_write3", "Expected an initialized dynptr as arg #1"}, + {"invalid_write3", "arg 1 is an unacquired reference"}, {"invalid_write4", "arg 1 is an unacquired reference"}, {"invalid_read1", "invalid read from stack"}, {"invalid_read2", "cannot pass in dynptr at an offset"}, {"invalid_read3", "invalid read from stack"}, {"invalid_read4", "invalid read from stack"}, - {"invalid_offset", "invalid write to stack"}, + {"invalid_offset", "cannot pass in dynptr at an offset=0"}, {"global", "type=map_value expected=fp"}, {"release_twice", "arg 1 is an unacquired reference"}, {"release_twice_callback", "arg 1 is an unacquired reference"}, diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c index fc562e863e79..e4b970bc2d3f 100644 --- a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c @@ -18,7 +18,7 @@ static struct { const char *expected_verifier_err_msg; int expected_runtime_err; } kfunc_dynptr_tests[] = { - {"not_valid_dynptr", "Expected an initialized dynptr as arg #1", 0}, + {"not_valid_dynptr", "cannot pass in dynptr at an offset=-8", 0}, {"not_ptr_to_stack", "arg#0 pointer type STRUCT bpf_dynptr_kern not to stack", 0}, {"dynptr_data_null", NULL, -EBADMSG}, };
Currently, the dynptr function is not checking the variable offset part of PTR_TO_STACK that it needs to check. The fixed offset is considered when computing the stack pointer index, but if the variable offset was not a constant (such that it could not be accumulated in reg->off), we will end up a discrepency where runtime pointer does not point to the actual stack slot we mark as STACK_DYNPTR. It is impossible to precisely track dynptr state when variable offset is not constant, hence, just like bpf_timer, kptr, bpf_spin_lock, etc. simply reject the case where reg->var_off is not constant. Then, consider both reg->off and reg->var_off.value when computing the stack pointer index. A new helper dynptr_get_spi is introduced to hide over these details since the dynptr needs to be located in multiple places outside the process_dynptr_func checks, hence once we know it's a PTR_TO_STACK, we need to enforce these checks in all places. Note that it is disallowed for unprivileged users to have a non-constant var_off, so this problem should only be possible to trigger from programs having CAP_PERFMON. However, its effects can vary. Without the fix, it is possible to replace the contents of the dynptr arbitrarily by making verifier mark different stack slots than actual location and then doing writes to the actual stack address of dynptr at runtime. Fixes: 97e03f521050 ("bpf: Add verifier support for dynptrs") Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/verifier.c | 80 +++++++++++++++---- .../testing/selftests/bpf/prog_tests/dynptr.c | 6 +- .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +- 3 files changed, 67 insertions(+), 21 deletions(-)