netdev - Re: [PATCHv3 bpf-next 04/12] bpf: Add PTR_TO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAOftzPhEKxb4G7ckuZWdSAXHgcVXy0U9Cko9CbT+0ERVAXD3bg@mail.gmail.com>
Date:   Tue, 2 Oct 2018 09:45:56 -0700
From:   Joe Stringer <joe@...d.net.nz>
To:     daniel@...earbox.net
Cc:     Joe Stringer <joe@...d.net.nz>, netdev <netdev@...r.kernel.org>,
        ast@...nel.org, john fastabend <john.fastabend@...il.com>,
        tgraf@...g.ch, Martin KaFai Lau <kafai@...com>,
        Nitin Hande <nitin.hande@...il.com>, mauricio.vasquez@...ito.it
Subject: Re: [PATCHv3 bpf-next 04/12] bpf: Add PTR_TO_SOCKET verifier type

On Fri, 28 Sep 2018 at 06:38, Daniel Borkmann <daniel@...earbox.net> wrote:
>
> On 09/28/2018 01:26 AM, Joe Stringer wrote:
> > Teach the verifier a little bit about a new type of pointer, a
> > PTR_TO_SOCKET. This pointer type is accessed from BPF through the
> > 'struct bpf_sock' structure.
> >
> > Signed-off-by: Joe Stringer <joe@...d.net.nz>
> [...]
> > +/* Return true if it's OK to have the same insn return a different type. */
> > +static bool reg_type_mismatch_ok(enum bpf_reg_type type)
> > +{
> > +     switch (type) {
> > +     case PTR_TO_CTX:
> > +     case PTR_TO_SOCKET:
> > +     case PTR_TO_SOCKET_OR_NULL:
> > +             return false;
> > +     default:
> > +             return true;
> > +     }
> > +}
> > +
> > +/* If an instruction was previously used with particular pointer types, then we
> > + * need to be careful to avoid cases such as the below, where it may be ok
> > + * for one branch accessing the pointer, but not ok for the other branch:
> > + *
> > + * R1 = sock_ptr
> > + * goto X;
> > + * ...
> > + * R1 = some_other_valid_ptr;
> > + * goto X;
> > + * ...
> > + * R2 = *(u32 *)(R1 + 0);
> > + */
> > +static bool reg_type_mismatch(enum bpf_reg_type src, enum bpf_reg_type prev)
> > +{
> > +     return src != prev && (!reg_type_mismatch_ok(src) ||
> > +                            !reg_type_mismatch_ok(prev));
> > +}
> > +
> >  static int do_check(struct bpf_verifier_env *env)
> >  {
> >       struct bpf_verifier_state *state;
> > @@ -4812,9 +4894,7 @@ static int do_check(struct bpf_verifier_env *env)
> >                                */
> >                               *prev_src_type = src_reg_type;
> >
> > -                     } else if (src_reg_type != *prev_src_type &&
> > -                                (src_reg_type == PTR_TO_CTX ||
> > -                                 *prev_src_type == PTR_TO_CTX)) {
> > +                     } else if (reg_type_mismatch(src_reg_type, *prev_src_type)) {
> >                               /* ABuser program is trying to use the same insn
> >                                * dst_reg = *(u32*) (src_reg + off)
> >                                * with different pointer types:
> > @@ -4859,9 +4939,7 @@ static int do_check(struct bpf_verifier_env *env)
> >
> >                       if (*prev_dst_type == NOT_INIT) {
> >                               *prev_dst_type = dst_reg_type;
> > -                     } else if (dst_reg_type != *prev_dst_type &&
> > -                                (dst_reg_type == PTR_TO_CTX ||
> > -                                 *prev_dst_type == PTR_TO_CTX)) {
> > +                     } else if (reg_type_mismatch(dst_reg_type, *prev_dst_type)) {
> >                               verbose(env, "same insn cannot be used with different pointers\n");
> >                               return -EINVAL;
>
> Can also be as follow-up later on, but it would be crucial to also have
> test_verifier tests on this logic here with mixing these pointer types
> from different branches (right now we only cover ctx there).

Thanks for the feedback. I've applied all of your suggestions.

Regarding these newer tests, I have added a few and will post that
with my next revision. Fortunately with the reference tracking it's
actually quite difficult to mix up the pointer types between socket
and another type, because if the type of the register is ambiguous
then you either end up leaking a reference or attempting to release
using a pointer to a non-socket. I've added tests for both of those
cases, along with attempts to read and write into offsets inside
ambiguous pointers which triggers most of these paths.