Re: [PATCH net-next v3 2/3] virtio_net: add optional flow dissection in virtio_net_hdr_to_skb

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Cc: Tanner Love <tannerlove.kernel@gmail.com>,
	Network Development <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Eric Dumazet <edumazet@google.com>,
	Petar Penkov <ppenkov@google.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Tanner Love <tannerlove@google.com>,
	kernel test robot <lkp@intel.com>
Subject: Re: [PATCH net-next v3 2/3] virtio_net: add optional flow dissection in virtio_net_hdr_to_skb
Date: Thu, 3 Jun 2021 19:04:23 -0700	[thread overview]
Message-ID: <20210604020423.kfagqwwwf4k4t663@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <CA+FuTSfXXQWi5GZQYN=E+qpaa7jPii1jgvJPeTSYuXOzZkQFog@mail.gmail.com>

On Thu, Jun 03, 2021 at 08:44:51PM -0400, Willem de Bruijn wrote:
> On Thu, Jun 3, 2021 at 7:56 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Jun 01, 2021 at 06:18:39PM -0400, Tanner Love wrote:
> > > From: Tanner Love <tannerlove@google.com>
> > >
> > > Syzkaller bugs have resulted from loose specification of
> > > virtio_net_hdr[1]. Enable execution of a BPF flow dissector program
> > > in virtio_net_hdr_to_skb to validate the vnet header and drop bad
> > > input.
> > >
> > > The existing behavior of accepting these vnet headers is part of the
> > > ABI.
> >
> > So ?
> > It's ok to fix ABI when it's broken.
> > The whole feature is a way to workaround broken ABI with additional
> > BPF based validation.
> > It's certainly a novel idea.
> > I've never seen BPF being used to fix the kernel bugs.
> > But I think the better way forward is to admit that vnet ABI is broken
> > and fix it in the kernel with proper validation.
> > BPF-based validation is a band-aid. The out of the box kernel will
> > stay broken and syzbot will continue to crash it.
> 
> The intention is not to use this to avoid kernel fixes.
> 
> There are three parts:
> 
> 1. is the ABI broken and can we simply drop certain weird packets?
> 2. will we accept an extra packet parsing stage in
> virtio_net_hdr_to_skb to find all the culprits?
> 3. do we want to add yet another parser in C when this is exactly what
> the BPF flow dissector does well?
> 
> The virtio_net_hdr interface is more permissive than I think it was
> intended. But weird edge cases like protocol 0 do occur. So I believe
> it is simply too late to drop everything that is not obviously
> correct.
> 
> More problematic is that some of the gray area cases are non-trivial
> and require protocol parsing. Do the packet headers match the gso
> type? Are subtle variants like IPv6/TCP with extension headers
> supported or not?
> 
> We have previously tried to add unconditional protocol parsing in
> virtio_net_hdr_to_skb, but received pushback because this will cause
> performance regressions, e.g., for vm-to-vm forwarding.
> 
> Combined, that's why I think BPF flow dissection is the right approach
> here. It is optional, can be as pedantic as the admin wants / workload
> allows (e.g., dropping UFO altogether) and ensures protocol parsing
> itself is robust. Even if not enabled continuously, offers a quick way
> to patch a vulnerability when it is discovered. This is the same
> reasoning of the existing BPF flow dissector.
> 
> It is not intended as an alternative to subsequently fixing a bug in
> the kernel path.

Thanks for these details. They need to be part of commit log.

As far as patches the overhead of copying extra fields can be avoided.
How about copying single pointer to struct virtio_net_hdr into bpf_flow_keys instead?
See struct bpf_sockopt and _bpf_md_ptr(struct bpf_sock *, sk).
The overhead will much lower and bpf prog will access all virtio_net_hdr directly.