On Thu, Jan 19, 2023 at 2:14 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Fri, 20 Jan 2023 at 03:30, Joanne Koong <joannelkoong@xxxxxxxxx> wrote:
> >
> > On Wed, Jan 18, 2023 at 6:14 PM 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>
Acked-by: Joanne Koong <joannelkoong@xxxxxxxxx>
> > > ---
> > > kernel/bpf/verifier.c | 83 +++++++++++++++----
> > > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +-
> > > .../testing/selftests/bpf/progs/dynptr_fail.c | 4 +-
> > > 3 files changed, 68 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > > index 89de5bc46f27..eeb6f1b2bd60 100644
> > > --- a/kernel/bpf/verifier.c
> > > +++ b/kernel/bpf/verifier.c
> > > @@ -638,11 +638,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 off, spi;
> > > +
> > > + if (!tnum_is_const(reg->var_off)) {
> > > + verbose(env, "dynptr has to be at the constant offset\n");
> >
> > nit: "at a constant offset" instead of "at the constant offset"?
> >
>
> Will fix.
>
> > > + return -EINVAL;
> > > + }
> > > +
> > > + off = reg->off + reg->var_off.value;
> > > + if (off % BPF_REG_SIZE) {
> > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + spi = __get_spi(off);
> > > + if (spi < 1) {
> > > + verbose(env, "cannot pass in dynptr at an offset=%d\n", off);
> > > + return -EINVAL;
> > > + }
> >
> > I still think this if (spi < 1) check should have the same logic
> > is_spi_bounds_valid() does (eg checking against total allocated slots
> > as well). I think we can combine is_spi_bounds_valid() with this
> > function and then remove every place we call is_spi_bounds_valid().
> > WDYT?
> >
>
> I believe I addressed this in patch 5, but kept the name of the
> combined check as dynptr_get_spi instead (it looks better to me in the
> context of the code compared to is_spi_bounds_valid). Please take a
> look.
Ok, I see, these changes are in a separate patch.
