[PATCH net] net: validate untrusted gso packets

[PATCH net] net: validate untrusted gso packets

[Willem de Bruijn]

From: Willem de Bruijn <willemb@google.com>

Validate gso packet type and headers on kernel entry. Reuse the info
gathered by skb_probe_transport_header.

Syzbot found two bugs by passing bad gso packets in packet sockets.
Untrusted user packets are limited to a small set of gso types in
virtio_net_hdr_to_skb. But segmentation occurs on packet contents.
Syzkaller was able to enter gso callbacks that are not hardened
against untrusted user input.

User packets can also have corrupted headers, tripping up segmentation
logic that expects sane packets from the trusted protocol stack.
Hardening all segmentation paths against all bad packets is error
prone and slows down the common path, so validate on kernel entry.

Introduce skb_probe_transport_header_hard to unconditionally probe,
even if skb_partial_csum_set has already set an offset. That is under
user control, so do not trust it. I did not see a measurable change
in TCP_STREAM performance.

Move tpacket probe call after virtio_net_hdr_to_skb has set gso_type.

Fixes: bfd5f4a3d605 ("packet: Add GSO/csum offload support.")
Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
Fixes: f942dc2552b8 ("xen network backend driver")
Link: http://lkml.kernel.org/r/<001a1137452496ffc305617e5fe0@google.com>
Reported-by: syzbot+fee64147a25aecd48055@syzkaller.appspotmail.com
Signed-off-by: Willem de Bruijn <willemb@google.com>

---

I can resubmit as separate patches for packet, virtio, xen. That
is cleaner and easier to review, but more work to backport, I figured.

An alternative fix is to narrowly address the two specific bugs
syzkaller found. The link above sketches a check in inet_gso_segment
for gso_type.
---
 drivers/net/tap.c                 |  5 ++++-
 drivers/net/tun.c                 | 24 +++++++++++----------
 drivers/net/xen-netback/netback.c |  5 ++---
 include/linux/skbuff.h            | 44 +++++++++++++++++++++++++++++++++------
 net/packet/af_packet.c            | 12 ++++++++---
 5 files changed, 66 insertions(+), 24 deletions(-)

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 0a886fda0129..b0b7dab1d4a8 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -715,7 +715,10 @@ static ssize_t tap_get_user(struct tap_queue *q, struct msghdr *m,
 			goto err_kfree;
 	}
 
-	skb_probe_transport_header(skb, ETH_HLEN);
+	if (!skb_probe_transport_header_hard(skb, ETH_HLEN)) {
+		err = -EINVAL;
+		goto err;
+	}
 
 	/* Move network header to the right position for VLAN tagged packets */
 	if ((skb->protocol == htons(ETH_P_8021Q) ||
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 4f4a842a1c9c..68a0fc55c723 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1670,16 +1670,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 		}
 	}
 
-	if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun))) {
-		this_cpu_inc(tun->pcpu_stats->rx_frame_errors);
-		kfree_skb(skb);
-		if (frags) {
-			tfile->napi.skb = NULL;
-			mutex_unlock(&tfile->napi_mutex);
-		}
-
-		return -EINVAL;
-	}
+	if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun)))
+		goto frame_error;
 
 	switch (tun->flags & TUN_TYPE_MASK) {
 	case IFF_TUN:
@@ -1721,7 +1713,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	}
 
 	skb_reset_network_header(skb);
-	skb_probe_transport_header(skb, 0);
+	if (!skb_probe_transport_header_hard(skb, 0))
+		goto frame_error;
 
 	if (skb_xdp) {
 		struct bpf_prog *xdp_prog;
@@ -1785,6 +1778,15 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 
 	tun_flow_update(tun, rxhash, tfile);
 	return total_len;
+
+frame_error:
+	this_cpu_inc(tun->pcpu_stats->rx_frame_errors);
+	kfree_skb(skb);
+	if (frags) {
+		tfile->napi.skb = NULL;
+		mutex_unlock(&tfile->napi_mutex);
+	}
+	return -EINVAL;
 }
 
 static ssize_t tun_chr_write_iter(struct kiocb *iocb, struct iov_iter *from)
diff --git a/drivers/net/xen-netback/netback.c b/drivers/net/xen-netback/netback.c
index a27daa23c9dc..ae59a96bc11b 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1159,7 +1159,8 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
 		skb->protocol = eth_type_trans(skb, skb->dev);
 		skb_reset_network_header(skb);
 
-		if (checksum_setup(queue, skb)) {
+		if (checksum_setup(queue, skb) ||
+		    !skb_probe_transport_header_hard(skb, 0)) {
 			netdev_dbg(queue->vif->dev,
 				   "Can't setup checksum in net_tx_action\n");
 			/* We have to set this flag to trigger the callback */
@@ -1169,8 +1170,6 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
 			continue;
 		}
 
-		skb_probe_transport_header(skb, 0);
-
 		/* If the packet is GSO then we will have just set up the
 		 * transport header offset in checksum_setup so it's now
 		 * straightforward to calculate gso_segs.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a38c80e9f91e..0d931feb236f 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -37,6 +37,7 @@
 #include <linux/sched/clock.h>
 #include <net/flow_dissector.h>
 #include <linux/splice.h>
+#include <linux/in.h>
 #include <linux/in6.h>
 #include <linux/if_packet.h>
 #include <net/flow.h>
@@ -2335,17 +2336,48 @@ static inline void skb_pop_mac_header(struct sk_buff *skb)
 	skb->mac_header = skb->network_header;
 }
 
-static inline void skb_probe_transport_header(struct sk_buff *skb,
-					      const int offset_hint)
+static inline bool skb_validate_dodgy_gso(struct sk_buff *skb,
+					  struct flow_keys *keys)
+{
+	switch (skb_shinfo(skb)->gso_type) {
+	case 0:
+		return true;
+	case SKB_GSO_TCPV4 | SKB_GSO_DODGY:
+	case SKB_GSO_TCPV4 | SKB_GSO_DODGY | SKB_GSO_TCP_ECN:
+		return keys->basic.n_proto == htons(ETH_P_IP) &&
+		       keys->basic.ip_proto == IPPROTO_TCP;
+	case SKB_GSO_TCPV6 | SKB_GSO_DODGY:
+	case SKB_GSO_TCPV6 | SKB_GSO_DODGY | SKB_GSO_TCP_ECN:
+		return keys->basic.n_proto == htons(ETH_P_IPV6) &&
+		       keys->basic.ip_proto == IPPROTO_TCP;
+	case SKB_GSO_UDP | SKB_GSO_DODGY:
+		return keys->basic.ip_proto == IPPROTO_UDP;
+	default:
+		return false;
+	}
+}
+
+static inline bool skb_probe_transport_header_hard(struct sk_buff *skb,
+						   const int offset_hint)
 {
 	struct flow_keys keys;
 
-	if (skb_transport_header_was_set(skb))
-		return;
-	else if (skb_flow_dissect_flow_keys(skb, &keys, 0))
+	if (skb_flow_dissect_flow_keys(skb, &keys, 0)) {
 		skb_set_transport_header(skb, keys.control.thoff);
-	else
+		return skb_validate_dodgy_gso(skb, &keys);
+	} else {
 		skb_set_transport_header(skb, offset_hint);
+		return !skb_shinfo(skb)->gso_type;
+	}
+}
+
+static inline void skb_probe_transport_header(struct sk_buff *skb,
+					      const int offset_hint)
+{
+	if (skb_transport_header_was_set(skb))
+		return;
+
+	skb_probe_transport_header_hard(skb, offset_hint);
 }
 
 static inline void skb_mac_header_rebuild(struct sk_buff *skb)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index da215e5c1399..e30b8c43d3bc 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2542,8 +2542,6 @@ static int tpacket_fill_skb(struct packet_sock *po, struct sk_buff *skb,
 		len = ((to_write > len_max) ? len_max : to_write);
 	}
 
-	skb_probe_transport_header(skb, 0);
-
 	return tp_len;
 }
 
@@ -2744,6 +2742,11 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
 			goto tpacket_error;
 		}
 
+		if (!skb_probe_transport_header_hard(skb, 0)) {
+			tp_len = -EINVAL;
+			goto tpacket_error;
+		}
+
 		skb->destructor = tpacket_destruct_skb;
 		__packet_set_status(po, ph, TP_STATUS_SENDING);
 		packet_inc_pending(&po->tx_ring);
@@ -2935,7 +2938,10 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
 		len += sizeof(vnet_hdr);
 	}
 
-	skb_probe_transport_header(skb, reserve);
+	if (!skb_probe_transport_header_hard(skb, reserve)) {
+		err = -EINVAL;
+		goto out_free;
+	}
 
 	if (unlikely(extra_len == 4))
 		skb->no_fcs = 1;
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH net] net: validate untrusted gso packets

[Jason Wang]

On 2018年01月17日 04:29, Willem de Bruijn wrote:
> From: Willem de Bruijn<willemb@google.com>
>
> Validate gso packet type and headers on kernel entry. Reuse the info
> gathered by skb_probe_transport_header.
>
> Syzbot found two bugs by passing bad gso packets in packet sockets.
> Untrusted user packets are limited to a small set of gso types in
> virtio_net_hdr_to_skb. But segmentation occurs on packet contents.
> Syzkaller was able to enter gso callbacks that are not hardened
> against untrusted user input.

Do this mean there's something missed in exist header check for dodgy 
packets?

>
> User packets can also have corrupted headers, tripping up segmentation
> logic that expects sane packets from the trusted protocol stack.
> Hardening all segmentation paths against all bad packets is error
> prone and slows down the common path, so validate on kernel entry.

I think evil packets should be rare in common case, so I'm not sure 
validate it on kernel entry is a good choice especially consider we've 
already had header check.

>
> Introduce skb_probe_transport_header_hard to unconditionally probe,
> even if skb_partial_csum_set has already set an offset. That is under
> user control, so do not trust it. I did not see a measurable change
> in TCP_STREAM performance.
>
> Move tpacket probe call after virtio_net_hdr_to_skb has set gso_type.
>
> Fixes: bfd5f4a3d605 ("packet: Add GSO/csum offload support.")
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> Fixes: f942dc2552b8 ("xen network backend driver")
> Link:http://lkml.kernel.org/r/<001a1137452496ffc305617e5fe0@google.com>
> Reported-by:syzbot+fee64147a25aecd48055@syzkaller.appspotmail.com
> Signed-off-by: Willem de Bruijn<willemb@google.com>

...

Thanks

Re: [PATCH net] net: validate untrusted gso packets

[Willem de Bruijn]

On Tue, Jan 16, 2018 at 11:04 PM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2018年01月17日 04:29, Willem de Bruijn wrote:
>>
>> From: Willem de Bruijn<willemb@google.com>
>>
>> Validate gso packet type and headers on kernel entry. Reuse the info
>> gathered by skb_probe_transport_header.
>>
>> Syzbot found two bugs by passing bad gso packets in packet sockets.
>> Untrusted user packets are limited to a small set of gso types in
>> virtio_net_hdr_to_skb. But segmentation occurs on packet contents.
>> Syzkaller was able to enter gso callbacks that are not hardened
>> against untrusted user input.
>
>
> Do this mean there's something missed in exist header check for dodgy
> packets?

virtio_net_hdr_to_skb checks gso_type, but it does not verify that this
type correctly describes the actual packet. Segmentation happens based
on packet contents. So a packet was crafted to enter sctp gso, even
though no such gso_type exists. This issue is not specific to sctp.

>>
>> User packets can also have corrupted headers, tripping up segmentation
>> logic that expects sane packets from the trusted protocol stack.
>> Hardening all segmentation paths against all bad packets is error
>> prone and slows down the common path, so validate on kernel entry.
>
>
> I think evil packets should be rare in common case, so I'm not sure validate
> it on kernel entry is a good choice especially consider we've already had
> header check.

This just makes that check more strict. Frequency of malicious packets is
not really relevant if a single bad packet can cause damage.

The alternative to validate on kernel entry is to harden the entire segmentation
layer and lower part of the stack. That is much harder to get right and not
necessarily cheaper.

As a matter of fact, it incurs a cost on all packets, including the common
case generated by the protocol stack.

Re: [PATCH net] net: validate untrusted gso packets

[Willem de Bruijn]

On Tue, Jan 16, 2018 at 11:33 PM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Tue, Jan 16, 2018 at 11:04 PM, Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年01月17日 04:29, Willem de Bruijn wrote:
>>>
>>> From: Willem de Bruijn<willemb@google.com>
>>>
>>> Validate gso packet type and headers on kernel entry. Reuse the info
>>> gathered by skb_probe_transport_header.
>>>
>>> Syzbot found two bugs by passing bad gso packets in packet sockets.
>>> Untrusted user packets are limited to a small set of gso types in
>>> virtio_net_hdr_to_skb. But segmentation occurs on packet contents.
>>> Syzkaller was able to enter gso callbacks that are not hardened
>>> against untrusted user input.
>>
>>
>> Do this mean there's something missed in exist header check for dodgy
>> packets?
>
> virtio_net_hdr_to_skb checks gso_type, but it does not verify that this
> type correctly describes the actual packet. Segmentation happens based
> on packet contents. So a packet was crafted to enter sctp gso, even
> though no such gso_type exists. This issue is not specific to sctp.
>
>>>
>>> User packets can also have corrupted headers, tripping up segmentation
>>> logic that expects sane packets from the trusted protocol stack.
>>> Hardening all segmentation paths against all bad packets is error
>>> prone and slows down the common path, so validate on kernel entry.
>>
>>
>> I think evil packets should be rare in common case, so I'm not sure validate
>> it on kernel entry is a good choice especially consider we've already had
>> header check.
>
> This just makes that check more strict. Frequency of malicious packets is
> not really relevant if a single bad packet can cause damage.
>
> The alternative to validate on kernel entry is to harden the entire segmentation
> layer and lower part of the stack. That is much harder to get right and not
> necessarily cheaper.
>
> As a matter of fact, it incurs a cost on all packets, including the common
> case generated by the protocol stack.

If packets can be fully validated at the source, we can eventually also
get rid of the entire SKB_GSO_DODGY and NETIF_F_GSO_ROBUST
logic. Then virtio packets won't have to enter the segmentation layer
at all for TSO capable devices.

Re: [PATCH net] net: validate untrusted gso packets

[Jason Wang]

On 2018年01月17日 12:33, Willem de Bruijn wrote:
> On Tue, Jan 16, 2018 at 11:04 PM, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2018年01月17日 04:29, Willem de Bruijn wrote:
>>> From: Willem de Bruijn<willemb@google.com>
>>>
>>> Validate gso packet type and headers on kernel entry. Reuse the info
>>> gathered by skb_probe_transport_header.
>>>
>>> Syzbot found two bugs by passing bad gso packets in packet sockets.
>>> Untrusted user packets are limited to a small set of gso types in
>>> virtio_net_hdr_to_skb. But segmentation occurs on packet contents.
>>> Syzkaller was able to enter gso callbacks that are not hardened
>>> against untrusted user input.
>>
>> Do this mean there's something missed in exist header check for dodgy
>> packets?
> virtio_net_hdr_to_skb checks gso_type, but it does not verify that this
> type correctly describes the actual packet. Segmentation happens based
> on packet contents. So a packet was crafted to enter sctp gso, even
> though no such gso_type exists. This issue is not specific to sctp.

So it looks to me we should do it in here in sctp_gso_segment().

if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
         /* Packet is from an untrusted source, reset gso_segs. */


And we probably need to recover what has been removed since 
5c7cdf339af560f980b12eb6b0b5aa5f68ac6658 ("gso: Remove arbitrary checks 
for unsupported GSO").

>
>>> User packets can also have corrupted headers, tripping up segmentation
>>> logic that expects sane packets from the trusted protocol stack.
>>> Hardening all segmentation paths against all bad packets is error
>>> prone and slows down the common path, so validate on kernel entry.
>>
>> I think evil packets should be rare in common case, so I'm not sure validate
>> it on kernel entry is a good choice especially consider we've already had
>> header check.
> This just makes that check more strict. Frequency of malicious packets is
> not really relevant if a single bad packet can cause damage.

We try hard to avoid flow dissector since its overhead is obvious. But 
looks like this patch did it unconditionally, and even for non gso packet.

> The alternative to validate on kernel entry is to harden the entire segmentation
> layer and lower part of the stack. That is much harder to get right and not
> necessarily cheaper.

For performance reason. I think we should delay the check or 
segmentation as much as possible until it was really needed.

>
> As a matter of fact, it incurs a cost on all packets, including the common
> case generated by the protocol stack.

Btw, this looks could be triggered from guest. So it looks at least a 
DOS form guest to host which should be treated as CVE?

Thanks

Re: [PATCH net] net: validate untrusted gso packets

[Jason Wang]

On 2018年01月17日 12:56, Willem de Bruijn wrote:
>> This just makes that check more strict. Frequency of malicious packets is
>> not really relevant if a single bad packet can cause damage.
>>
>> The alternative to validate on kernel entry is to harden the entire segmentation
>> layer and lower part of the stack. That is much harder to get right and not
>> necessarily cheaper.
>>
>> As a matter of fact, it incurs a cost on all packets, including the common
>> case generated by the protocol stack.
> If packets can be fully validated at the source, we can eventually also
> get rid of the entire SKB_GSO_DODGY and NETIF_F_GSO_ROBUST
> logic. Then virtio packets won't have to enter the segmentation layer
> at all for TSO capable devices.

On the contrary I think, if I read the code correctly, the point is to 
delay the check as much as possible. Then for GSO_ROBUST device, we 
don't even need to do any header check at all. This help for 
performance. What's more, the check should be done layer by layer which 
makes gso_segment a perfect place to do that.

Thanks

Re: [PATCH net] net: validate untrusted gso packets

[Willem de Bruijn]

On Wed, Jan 17, 2018 at 6:54 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2018年01月17日 12:33, Willem de Bruijn wrote:
>>
>> On Tue, Jan 16, 2018 at 11:04 PM, Jason Wang <jasowang@redhat.com> wrote:
>>>
>>>
>>> On 2018年01月17日 04:29, Willem de Bruijn wrote:
>>>>
>>>> From: Willem de Bruijn<willemb@google.com>
>>>>
>>>> Validate gso packet type and headers on kernel entry. Reuse the info
>>>> gathered by skb_probe_transport_header.
>>>>
>>>> Syzbot found two bugs by passing bad gso packets in packet sockets.
>>>> Untrusted user packets are limited to a small set of gso types in
>>>> virtio_net_hdr_to_skb. But segmentation occurs on packet contents.
>>>> Syzkaller was able to enter gso callbacks that are not hardened
>>>> against untrusted user input.
>>>
>>>
>>> Do this mean there's something missed in exist header check for dodgy
>>> packets?
>>
>> virtio_net_hdr_to_skb checks gso_type, but it does not verify that this
>> type correctly describes the actual packet. Segmentation happens based
>> on packet contents. So a packet was crafted to enter sctp gso, even
>> though no such gso_type exists. This issue is not specific to sctp.
>
>
> So it looks to me we should do it in here in sctp_gso_segment().
>
> if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
>         /* Packet is from an untrusted source, reset gso_segs. */

No dodgy source can legitimately generate sctp code, so it should not
even get there. Also, a packet can just as easily spoof an esp packet.
See also the discussion in the Link above.

We can address this specific issue in segmentation by placing a check
analogous to skb_validate_dodgy_gso in the network layer. Something
like this (but with ECN option support):

@@ -1258,6 +1258,22 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,

        skb_reset_transport_header(skb);

+       gso_type = skb_shinfo(skb)->gso_type;
+       if (gso_type & SKB_GSO_DODGY) {
+               switch (gso_type & (SKB_GSO_TCPV4 | SKB_GSO_UDP)) {
+               case SKB_GSO_TCPV4:
+                       if (proto != IPPROTO_TCP)
+                               goto out;
+                       break;
+               case SKB_GSO_UDP:
+                       if (proto != IPPROTO_UDP)
+                               goto out;
+                       break;
+               default:
+                       goto out;
+               }
+       }

> And we probably need to recover what has been removed since
> 5c7cdf339af560f980b12eb6b0b5aa5f68ac6658 ("gso: Remove arbitrary checks for
> unsupported GSO").
>
>>
>>>> User packets can also have corrupted headers, tripping up segmentation
>>>> logic that expects sane packets from the trusted protocol stack.
>>>> Hardening all segmentation paths against all bad packets is error
>>>> prone and slows down the common path, so validate on kernel entry.
>>>
>>>
>>> I think evil packets should be rare in common case, so I'm not sure
>>> validate
>>> it on kernel entry is a good choice especially consider we've already had
>>> header check.
>>
>> This just makes that check more strict. Frequency of malicious packets is
>> not really relevant if a single bad packet can cause damage.
>
>
> We try hard to avoid flow dissector since its overhead is obvious. But looks
> like this patch did it unconditionally, and even for non gso packet.
>
>> The alternative to validate on kernel entry is to harden the entire
>> segmentation
>> layer and lower part of the stack. That is much harder to get right and
>> not
>> necessarily cheaper.
>
>
> For performance reason. I think we should delay the check or segmentation as
> much as possible until it was really needed.

Going through segmentation is probably as expensive as flow dissector,
if not more so because of the indirect branches. And now we have two
pieces of code that need to be hardened against malicious packets.

But I did overlook the guest to guest communication, with virtual devices
that can set NETIF_F_GSO_ROBUST and so do not enter the stack. That
means that one guest can send misrepresented packets to another. Is that
preferable to absorbing the cost of validation?

The current patch does not actually deprecate the path through the
segmentation layer. So it will indeed add overhead. We would have to
remove the DODGY logic in net-next. One additional possible optimization
is to switch to flow_keys_buf_dissector, which does not dissect as deeply.
I did that in an earlier patch; it should be sufficient for this purpose.

A third option is to apply the above inet_gso_segment fix in net, then
add flow dissection at the source in net-next together with removal of
dodgy checks in the stack. Then that can be benchmarked properly.

>>
>> As a matter of fact, it incurs a cost on all packets, including the common
>> case generated by the protocol stack.
>
>
> Btw, this looks could be triggered from guest. So it looks at least a DOS
> form guest to host which should be treated as CVE?

Good point. The report was public, so I focused on getting it fixed first.
You mean just to request a CVE? I'll do that that once we have a fix.

Re: [PATCH net] net: validate untrusted gso packets

[Willem de Bruijn]

>>>>> From: Willem de Bruijn<willemb@google.com>
>>>>>
>>>>> Validate gso packet type and headers on kernel entry. Reuse the info
>>>>> gathered by skb_probe_transport_header.
>>>>>
>>>>> Syzbot found two bugs by passing bad gso packets in packet sockets.
>>>>> Untrusted user packets are limited to a small set of gso types in
>>>>> virtio_net_hdr_to_skb. But segmentation occurs on packet contents.
>>>>> Syzkaller was able to enter gso callbacks that are not hardened
>>>>> against untrusted user input.
>>>>
>>>>
>>>> Do this mean there's something missed in exist header check for dodgy
>>>> packets?
>>>
>>> virtio_net_hdr_to_skb checks gso_type, but it does not verify that this
>>> type correctly describes the actual packet. Segmentation happens based
>>> on packet contents. So a packet was crafted to enter sctp gso, even
>>> though no such gso_type exists. This issue is not specific to sctp.
>>
>>
>> So it looks to me we should do it in here in sctp_gso_segment().
>>
>> if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
>>         /* Packet is from an untrusted source, reset gso_segs. */
>
> No dodgy source can legitimately generate sctp code, so it should not
> even get there. Also, a packet can just as easily spoof an esp packet.
> See also the discussion in the Link above.
>
> We can address this specific issue in segmentation by placing a check
> analogous to skb_validate_dodgy_gso in the network layer. Something
> like this (but with ECN option support):
>
> @@ -1258,6 +1258,22 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
>
>         skb_reset_transport_header(skb);
>
> +       gso_type = skb_shinfo(skb)->gso_type;
> +       if (gso_type & SKB_GSO_DODGY) {
> +               switch (gso_type & (SKB_GSO_TCPV4 | SKB_GSO_UDP)) {
> +               case SKB_GSO_TCPV4:
> +                       if (proto != IPPROTO_TCP)
> +                               goto out;
> +                       break;
> +               case SKB_GSO_UDP:
> +                       if (proto != IPPROTO_UDP)
> +                               goto out;
> +                       break;
> +               default:
> +                       goto out;
> +               }
> +       }

Okay, I sent this instead: http://patchwork.ozlabs.org/patch/862643/

Re: [PATCH net] net: validate untrusted gso packets

[Jason Wang]

On 2018年01月17日 22:27, Willem de Bruijn wrote:
> On Wed, Jan 17, 2018 at 6:54 AM, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2018年01月17日 12:33, Willem de Bruijn wrote:
>>> On Tue, Jan 16, 2018 at 11:04 PM, Jason Wang <jasowang@redhat.com> wrote:
>>>>
>>>> On 2018年01月17日 04:29, Willem de Bruijn wrote:
>>>>> From: Willem de Bruijn<willemb@google.com>
>>>>>
>>>>> Validate gso packet type and headers on kernel entry. Reuse the info
>>>>> gathered by skb_probe_transport_header.
>>>>>
>>>>> Syzbot found two bugs by passing bad gso packets in packet sockets.
>>>>> Untrusted user packets are limited to a small set of gso types in
>>>>> virtio_net_hdr_to_skb. But segmentation occurs on packet contents.
>>>>> Syzkaller was able to enter gso callbacks that are not hardened
>>>>> against untrusted user input.
>>>>
>>>> Do this mean there's something missed in exist header check for dodgy
>>>> packets?
>>> virtio_net_hdr_to_skb checks gso_type, but it does not verify that this
>>> type correctly describes the actual packet. Segmentation happens based
>>> on packet contents. So a packet was crafted to enter sctp gso, even
>>> though no such gso_type exists. This issue is not specific to sctp.
>>
>> So it looks to me we should do it in here in sctp_gso_segment().
>>
>> if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
>>          /* Packet is from an untrusted source, reset gso_segs. */
> No dodgy source can legitimately generate sctp code, so it should not
> even get there.

I think that's why we need extra check for dodgy packets here, we used 
to have gso_type verification here but was removed. In the future, 
virtio will support passing sctp offload packet down for sure, so we 
need consider this case.

>   Also, a packet can just as easily spoof an esp packet.
> See also the discussion in the Link above.

Then we probably need check there.

>
> We can address this specific issue in segmentation by placing a check
> analogous to skb_validate_dodgy_gso in the network layer. Something
> like this (but with ECN option support):
>
> @@ -1258,6 +1258,22 @@ struct sk_buff *inet_gso_segment(struct sk_buff *skb,
>
>          skb_reset_transport_header(skb);
>
> +       gso_type = skb_shinfo(skb)->gso_type;
> +       if (gso_type & SKB_GSO_DODGY) {
> +               switch (gso_type & (SKB_GSO_TCPV4 | SKB_GSO_UDP)) {
> +               case SKB_GSO_TCPV4:
> +                       if (proto != IPPROTO_TCP)
> +                               goto out;
> +                       break;
> +               case SKB_GSO_UDP:
> +                       if (proto != IPPROTO_UDP)
> +                               goto out;
> +                       break;
> +               default:
> +                       goto out;
> +               }
> +       }

This implements subset of function for codes which was removed by the 
commit I mentioned below.

>> And we probably need to recover what has been removed since
>> 5c7cdf339af560f980b12eb6b0b5aa5f68ac6658 ("gso: Remove arbitrary checks for
>> unsupported GSO").
>>
>>>>> User packets can also have corrupted headers, tripping up segmentation
>>>>> logic that expects sane packets from the trusted protocol stack.
>>>>> Hardening all segmentation paths against all bad packets is error
>>>>> prone and slows down the common path, so validate on kernel entry.
>>>>
>>>> I think evil packets should be rare in common case, so I'm not sure
>>>> validate
>>>> it on kernel entry is a good choice especially consider we've already had
>>>> header check.
>>> This just makes that check more strict. Frequency of malicious packets is
>>> not really relevant if a single bad packet can cause damage.
>>
>> We try hard to avoid flow dissector since its overhead is obvious. But looks
>> like this patch did it unconditionally, and even for non gso packet.
>>
>>> The alternative to validate on kernel entry is to harden the entire
>>> segmentation
>>> layer and lower part of the stack. That is much harder to get right and
>>> not
>>> necessarily cheaper.
>>
>> For performance reason. I think we should delay the check or segmentation as
>> much as possible until it was really needed.
> Going through segmentation is probably as expensive as flow dissector,
> if not more so because of the indirect branches.

I think we don't even need to care about this consider the evil packet 
should be rare. And what you propose here is just a very small subset of 
the necessary checking, more comes at gso header checking. So even if we 
care performance, it only help for some specific case.

>   And now we have two
> pieces of code that need to be hardened against malicious packets.

To me fixing the exist path is a good balance between security and 
performance.

>
> But I did overlook the guest to guest communication, with virtual devices
> that can set NETIF_F_GSO_ROBUST and so do not enter the stack. That
> means that one guest can send misrepresented packets to another. Is that
> preferable to absorbing the cost of validation?

The problem is that guests and hosts don't trust each other. Even if one 
packet has already been validated by one side, it must be validated 
again by another side when needed. Doing validation twice is meaningless 
in this case.

>
> The current patch does not actually deprecate the path through the
> segmentation layer. So it will indeed add overhead. We would have to
> remove the DODGY logic in net-next.

I don't get the point of removing this.

>   One additional possible optimization
> is to switch to flow_keys_buf_dissector, which does not dissect as deeply.
> I did that in an earlier patch; it should be sufficient for this purpose.

I suspect the cost is still noticeable, and consider we may support kind 
of tunnel offload in the future.

We should first assume the correctness of metadata provided by untrusted 
source and validate it before we really want to use it. Validate them 
during entry means we assume they are wrong, then there's no need for 
most of the fields of virtio-net header,

>
> A third option is to apply the above inet_gso_segment fix in net, then
> add flow dissection at the source in net-next together with removal of
> dodgy checks in the stack. Then that can be benchmarked properly.
>

So as I replied before, we probably want to bring back (at least dodgy 
part of 5c7cdf339af5).

>>> As a matter of fact, it incurs a cost on all packets, including the common
>>> case generated by the protocol stack.
>>
>> Btw, this looks could be triggered from guest. So it looks at least a DOS
>> form guest to host which should be treated as CVE?
> Good point. The report was public, so I focused on getting it fixed first.
> You mean just to request a CVE? I'll do that that once we have a fix.

Yes, just no need for embargo.

Thanks

Re: [PATCH net] net: validate untrusted gso packets

[Willem de Bruijn]

>>>> virtio_net_hdr_to_skb checks gso_type, but it does not verify that this
>>>> type correctly describes the actual packet. Segmentation happens based
>>>> on packet contents. So a packet was crafted to enter sctp gso, even
>>>> though no such gso_type exists. This issue is not specific to sctp.
>>>
>>>
>>> So it looks to me we should do it in here in sctp_gso_segment().
>>>
>>> if (skb_gso_ok(skb, features | NETIF_F_GSO_ROBUST)) {
>>>          /* Packet is from an untrusted source, reset gso_segs. */
>>
>> No dodgy source can legitimately generate sctp code, so it should not
>> even get there.
>
>
> I think that's why we need extra check for dodgy packets here

Commit 5c7cdf339af5 ("gso: Remove arbitrary checks for unsupported
GSO") only compares gso_type against a list of allowed gso types.

It did not compare gso_type against the actual packet contents, so
would not have protected against this vulnerability.

>, we used to
> have gso_type verification here but was removed. In the future, virtio will
> support passing sctp offload packet down for sure, so we need consider this
> case.

Then the sctp handler will first need to be fixed to not crash on bad packets.
And every other handler for which no virtio gso type exists.

>>   Also, a packet can just as easily spoof an esp packet.
>> See also the discussion in the Link above.
>
>
> Then we probably need check there.

Adding a check in every gso handler that does not expect packets
with SKB_GSO_DODGY source seems backwards to me.

A packet with gso_type SKB_GSO_TCPV4 should never be
delivered to an sctp handler, say. We must check before passing
to a handler whether the packet matches the callback.

>>
>> We can address this specific issue in segmentation by placing a check
>> analogous to skb_validate_dodgy_gso in the network layer. Something
>> like this (but with ECN option support):
>>
>> @@ -1258,6 +1258,22 @@ struct sk_buff *inet_gso_segment(struct sk_buff
>> *skb,
>>
>>          skb_reset_transport_header(skb);
>>
>> +       gso_type = skb_shinfo(skb)->gso_type;
>> +       if (gso_type & SKB_GSO_DODGY) {
>> +               switch (gso_type & (SKB_GSO_TCPV4 | SKB_GSO_UDP)) {
>> +               case SKB_GSO_TCPV4:
>> +                       if (proto != IPPROTO_TCP)
>> +                               goto out;
>> +                       break;
>> +               case SKB_GSO_UDP:
>> +                       if (proto != IPPROTO_UDP)
>> +                               goto out;
>> +                       break;
>> +               default:
>> +                       goto out;
>> +               }
>> +       }
>
>
> This implements subset of function for codes which was removed by the commit
> I mentioned below.

No, as I explain above, it performs a different check.

>
>>> And we probably need to recover what has been removed since
>>> 5c7cdf339af560f980b12eb6b0b5aa5f68ac6658 ("gso: Remove arbitrary checks
>>> for
>>> unsupported GSO").
>>>
>>>>>> User packets can also have corrupted headers, tripping up segmentation
>>>>>> logic that expects sane packets from the trusted protocol stack.
>>>>>> Hardening all segmentation paths against all bad packets is error
>>>>>> prone and slows down the common path, so validate on kernel entry.
>>>>>
>>>>>
>>>>> I think evil packets should be rare in common case, so I'm not sure
>>>>> validate
>>>>> it on kernel entry is a good choice especially consider we've already
>>>>> had
>>>>> header check.
>>>>
>>>> This just makes that check more strict. Frequency of malicious packets
>>>> is
>>>> not really relevant if a single bad packet can cause damage.
>>>
>>>
>>> We try hard to avoid flow dissector since its overhead is obvious. But
>>> looks
>>> like this patch did it unconditionally, and even for non gso packet.
>>>
>>>> The alternative to validate on kernel entry is to harden the entire
>>>> segmentation
>>>> layer and lower part of the stack. That is much harder to get right and
>>>> not
>>>> necessarily cheaper.
>>>
>>>
>>> For performance reason. I think we should delay the check or segmentation
>>> as
>>> much as possible until it was really needed.
>>
>> Going through segmentation is probably as expensive as flow dissector,
>> if not more so because of the indirect branches.
>
>
> I think we don't even need to care about this consider the evil packet
> should be rare.

How does frequency matter when a single packet can crash a host?

> And what you propose here is just a very small subset of the
> necessary checking, more comes at gso header checking. So even if we care
> performance, it only help for some specific case.

It also fixed the bug that Eric sent a separate patch for, as that did
not dissect as a valid TCP packet, either.

>>   And now we have two
>> pieces of code that need to be hardened against malicious packets.
>
>
> To me fixing the exist path is a good balance between security and
> performance.
>
>>
>> But I did overlook the guest to guest communication, with virtual devices
>> that can set NETIF_F_GSO_ROBUST and so do not enter the stack. That
>> means that one guest can send misrepresented packets to another. Is that
>> preferable to absorbing the cost of validation?
>
>
> The problem is that guests and hosts don't trust each other. Even if one
> packet has already been validated by one side, it must be validated again by
> another side when needed. Doing validation twice is meaningless in this
> case.

Ack, agreed that guests need to have defensive coding of themselves.
Then it's fine to pass packets to guests where the gso_type does not
match the contents.

>
>>
>> The current patch does not actually deprecate the path through the
>> segmentation layer. So it will indeed add overhead. We would have to
>> remove the DODGY logic in net-next.
>
>
> I don't get the point of removing this.
>
>>   One additional possible optimization
>> is to switch to flow_keys_buf_dissector, which does not dissect as deeply.
>> I did that in an earlier patch; it should be sufficient for this purpose.
>
>
> I suspect the cost is still noticeable, and consider we may support kind of
> tunnel offload in the future.
>
> We should first assume the correctness of metadata provided by untrusted
> source and validate it before we really want to use it. Validate them during
> entry means we assume they are wrong, then there's no need for most of the
> fields of virtio-net header,

If you always need to use the data, then you always validate. In which case
you want to validate early, as there will be less vulnerable code before
validation.

But I see what I think is your point: in guest to guest communication we
do not need to validate at all, so early validation adds unnecessary cost
for this important use case. That's a fair argument for validating later and
only when needed (i.e., a path without NETIF_F_GSO_ROBUST).

Re: [PATCH net] net: validate untrusted gso packets

[Jason Wang]

On 2018年01月18日 13:09, Willem de Bruijn wrote:
>>>    Also, a packet can just as easily spoof an esp packet.
>>> See also the discussion in the Link above.
>> Then we probably need check there.
> Adding a check in every gso handler that does not expect packets
> with SKB_GSO_DODGY source seems backwards to me.
>
> A packet with gso_type SKB_GSO_TCPV4 should never be
> delivered to an sctp handler, say. We must check before passing
> to a handler whether the packet matches the callback.

That's the case for trusted source. For dodgy source, we can't assume that.

>
>>> We can address this specific issue in segmentation by placing a check
>>> analogous to skb_validate_dodgy_gso in the network layer. Something
>>> like this (but with ECN option support):
>>>
>>> @@ -1258,6 +1258,22 @@ struct sk_buff *inet_gso_segment(struct sk_buff
>>> *skb,
>>>
>>>           skb_reset_transport_header(skb);
>>>
>>> +       gso_type = skb_shinfo(skb)->gso_type;
>>> +       if (gso_type & SKB_GSO_DODGY) {
>>> +               switch (gso_type & (SKB_GSO_TCPV4 | SKB_GSO_UDP)) {
>>> +               case SKB_GSO_TCPV4:
>>> +                       if (proto != IPPROTO_TCP)
>>> +                               goto out;
>>> +                       break;
>>> +               case SKB_GSO_UDP:
>>> +                       if (proto != IPPROTO_UDP)
>>> +                               goto out;
>>> +                       break;
>>> +               default:
>>> +                       goto out;
>>> +               }
>>> +       }
>> This implements subset of function for codes which was removed by the commit
>> I mentioned below.
> No, as I explain above, it performs a different check.
>
>>>>

[...]

>>>> For performance reason. I think we should delay the check or segmentation
>>>> as
>>>> much as possible until it was really needed.
>>> Going through segmentation is probably as expensive as flow dissector,
>>> if not more so because of the indirect branches.
>> I think we don't even need to care about this consider the evil packet
>> should be rare.
> How does frequency matter when a single packet can crash a host?

I mean consider we had fix the crash, we don't care how expensive do we 
spot this.

>
>> And what you propose here is just a very small subset of the
>> necessary checking, more comes at gso header checking. So even if we care
>> performance, it only help for some specific case.
> It also fixed the bug that Eric sent a separate patch for, as that did
> not dissect as a valid TCP packet, either.

I may miss something but how did this patch protects an evil thoff?

>>>    And now we have two
>>> pieces of code that need to be hardened against malicious packets.
>> To me fixing the exist path is a good balance between security and
>> performance.
>>
>>> But I did overlook the guest to guest communication, with virtual devices
>>> that can set NETIF_F_GSO_ROBUST and so do not enter the stack. That
>>> means that one guest can send misrepresented packets to another. Is that
>>> preferable to absorbing the cost of validation?
>> The problem is that guests and hosts don't trust each other. Even if one
>> packet has already been validated by one side, it must be validated again by
>> another side when needed. Doing validation twice is meaningless in this
>> case.
> Ack, agreed that guests need to have defensive coding of themselves.
> Then it's fine to pass packets to guests where the gso_type does not
> match the contents.
>
>>> The current patch does not actually deprecate the path through the
>>> segmentation layer. So it will indeed add overhead. We would have to
>>> remove the DODGY logic in net-next.
>> I don't get the point of removing this.
>>
>>>    One additional possible optimization
>>> is to switch to flow_keys_buf_dissector, which does not dissect as deeply.
>>> I did that in an earlier patch; it should be sufficient for this purpose.
>> I suspect the cost is still noticeable, and consider we may support kind of
>> tunnel offload in the future.
>>
>> We should first assume the correctness of metadata provided by untrusted
>> source and validate it before we really want to use it. Validate them during
>> entry means we assume they are wrong, then there's no need for most of the
>> fields of virtio-net header,
> If you always need to use the data, then you always validate. In which case
> you want to validate early, as there will be less vulnerable code before
> validation.
>
> But I see what I think is your point: in guest to guest communication we
> do not need to validate at all, so early validation adds unnecessary cost
> for this important use case. That's a fair argument for validating later and
> only when needed (i.e., a path without NETIF_F_GSO_ROBUST).

Yes, and looking at Herbert's commit log for 576a30eb6453 ("[NET]: Added 
GSO header verification"). It was intended do the verification just 
before gso.

Thanks

Re: [PATCH net] net: validate untrusted gso packets

[Willem de Bruijn]

>>> This implements subset of function for codes which was removed by the
>>> commit
>>> I mentioned below.
>>
>> No, as I explain above, it performs a different check.
>>
>>>>>
>
> [...]

Clearly I was wrong, sorry. Thanks for pointing out that commit and
576a30eb6453 ("[NET]: Added GSO header verification").

>>>>> For performance reason. I think we should delay the check or
>>>>> segmentation
>>>>> as
>>>>> much as possible until it was really needed.
>>>>
>>>> Going through segmentation is probably as expensive as flow dissector,
>>>> if not more so because of the indirect branches.
>>>
>>> I think we don't even need to care about this consider the evil packet
>>> should be rare.
>>
>> How does frequency matter when a single packet can crash a host?
>
>
> I mean consider we had fix the crash, we don't care how expensive do we spot
> this.
>
>>
>>> And what you propose here is just a very small subset of the
>>> necessary checking, more comes at gso header checking. So even if we care
>>> performance, it only help for some specific case.
>>
>> It also fixed the bug that Eric sent a separate patch for, as that did
>> not dissect as a valid TCP packet, either.
>
>
> I may miss something but how did this patch protects an evil thoff?

Actually, it blocked that specific reproducer because the ip protocol
did not match.

I think that __skb_flow_dissect_tcp should return a boolean, causing
dissection return FLOW_DISSECT_RET_OUT_BAD if the tcph is bad.
That would be needed to really catch it with flow dissection at the source.

Re: [PATCH net] net: validate untrusted gso packets

[Jason Wang]

On 2018年01月19日 08:53, Willem de Bruijn wrote:
>>>> And what you propose here is just a very small subset of the
>>>> necessary checking, more comes at gso header checking. So even if we care
>>>> performance, it only help for some specific case.
>>> It also fixed the bug that Eric sent a separate patch for, as that did
>>> not dissect as a valid TCP packet, either.
>> I may miss something but how did this patch protects an evil thoff?
> Actually, it blocked that specific reproducer because the ip protocol
> did not match.

I see.

>
> I think that __skb_flow_dissect_tcp should return a boolean, causing
> dissection return FLOW_DISSECT_RET_OUT_BAD if the tcph is bad.
> That would be needed to really catch it with flow dissection at the source.

Just sanitize transport to offset_hint (0) in the case of tun. It looks 
to me flow dissector will return FLOW_DISSECT_RET_OUT_BAD too if it 
can't recognize the protocol. We can't differ the real failure from 
unrecognized protocol. (or change the return from bool to int).

Thanks

Re: [PATCH net] net: validate untrusted gso packets

[Willem de Bruijn]

On Fri, Jan 19, 2018 at 3:19 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2018年01月19日 08:53, Willem de Bruijn wrote:
>>>>>
>>>>> And what you propose here is just a very small subset of the
>>>>> necessary checking, more comes at gso header checking. So even if we
>>>>> care
>>>>> performance, it only help for some specific case.
>>>>
>>>> It also fixed the bug that Eric sent a separate patch for, as that did
>>>> not dissect as a valid TCP packet, either.
>>>
>>> I may miss something but how did this patch protects an evil thoff?
>>
>> Actually, it blocked that specific reproducer because the ip protocol
>> did not match.
>
>
> I see.
>
>>
>> I think that __skb_flow_dissect_tcp should return a boolean, causing
>> dissection return FLOW_DISSECT_RET_OUT_BAD if the tcph is bad.
>> That would be needed to really catch it with flow dissection at the
>> source.
>
>
> Just sanitize transport to offset_hint (0) in the case of tun.

That is the current approach in skb_probe_transport_header, but it
opens us up to parsing of garbage headers later in the stack when
unconditionally reading tcp_hdrlen. The qdisc_pkt_len_init case is one
such example. Seems better to leave transport header unset in such
cases and qualify all access to the headers if DODGY.

> It looks to
> me flow dissector will return FLOW_DISSECT_RET_OUT_BAD too if it can't
> recognize the protocol. We can't differ the real failure from unrecognized
> protocol. (or change the return from bool to int).

Unrecognized protocol should imply failure. virtio_net_hdr_to_skb accepts
a whitelist of protocols, all of which the flow dissector can verify.

Another argument for early validation: we cannot currently differentiate
tunnel headers inserted by the tun/pf_packet/xen user from those
generated by the stack if both are present.

The kernel currently does not define tunnel VIRTIO_NET_HDR_GSO
types, so should drop packets with tunnel headers. Tunnel gso handlers
indeed do drop these if skb->encapsulation is not set.

But if a packet travels through a tunnel device and the bit is set, at
segmentation time we cannot distinguish trusted stack headers from
possibly malformed user supplied ones.

Re: [PATCH net] net: validate untrusted gso packets

[Jason Wang]

On 2018年01月19日 22:39, Willem de Bruijn wrote:
> On Fri, Jan 19, 2018 at 3:19 AM, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2018年01月19日 08:53, Willem de Bruijn wrote:
>>>>>> And what you propose here is just a very small subset of the
>>>>>> necessary checking, more comes at gso header checking. So even if we
>>>>>> care
>>>>>> performance, it only help for some specific case.
>>>>> It also fixed the bug that Eric sent a separate patch for, as that did
>>>>> not dissect as a valid TCP packet, either.
>>>> I may miss something but how did this patch protects an evil thoff?
>>> Actually, it blocked that specific reproducer because the ip protocol
>>> did not match.
>>
>> I see.
>>
>>> I think that __skb_flow_dissect_tcp should return a boolean, causing
>>> dissection return FLOW_DISSECT_RET_OUT_BAD if the tcph is bad.
>>> That would be needed to really catch it with flow dissection at the
>>> source.
>>
>> Just sanitize transport to offset_hint (0) in the case of tun.
> That is the current approach in skb_probe_transport_header, but it
> opens us up to parsing of garbage headers later in the stack when
> unconditionally reading tcp_hdrlen. The qdisc_pkt_len_init case is one
> such example. Seems better to leave transport header unset in such
> cases and qualify all access to the headers if DODGY.
>
>> It looks to
>> me flow dissector will return FLOW_DISSECT_RET_OUT_BAD too if it can't
>> recognize the protocol. We can't differ the real failure from unrecognized
>> protocol. (or change the return from bool to int).
> Unrecognized protocol should imply failure. virtio_net_hdr_to_skb accepts
> a whitelist of protocols, all of which the flow dissector can verify.

Yes and only for gso packets.

>
> Another argument for early validation: we cannot currently differentiate
> tunnel headers inserted by the tun/pf_packet/xen user from those
> generated by the stack if both are present.
>
> The kernel currently does not define tunnel VIRTIO_NET_HDR_GSO
> types, so should drop packets with tunnel headers. Tunnel gso handlers
> indeed do drop these if skb->encapsulation is not set.
>
> But if a packet travels through a tunnel device and the bit is set, at
> segmentation time we cannot distinguish trusted stack headers from
> possibly malformed user supplied ones.

Right.

Thanks