[PATCH net v2] gso: validate gso_type if SKB_GSO_DODGY

[PATCH net v2] gso: validate gso_type if SKB_GSO_DODGY

[Willem de Bruijn]

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

Validate gso_type during segmentation as SKB_GSO_DODGY sources
may pass packets where the gso_type does not match the contents.

Syzkaller was able to enter the SCTP gso handler with a packet of
gso_type SKB_GSO_TCPV4.

On entry of transport layer gso handlers, verify that the gso_type
matches the transport protocol.

Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
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>

---
Similar checks existed until removed in commit 5c7cdf339af5 ("gso:
Remove arbitrary checks for unsupported GSO"). But those were limited
to the TSO path, not software GSO. I believe that this issue goes
back further, hence the Fixes at the first user of virtio_net_hdr.
---
 net/ipv4/esp4_offload.c  | 3 +++
 net/ipv4/tcp_offload.c   | 3 +++
 net/ipv4/udp_offload.c   | 3 +++
 net/ipv6/esp6_offload.c  | 3 +++
 net/ipv6/tcpv6_offload.c | 3 +++
 net/ipv6/udp_offload.c   | 3 +++
 net/sctp/offload.c       | 3 +++
 7 files changed, 21 insertions(+)

diff --git a/net/ipv4/esp4_offload.c b/net/ipv4/esp4_offload.c
index b1338e576d00..29b333a62ab0 100644
--- a/net/ipv4/esp4_offload.c
+++ b/net/ipv4/esp4_offload.c
@@ -122,6 +122,9 @@ static struct sk_buff *esp4_gso_segment(struct sk_buff *skb,
 	if (!xo)
 		goto out;
 
+	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_ESP))
+		goto out;
+
 	seq = xo->seq.low;
 
 	x = skb->sp->xvec[skb->sp->len - 1];
diff --git a/net/ipv4/tcp_offload.c b/net/ipv4/tcp_offload.c
index b6a2aa1dcf56..4d58e2ce0b5b 100644
--- a/net/ipv4/tcp_offload.c
+++ b/net/ipv4/tcp_offload.c
@@ -32,6 +32,9 @@ static void tcp_gso_tstamp(struct sk_buff *skb, unsigned int ts_seq,
 static struct sk_buff *tcp4_gso_segment(struct sk_buff *skb,
 					netdev_features_t features)
 {
+	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_TCPV4))
+		return ERR_PTR(-EINVAL);
+
 	if (!pskb_may_pull(skb, sizeof(struct tcphdr)))
 		return ERR_PTR(-EINVAL);
 
diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
index 01801b77bd0d..ea6e6e7df0ee 100644
--- a/net/ipv4/udp_offload.c
+++ b/net/ipv4/udp_offload.c
@@ -203,6 +203,9 @@ static struct sk_buff *udp4_ufo_fragment(struct sk_buff *skb,
 		goto out;
 	}
 
+	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP))
+		goto out;
+
 	if (!pskb_may_pull(skb, sizeof(struct udphdr)))
 		goto out;
 
diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
index dd9627490c7c..f52c314d4c97 100644
--- a/net/ipv6/esp6_offload.c
+++ b/net/ipv6/esp6_offload.c
@@ -149,6 +149,9 @@ static struct sk_buff *esp6_gso_segment(struct sk_buff *skb,
 	if (!xo)
 		goto out;
 
+	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_ESP))
+		goto out;
+
 	seq = xo->seq.low;
 
 	x = skb->sp->xvec[skb->sp->len - 1];
diff --git a/net/ipv6/tcpv6_offload.c b/net/ipv6/tcpv6_offload.c
index d883c9204c01..278e49cd67d4 100644
--- a/net/ipv6/tcpv6_offload.c
+++ b/net/ipv6/tcpv6_offload.c
@@ -46,6 +46,9 @@ static struct sk_buff *tcp6_gso_segment(struct sk_buff *skb,
 {
 	struct tcphdr *th;
 
+	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6))
+		return ERR_PTR(-EINVAL);
+
 	if (!pskb_may_pull(skb, sizeof(*th)))
 		return ERR_PTR(-EINVAL);
 
diff --git a/net/ipv6/udp_offload.c b/net/ipv6/udp_offload.c
index a0f89ad76f9d..2a04dc9c781b 100644
--- a/net/ipv6/udp_offload.c
+++ b/net/ipv6/udp_offload.c
@@ -42,6 +42,9 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb,
 		const struct ipv6hdr *ipv6h;
 		struct udphdr *uh;
 
+		if (!(skb_shinfo(skb)->gso_type & SKB_GSO_UDP))
+			goto out;
+
 		if (!pskb_may_pull(skb, sizeof(struct udphdr)))
 			goto out;
 
diff --git a/net/sctp/offload.c b/net/sctp/offload.c
index 275925b93b29..35bc7106d182 100644
--- a/net/sctp/offload.c
+++ b/net/sctp/offload.c
@@ -45,6 +45,9 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb,
 	struct sk_buff *segs = ERR_PTR(-EINVAL);
 	struct sctphdr *sh;
 
+	if (!(skb_shinfo(skb)->gso_type & SKB_GSO_SCTP))
+		goto out;
+
 	sh = sctp_hdr(skb);
 	if (!pskb_may_pull(skb, sizeof(*sh)))
 		goto out;
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH net v2] gso: validate gso_type if SKB_GSO_DODGY

[Jason Wang]

On 2018年01月19日 08:19, Willem de Bruijn wrote:
> From: Willem de Bruijn<willemb@google.com>
>
> Validate gso_type during segmentation as SKB_GSO_DODGY sources
> may pass packets where the gso_type does not match the contents.
>
> Syzkaller was able to enter the SCTP gso handler with a packet of
> gso_type SKB_GSO_TCPV4.
>
> On entry of transport layer gso handlers, verify that the gso_type
> matches the transport protocol.
>
> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> 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, just two nits:

1) I still suspect the "Fixes" is not accurate, should it be the commit 
of sctp offloading?
2) The patch checks for non dodgy packets too so the title is not correct.

Re: [PATCH net v2] gso: validate gso_type if SKB_GSO_DODGY

[Willem de Bruijn]

On Fri, Jan 19, 2018 at 7:36 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2018年01月19日 08:19, Willem de Bruijn wrote:
>>
>> From: Willem de Bruijn<willemb@google.com>
>>
>> Validate gso_type during segmentation as SKB_GSO_DODGY sources
>> may pass packets where the gso_type does not match the contents.
>>
>> Syzkaller was able to enter the SCTP gso handler with a packet of
>> gso_type SKB_GSO_TCPV4.
>>
>> On entry of transport layer gso handlers, verify that the gso_type
>> matches the transport protocol.
>>
>> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
>> 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, just two nits:
>
> 1) I still suspect the "Fixes" is not accurate, should it be the commit of
> sctp offloading?

That commit c5c4e45c4b79 ("sctp: fix GSO for IPv6") is older than the
equivalent for ESP, so catches both protocols that were added since the
TSO checks were removed.

It implies that TCP and UFO are robust against packets of wrong
transport protocol. As the previous TSO checks did not cover software
GSO, either, I suppose we can assume that.

With that caveat: okay, I will change it.

> 2) The patch checks for non dodgy packets too so the title is not correct.

Ack, will change to.

Thanks

Re: [PATCH net v2] gso: validate gso_type if SKB_GSO_DODGY

[Willem de Bruijn]

On Fri, Jan 19, 2018 at 9:25 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Fri, Jan 19, 2018 at 7:36 AM, Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年01月19日 08:19, Willem de Bruijn wrote:
>>>
>>> From: Willem de Bruijn<willemb@google.com>
>>>
>>> Validate gso_type during segmentation as SKB_GSO_DODGY sources
>>> may pass packets where the gso_type does not match the contents.
>>>
>>> Syzkaller was able to enter the SCTP gso handler with a packet of
>>> gso_type SKB_GSO_TCPV4.
>>>
>>> On entry of transport layer gso handlers, verify that the gso_type
>>> matches the transport protocol.
>>>
>>> Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
>>> 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, just two nits:
>>
>> 1) I still suspect the "Fixes" is not accurate, should it be the commit of
>> sctp offloading?
>
> That commit c5c4e45c4b79 ("sctp: fix GSO for IPv6") is older than the
> equivalent for ESP, so catches both protocols that were added since the
> TSO checks were removed.

pasted the wrong commit. I meant 90017accff61 ("sctp: Add GSO support")