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?
> 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.
