| 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
| ||
|
Message-ID: <CALx6S36dnA_vXBJVpLTZc6WyZaDh_hnw4vpp57EC2jsfA0vHcQ@mail.gmail.com>
Date: Sat, 9 Jul 2016 08:47:52 -0500
From: Tom Herbert <tom@...bertland.com>
To: Jesper Dangaard Brouer <brouer@...hat.com>
Cc: Brenden Blanco <bblanco@...mgrid.com>,
"David S. Miller" <davem@...emloft.net>,
Linux Kernel Network Developers <netdev@...r.kernel.org>,
Martin KaFai Lau <kafai@...com>, Ari Saha <as754m@....com>,
Alexei Starovoitov <alexei.starovoitov@...il.com>,
Or Gerlitz <gerlitz.or@...il.com>,
john fastabend <john.fastabend@...il.com>,
Hannes Frederic Sowa <hannes@...essinduktion.org>,
Thomas Graf <tgraf@...g.ch>,
Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH v6 01/12] bpf: add XDP prog type for early driver filter
On Sat, Jul 9, 2016 at 3:14 AM, Jesper Dangaard Brouer
<brouer@...hat.com> wrote:
> On Thu, 7 Jul 2016 19:15:13 -0700
> Brenden Blanco <bblanco@...mgrid.com> wrote:
>
>> Add a new bpf prog type that is intended to run in early stages of the
>> packet rx path. Only minimal packet metadata will be available, hence a
>> new context type, struct xdp_md, is exposed to userspace. So far only
>> expose the packet start and end pointers, and only in read mode.
>>
>> An XDP program must return one of the well known enum values, all other
>> return codes are reserved for future use. Unfortunately, this
>> restriction is hard to enforce at verification time, so take the
>> approach of warning at runtime when such programs are encountered. The
>> driver can choose to implement unknown return codes however it wants,
>> but must invoke the warning helper with the action value.
>
> I believe we should define a stronger semantics for unknown/future
> return codes than the once stated above:
> "driver can choose to implement unknown return codes however it wants"
>
> The mlx4 driver implementation in:
> [PATCH v6 04/12] net/mlx4_en: add support for fast rx drop bpf program
>
> On Thu, 7 Jul 2016 19:15:16 -0700 Brenden Blanco <bblanco@...mgrid.com> wrote:
>
>> + /* A bpf program gets first chance to drop the packet. It may
>> + * read bytes but not past the end of the frag.
>> + */
>> + if (prog) {
>> + struct xdp_buff xdp;
>> + dma_addr_t dma;
>> + u32 act;
>> +
>> + dma = be64_to_cpu(rx_desc->data[0].addr);
>> + dma_sync_single_for_cpu(priv->ddev, dma,
>> + priv->frag_info[0].frag_size,
>> + DMA_FROM_DEVICE);
>> +
>> + xdp.data = page_address(frags[0].page) +
>> + frags[0].page_offset;
>> + xdp.data_end = xdp.data + length;
>> +
>> + act = bpf_prog_run_xdp(prog, &xdp);
>> + switch (act) {
>> + case XDP_PASS:
>> + break;
>> + default:
>> + bpf_warn_invalid_xdp_action(act);
>> + case XDP_DROP:
>> + goto next;
>> + }
>> + }
>
> Thus, mlx4 choice is to drop packets for unknown/future return codes.
>
> I think this is the wrong choice. I think the choice should be
> XDP_PASS, to pass the packet up the stack.
>
> I find "XDP_DROP" problematic because it happen so early in the driver,
> that we lost all possibilities to debug what packets gets dropped. We
> get a single kernel log warning, but we cannot inspect the packets any
> longer. By defaulting to XDP_PASS all the normal stack tools (e.g.
> tcpdump) is available.
>
It's an API issue though not a problem with the packet. Allowing
unknown return codes to pass seems like a major security problem also.
Tom
>
> I can also imagine that, defaulting to XDP_PASS, can be an important
> feature in the future.
>
> In the future we will likely have features, where XDP can "offload"
> packet delivery from the normal stack (e.g. delivery into a VM). On a
> running production system you can then load your XDP program. If the
> driver was too old defaulting to XDP_DROP, then you lost your service,
> instead if defaulting to XDP_PASS, your service would survive, falling
> back to normal delivery.
>
> (For the VM delivery use-case, there will likely be a need for having a
> fallback delivery method in place, when the XDP program is not active,
> in-order to support VM migration).
>
>
>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index c14ca1c..5b47ac3 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
> [...]
>>
>> +/* User return codes for XDP prog type.
>> + * A valid XDP program must return one of these defined values. All other
>> + * return codes are reserved for future use. Unknown return codes will result
>> + * in driver-dependent behavior.
>> + */
>> +enum xdp_action {
>> + XDP_DROP,
>> + XDP_PASS,
>> +};
>> +
> [...]
>> #endif /* _UAPI__LINUX_BPF_H__ */
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index e206c21..a8d67d0 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
> [...]
>> +void bpf_warn_invalid_xdp_action(int act)
>> +{
>> + WARN_ONCE(1, "\n"
>> + "*****************************************************\n"
>> + "** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n"
>> + "** **\n"
>> + "** XDP program returned unknown value %-10u **\n"
>> + "** **\n"
>> + "** XDP programs must return a well-known return **\n"
>> + "** value. Invalid return values will result in **\n"
>> + "** undefined packet actions. **\n"
>> + "** **\n"
>> + "** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **\n"
>> + "*****************************************************\n",
>> + act);
>> +}
>> +EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
>> +
>
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> Author of http://www.iptv-analyzer.org
> LinkedIn: http://www.linkedin.com/in/brouer
Powered by blists - more mailing lists