Re: [PATCH bpf-next v1 06/13] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

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.