Re: [PATCH bpf-next v3 06/13] bpf: Prevent escaping of kptr loaded from maps

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Tue, Mar 22, 2022 at 11:28:26AM IST, Andrii Nakryiko wrote:
> On Sun, Mar 20, 2022 at 8:55 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > While we can guarantee that even for unreferenced kptr, the object
> > pointer points to being freed etc. can be handled by the verifier's
> > exception handling (normal load patching to PROBE_MEM loads), we still
> > cannot allow the user to pass these pointers to BPF helpers and kfunc,
> > because the same exception handling won't be done for accesses inside
> > the kernel. The same is true if a referenced pointer is loaded using
> > normal load instruction. Since the reference is not guaranteed to be
> > held while the pointer is used, it must be marked as untrusted.
> >
> > Hence introduce a new type flag, PTR_UNTRUSTED, which is used to mark
> > all registers loading unreferenced and referenced kptr from BPF maps,
> > and ensure they can never escape the BPF program and into the kernel by
> > way of calling stable/unstable helpers.
> >
> > In check_ptr_to_btf_access, the !type_may_be_null check to reject type
> > flags is still correct, as apart from PTR_MAYBE_NULL, only MEM_USER,
> > MEM_PERCPU, and PTR_UNTRUSTED may be set for PTR_TO_BTF_ID. The first
> > two are checked inside the function and rejected using a proper error
> > message, but we still want to allow dereference of untrusted case.
> >
> > Also, we make sure to inherit PTR_UNTRUSTED when chain of pointers are
> > walked, so that this flag is never dropped once it has been set on a
> > PTR_TO_BTF_ID (i.e. trusted to untrusted transition can only be in one
> > direction).
> >
> > In convert_ctx_accesses, extend the switch case to consider untrusted
> > PTR_TO_BTF_ID in addition to normal PTR_TO_BTF_ID for PROBE_MEM
> > conversion for BPF_LDX.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf.h   | 10 +++++++++-
> >  kernel/bpf/verifier.c | 34 +++++++++++++++++++++++++++-------
> >  2 files changed, 36 insertions(+), 8 deletions(-)
> >
>
> [...]
>
> > -       if (reg->type != PTR_TO_BTF_ID && reg->type != PTR_TO_BTF_ID_OR_NULL)
> > -               goto bad_type;
> > +       if (off_desc->flags & BPF_MAP_VALUE_OFF_F_REF) {
> > +               if (reg->type != PTR_TO_BTF_ID &&
> > +                   reg->type != (PTR_TO_BTF_ID | PTR_MAYBE_NULL))
> > +                       goto bad_type;
> > +       } else { /* only unreferenced case accepts untrusted pointers */
> > +               if (reg->type != PTR_TO_BTF_ID &&
> > +                   reg->type != (PTR_TO_BTF_ID | PTR_MAYBE_NULL) &&
> > +                   reg->type != (PTR_TO_BTF_ID | PTR_UNTRUSTED) &&
> > +                   reg->type != (PTR_TO_BTF_ID | PTR_MAYBE_NULL | PTR_UNTRUSTED))
>
> use base_type(), Luke! ;)
>

Ack, will switch.

> > +                       goto bad_type;
> > +       }
> >
> >         if (!btf_is_kernel(reg->btf)) {
> >                 verbose(env, "R%d must point to kernel BTF\n", regno);
>
> [...]

--
Kartikeya