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