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

On Thu, Jun 3, 2021 at 7:56 PM Alexei Starovoitov
[off-list ref] wrote:

On Tue, Jun 01, 2021 at 06:18:39PM -0400, Tanner Love wrote:

quoted

From: Tanner Love <redacted>

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.