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.
> + 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.
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.
> +
> static bool is_spi_bounds_valid(struct bpf_func_state *state, int spi, int nr_slots)
> {
> int allocated_slots = state->allocated_stack / BPF_REG_SIZE;
> @@ -725,7 +748,9 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_
> enum bpf_dynptr_type type;
> int spi, i, id;
>
> - spi = get_spi(reg->off);
> + spi = dynptr_get_spi(env, reg);
> + if (spi < 0)
> + return spi;
>
> if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> return -EINVAL;
