| 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
| ||
|
Date: Sun, 31 Oct 2021 16:49:29 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: Andrew Melnichenko <andrew@...nix.com>, 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 10:19 AM Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
>
> 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.
If the protocol can be inferred from ll_type, that should take
precedence over inferring from gso_type.
For packet sockets, tpacket_snd does call dev_parse_header_protocol
before virtio_net_hdr_to_skb. But packet_snd calls it after.
Does the following solve your bug?
"
@@ -3001,6 +3001,8 @@ static int packet_snd(struct socket *sock,
struct msghdr *msg, size_t len)
skb->mark = sockc.mark;
skb->tstamp = sockc.transmit_time;
+ packet_parse_headers(skb, sock);
+
if (has_vnet_hdr) {
err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
if (err)
@@ -3009,8 +3011,6 @@ static int packet_snd(struct socket *sock,
struct msghdr *msg, size_t len)
virtio_net_hdr_set_proto(skb, &vnet_hdr);
}
- packet_parse_headers(skb, sock);
-
"
If the protocol is set, virtio_net_hdr_to_skb should not try to
overwrite it, but check that the values match:
+static inline bool virtio_net_hdr_check_gso_proto(struct sk_buff *skb,
+ const struct
virtio_net_hdr *hdr)
+{
+ u8 gso_type = hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN;
+
+ switch (skb->protocol) {
+ case htons(ETH_P_IP):
+ return (gso_type == VIRTIO_NET_HDR_GSO_TCPV4 ||
+ gso_type == VIRTIO_NET_HDR_GSO_UDP);
+ case htons(ETH_P_IPV6):
+ return (gso_type == VIRTIO_NET_HDR_GSO_TCPV6 ||
+ gso_type == VIRTIO_NET_HDR_GSO_UDP);
+ default:
+ return false;
+ }
+}
This can be called on any packet entering virtio_net_hdr_to_skb.
But such larger change should go to net-next.
Powered by blists - more mailing lists