netdev - Re: [bpf-next RFC 1/3] flow_dissector: implements flow dissector BPF hook

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPhsuW6QJqQq6qT6eEQ6-ufF3PfNONmw7FOAEhaOmp-GSkYAaQ@mail.gmail.com>
Date:   Mon, 20 Aug 2018 09:04:55 -0700
From:   Song Liu <liu.song.a23@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Petar Penkov <ppenkov@...gle.com>,
        Petar Penkov <peterpenkov96@...il.com>,
        Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        simon.horman@...ronome.com, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [bpf-next RFC 1/3] flow_dissector: implements flow dissector BPF hook

On Mon, Aug 20, 2018 at 7:13 AM, Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
> On Mon, Aug 20, 2018 at 1:47 AM Song Liu <liu.song.a23@...il.com> wrote:
>>
>> On Thu, Aug 16, 2018 at 4:14 PM, Petar Penkov <ppenkov@...gle.com> wrote:
>> > On Thu, Aug 16, 2018 at 3:40 PM, Song Liu <liu.song.a23@...il.com> wrote:
>> >>
>> >> On Thu, Aug 16, 2018 at 9:44 AM, Petar Penkov <peterpenkov96@...il.com> wrote:
>> >> > From: Petar Penkov <ppenkov@...gle.com>
>> >> >
>> >> > Adds a hook for programs of type BPF_PROG_TYPE_FLOW_DISSECTOR and
>> >> > attach type BPF_FLOW_DISSECTOR that is executed in the flow dissector
>> >> > path. The BPF program is kept as a global variable so it is
>> >> > accessible to all flow dissectors.
>> >> >
>> >> > Signed-off-by: Petar Penkov <ppenkov@...gle.com>
>> >> > Signed-off-by: Willem de Bruijn <willemb@...gle.com>
>
>> >> > @@ -658,6 +698,42 @@ bool __skb_flow_dissect(const struct sk_buff *skb,
>> >> >                                               FLOW_DISSECTOR_KEY_BASIC,
>> >> >                                               target_container);
>> >> >
>> >> > +       rcu_read_lock();
>> >> > +       attached = rcu_dereference(flow_dissector_prog);
>> >> > +       if (attached) {
>> >> > +               /* Note that even though the const qualifier is discarded
>> >> > +                * throughout the execution of the BPF program, all changes(the
>> >> > +                * control block) are reverted after the BPF program returns.
>> >> > +                * Therefore, __skb_flow_dissect does not alter the skb.
>> >> > +                */
>> >> > +               struct bpf_flow_dissect_cb *cb;
>> >> > +               u8 cb_saved[BPF_SKB_CB_LEN];
>> >> > +               u32 result;
>> >> > +
>> >> > +               cb = (struct bpf_flow_dissect_cb *)(bpf_skb_cb((struct sk_buff *)skb));
>> >> > +
>> >> > +               /* Save Control Block */
>> >> > +               memcpy(cb_saved, cb, sizeof(cb_saved));
>> >> > +               memset(cb, 0, sizeof(cb_saved));
>> >> > +
>> >> > +               /* Pass parameters to the BPF program */
>> >> > +               cb->nhoff = nhoff;
>> >> > +               cb->target_container = target_container;
>> >> > +               cb->flow_dissector = flow_dissector;
>> >> > +
>> >> > +               bpf_compute_data_pointers((struct sk_buff *)skb);
>> >> > +               result = BPF_PROG_RUN(attached, skb);
>> >> > +
>> >> > +               /* Restore state */
>> >> > +               memcpy(cb, cb_saved, sizeof(cb_saved));
>> >> > +
>> >> > +               key_control->thoff = min_t(u16, key_control->thoff,
>> >> > +                                          skb ? skb->len : hlen);
>> >> > +               rcu_read_unlock();
>> >> > +               return result == BPF_OK;
>> >> > +       }
>> >>
>> >> If the BPF program cannot handle certain protocol, shall we fall back
>> >> to the built-in logic? Otherwise, all BPF programs need to have some
>> >> code for all protocols.
>> >>
>> >> Song
>> >
>> > I believe that if we fall back to the built-in logic we lose all security
>> > guarantees from BPF and this is why the code does not support
>> > fall back.
>> >
>> > Petar
>>
>> I am not really sure we are on the same page. I am proposing 3
>> different return values from BPF_PROG_RUN(), and they should be
>> handled as
>>
>> 1. result == BPF_OK                      => return true;
>> 2. result == BPF_DROP                 => return false;
>> 3. result == something else            => fall back.
>>
>> Does this proposal make any sense?
>>
>> Thanks,
>> Song
>
> It certainly makes sense. We debated it initially, as well.
>
> In the short term, it allows for simpler BPF programs, as they can
> off-load some protocols to the C implementation.
>
> But the RFC patchset already implements most protocols in BPF.
> I had not expected that when we started out.
>
> Eventually, I think it is preferable to just deprecate the C
> implementation. Which is not possible if we make this opt-out
> a part of the BPF flow dissector interface.
>
> There is also the lesser issue that a buggy BPF program might
> accidentally pass the third value and unknowing open itself up
> to the large attack surface. Without this option, the security
> audit is much simpler.

Thanks for the explanation. I didn't realize that the end goal is to
deprecate the C implementation.

Acked-by: Song Liu <songliubraving@...com>