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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> > > > > > > ---
> > > > > > > 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.
