netdev - Re: VirtioNet L3 protocol patch advice request.

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: <CA+FuTSfytchd3Fk7=VB-6mTHsdjEjkEEHUFXRg_8ZaZkAyxbrg@mail.gmail.com>
Date:   Fri, 29 Oct 2021 10:19:06 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Andrew Melnichenko <andrew@...nix.com>
Cc:     davem@...emloft.net, bnemeth@...hat.com,
        gregkh@...uxfoundation.org, Yan Vugenfirer <yan@...nix.com>,
        Yuri Benditovich <yuri.benditovich@...nix.com>,
        netdev@...r.kernel.org
Subject: Re: VirtioNet L3 protocol patch advice request.

On Fri, Oct 29, 2021 at 6:51 AM Andrew Melnichenko <andrew@...nix.com> wrote:
>
> Hi all,
> Recently I've discovered a patch that added an additional check for the
> protocol in VirtioNet.
> (https://www.spinics.net/lists/kernel/msg3866319.html)
> Currently, that patch breaks UFOv6 support and possible USOv6 support in
> upcoming patches.
> The issue is the code next to the patch expects failure of
> skb_flow_dissect_flow_keys_basic()
> for IPv6 packets to retry it with protocol IPv6.
> I'm not sure about the goals of the patch

A well behaved configuration should not enter that code path to begin
with. GSO packets should also request NEEDS_CSUM, and in normal cases
skb->protocol is set. But packet sockets allow leaving skb->protocol
0, in which case this code tries to infer the protocol from the link
layer header if present and supported, using
dev_parse_header_protocol.

Commit 924a9bc362a5 ("net: check if protocol extracted by
virtio_net_hdr_set_proto is correct") added the
dev_parse_header_protocol check and will drop packets where the GSO
type (e.g., VIRTIO_NET_HDR_GSO_TCPV4) does not match the network
protocol as stores in the link layer header (ETH_P_IPV6, or even
something unrelated like ETH_P_ARP).

You're right that it can drop UFOv6 packets. VIRTIO_NET_HDR_GSO_UDP
has no separate V4 and V6 variants, so we have to accept both
protocols. We need to fix that.

This guess in virtio_net_hdr_set_proto

        case VIRTIO_NET_HDR_GSO_UDP:
                skb->protocol = cpu_to_be16(ETH_P_IP);

might be wrong to assume IPv4 for UFOv6, and then as of that commit
this check will incorrectly drop the packet

                                virtio_net_hdr_set_proto(skb, hdr);
                                if (protocol && protocol != skb->protocol)
                                        return -EINVAL;

> and propose the next solution:
>
> static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,
> >                      const struct virtio_net_hdr *hdr)
> > {
> >     __be16 protocol;
> >
> >     protocol = dev_parse_header_protocol(skb);
> >     switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
> >     case VIRTIO_NET_HDR_GSO_TCPV4:
> >         skb->protocol = cpu_to_be16(ETH_P_IP);
> >         break;
> >     case VIRTIO_NET_HDR_GSO_TCPV6:
> >         skb->protocol = cpu_to_be16(ETH_P_IPV6);
> >         break;
> >     case VIRTIO_NET_HDR_GSO_UDP:
> >     case VIRTIO_NET_HDR_GSO_UDP_L4:

Please use diff to show your changes. Also do not mix bug fixes (that
go to net) with new features (that go to net-next).

> >         skb->protocol = protocol;

Not exactly, this would just remove the added verification.

We need something like

@@ -89,8 +92,13 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
                                __be16 protocol =
dev_parse_header_protocol(skb);

                                virtio_net_hdr_set_proto(skb, hdr);
-                               if (protocol && protocol != skb->protocol)
-                                       return -EINVAL;
+                               if (protocol && protocol != skb->protocol) {
+                                       if (gso_type ==
VIRTIO_NET_HDR_GSO_UDP &&
+                                           protocol == cpu_to_be16(ETH_P_IPV6))
+                                               skb->protocol = protocol;
+                                       else
+                                               return -EINVAL;
+                               }

But preferably less ugly. Your suggestion of moving the
dev_parse_header_protocol step into virtio_net_hdr_to_skb is cleaner.
But also executes this check in the two other callers that may not
need it. Need to double check whether that is correct.