From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Joanne Koong <joannelkoong@gmail.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
Andrii Nakryiko <andrii@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
Martin KaFai Lau <martin.lau@kernel.org>,
David Vernet <void@manifault.com>,
Eduard Zingerman <eddyz87@gmail.com>
Subject: Re: [PATCH bpf-next v1 2/8] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR
Date: Mon, 9 Jan 2023 16:51:24 +0530 [thread overview]
Message-ID: <20230109112124.ihkrsf6cjcxyhite@apollo> (raw)
In-Reply-To: <CAJnrk1bfWghAaSr44EC_jsQpc6hVEzKR9iCzGgru3wfedTM6HA@mail.gmail.com>
On Fri, Jan 06, 2023 at 06:27:06AM IST, Joanne Koong wrote:
> On Sun, Jan 1, 2023 at 12:34 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 | 83 ++++++++++++++-----
> > .../bpf/prog_tests/kfunc_dynptr_param.c | 2 +-
> > .../testing/selftests/bpf/progs/dynptr_fail.c | 6 +-
> > 3 files changed, 66 insertions(+), 25 deletions(-)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index f7248235e119..ca970f80e395 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");
> > + 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", reg->off);
>
> I think you meant off instead of reg->off?
>
Ack.
> > + return -EINVAL;
> > + }
> > +
> > + spi = __get_spi(off);
> > + if (spi < 1) {
> > + verbose(env, "cannot pass in dynptr at an offset=%d\n", (int)(off + reg->var_off.value));
>
> I think you meant off instead of off + reg->var_off.value
>
Ack.
> > + return -EINVAL;
> > + }
>
> I 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 every place we call is_spi_bounds_valid()
>
Ok, I'll combine both.
> > + return spi;
> > +}
> > +
> > 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;
> > @@ -754,7 +777,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;
> > @@ -792,7 +817,9 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re
> > struct bpf_func_state *state = func(env, reg);
> > int spi, i;
> >
> > - 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;
> > @@ -839,7 +866,11 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> > if (reg->type == CONST_PTR_TO_DYNPTR)
> > return false;
> >
> > - spi = get_spi(reg->off);
> > + spi = dynptr_get_spi(env, reg);
> > + if (spi < 0)
> > + return spi;
> > +
> > + /* We will do check_mem_access to check and update stack bounds later */
> > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS))
> > return true;
> >
> > @@ -855,14 +886,15 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_
> > static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg)
> > {
> > struct bpf_func_state *state = func(env, reg);
> > - int spi;
> > - int i;
> > + int spi, i;
> >
> > /* This already represents first slot of initialized bpf_dynptr */
> > if (reg->type == CONST_PTR_TO_DYNPTR)
> > return true;
> >
> > - spi = get_spi(reg->off);
> > + spi = dynptr_get_spi(env, reg);
> > + if (spi < 0)
> > + return false;
> > if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) ||
> > !state->stack[spi].spilled_ptr.dynptr.first_slot)
> > return false;
> > @@ -891,7 +923,9 @@ static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg
> > if (reg->type == CONST_PTR_TO_DYNPTR) {
> > return reg->dynptr.type == dynptr_type;
> > } else {
> > - spi = get_spi(reg->off);
> > + spi = dynptr_get_spi(env, reg);
> > + if (WARN_ON_ONCE(spi < 0))
> > + return false;
> > return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type;
> > }
> > }
> > @@ -2422,7 +2456,9 @@ static int mark_dynptr_read(struct bpf_verifier_env *env, struct bpf_reg_state *
> > */
> > if (reg->type == CONST_PTR_TO_DYNPTR)
> > return 0;
> > - spi = get_spi(reg->off);
> > + spi = dynptr_get_spi(env, reg);
> > + if (WARN_ON_ONCE(spi < 0))
> > + return spi;
> > /* Caller ensures dynptr is valid and initialized, which means spi is in
> > * bounds and spi is the first dynptr slot. Simply mark stack slot as
> > * read.
> > @@ -5946,6 +5982,11 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno,
> > return 0;
> > }
> >
> > +static bool arg_type_is_release(enum bpf_arg_type type)
> > +{
> > + return type & OBJ_RELEASE;
> > +}
>
> nit: I dont think you need this arg_type_is_release() change
>
Ack.
> > +
> > /* There are two register types representing a bpf_dynptr, one is PTR_TO_STACK
> > * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR.
> > *
> > @@ -5986,12 +6027,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno,
> > }
> > /* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to
> > * check_func_arg_reg_off's logic. We only need to check offset
> > - * alignment for PTR_TO_STACK.
> > + * and its alignment for PTR_TO_STACK.
> > */
> > - if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) {
> > - verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off);
> > - return -EINVAL;
>
> > + if (reg->type == PTR_TO_STACK) {
> > + err = dynptr_get_spi(env, reg);
> > + if (err < 0)
> > + return err;
> > }
>
> nit: if we do something like
>
> If (reg->type == PTR_TO_STACK) {
> spi = dynptr_get_spi(env, reg);
> if (spi < 0)
> return spi;
> } else {
> spi = __get_spi(reg->off);
> }
>
> then we can just pass in spi to is_dynptr_reg_valid_uninit() and
> is_dynptr_reg_valid_init() instead of having to recompute/check them
> again
>
Seems a little misleading to set it to something in the else branch (where stack
pointer index has no meaning), but I do see your point, I guess it can be
ignored for the other case and set to 0 by default.
> [...]
next prev parent reply other threads:[~2023-01-09 11:21 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-01 8:33 [PATCH bpf-next v1 0/8] Dynptr fixes Kumar Kartikeya Dwivedi
2023-01-01 8:33 ` [PATCH bpf-next v1 1/8] bpf: Fix state pruning for STACK_DYNPTR stack slots Kumar Kartikeya Dwivedi
2023-01-02 19:28 ` Eduard Zingerman
2023-01-09 10:59 ` Kumar Kartikeya Dwivedi
2023-01-04 22:24 ` Andrii Nakryiko
2023-01-09 11:05 ` Kumar Kartikeya Dwivedi
2023-01-12 0:47 ` Andrii Nakryiko
2023-01-06 0:18 ` Joanne Koong
2023-01-09 11:17 ` Kumar Kartikeya Dwivedi
2023-01-01 8:33 ` [PATCH bpf-next v1 2/8] bpf: Fix missing var_off check for ARG_PTR_TO_DYNPTR Kumar Kartikeya Dwivedi
2023-01-04 22:32 ` Andrii Nakryiko
2023-01-09 11:18 ` Kumar Kartikeya Dwivedi
2023-01-06 0:57 ` Joanne Koong
2023-01-06 17:56 ` Joanne Koong
2023-01-09 11:21 ` Kumar Kartikeya Dwivedi [this message]
2023-01-01 8:33 ` [PATCH bpf-next v1 3/8] bpf: Fix partial dynptr stack slot reads/writes Kumar Kartikeya Dwivedi
2023-01-04 22:42 ` Andrii Nakryiko
2023-01-09 11:26 ` Kumar Kartikeya Dwivedi
2023-01-05 3:06 ` Alexei Starovoitov
2023-01-09 11:52 ` Kumar Kartikeya Dwivedi
2023-01-10 2:19 ` Alexei Starovoitov
2023-01-06 19:16 ` Joanne Koong
2023-01-06 19:31 ` Joanne Koong
2023-01-09 11:30 ` Kumar Kartikeya Dwivedi
2023-01-12 18:51 ` Joanne Koong
2023-01-01 8:33 ` [PATCH bpf-next v1 4/8] bpf: Allow reinitializing unreferenced dynptr stack slots Kumar Kartikeya Dwivedi
2023-01-04 22:44 ` Andrii Nakryiko
2023-01-06 19:33 ` Joanne Koong
2023-01-09 11:40 ` Kumar Kartikeya Dwivedi
2023-01-01 8:33 ` [PATCH bpf-next v1 5/8] selftests/bpf: Add dynptr pruning tests Kumar Kartikeya Dwivedi
2023-01-04 22:49 ` Andrii Nakryiko
2023-01-09 11:44 ` Kumar Kartikeya Dwivedi
2023-01-01 8:34 ` [PATCH bpf-next v1 6/8] selftests/bpf: Add dynptr var_off tests Kumar Kartikeya Dwivedi
2023-01-01 8:34 ` [PATCH bpf-next v1 7/8] selftests/bpf: Add dynptr partial slot overwrite tests Kumar Kartikeya Dwivedi
2023-01-01 8:34 ` [PATCH bpf-next v1 8/8] selftests/bpf: Add dynptr helper tests Kumar Kartikeya Dwivedi
2023-01-04 22:51 ` [PATCH bpf-next v1 0/8] Dynptr fixes Andrii Nakryiko
2023-01-12 1:08 ` Kumar Kartikeya Dwivedi
2023-01-13 22:31 ` Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230109112124.ihkrsf6cjcxyhite@apollo \
--to=memxor@gmail.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=joannelkoong@gmail.com \
--cc=martin.lau@kernel.org \
--cc=void@manifault.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox