general protection fault in skb_segment

[syzbot]

Hello,

syzkaller hit the following crash on
37759fa6d0fa9e4d6036d19ac12f555bfc0aeafd
git://git.cmpxchg.org/linux-mmots.git/master
compiler: gcc (GCC) 7.1.1 20170620
.config is attached
Raw console output is attached.
C reproducer is attached
syzkaller reproducer is attached. See https://goo.gl/kgGztJ
for information about syzkaller reproducers


IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+fee641...@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed. See footer for
details.
If you forward the report, please keep this part and the footer.

kasan: GPF could be caused by NULL-ptr deref or user memory access
general protection fault: 0000 [#1] SMP KASAN
Dumping ftrace buffer:
(ftrace buffer empty)
Modules linked in:
CPU: 0 PID: 3154 Comm: syzkaller909359 Not tainted 4.15.0-rc4-mm1+ #49
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
RIP: 0010:skb_segment+0x667/0x2fe0 net/core/skbuff.c:3566
RSP: 0018:ffff8801c84de9b8 EFLAGS: 00010206
RAX: 000000000000000f RBX: 0000000000000000 RCX: ffffffff842c1915
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000078
RBP: ffff8801c84dec70 R08: 0000000000000020 R09: 0000000000000002
R10: ffff8801c84decf8 R11: 0000000000000000 R12: 0000000000000008
R13: dffffc0000000000 R14: 0000000000000000 R15: ffff8801d55160c0
FS: 0000000000f53880(0000) GS:ffff8801db200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000020217f75 CR3: 00000001c86de001 CR4: 00000000001606f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
sctp_gso_segment+0x213/0x890 net/sctp/offload.c:72
inet_gso_segment+0x609/0x11b0 net/ipv4/af_inet.c:1288
skb_mac_gso_segment+0x33f/0x660 net/core/dev.c:2748
__skb_gso_segment+0x363/0x810 net/core/dev.c:2821
skb_gso_segment include/linux/netdevice.h:3996 [inline]
validate_xmit_skb+0x4ba/0xb20 net/core/dev.c:3074
validate_xmit_skb_list+0xb7/0x120 net/core/dev.c:3125
sch_direct_xmit+0x38e/0x920 net/sched/sch_generic.c:295
qdisc_restart net/sched/sch_generic.c:366 [inline]
__qdisc_run+0x571/0x1930 net/sched/sch_generic.c:374
__dev_xmit_skb net/core/dev.c:3189 [inline]
__dev_queue_xmit+0x89e/0x2200 net/core/dev.c:3489
dev_queue_xmit+0x17/0x20 net/core/dev.c:3554
packet_snd net/packet/af_packet.c:2943 [inline]
packet_sendmsg+0x3ad5/0x60a0 net/packet/af_packet.c:2968
sock_sendmsg_nosec net/socket.c:628 [inline]
sock_sendmsg+0xca/0x110 net/socket.c:638
sock_write_iter+0x31a/0x5d0 net/socket.c:907
call_write_iter include/linux/fs.h:1776 [inline]
new_sync_write fs/read_write.c:469 [inline]
__vfs_write+0x684/0x970 fs/read_write.c:482
vfs_write+0x189/0x510 fs/read_write.c:544
SYSC_write fs/read_write.c:589 [inline]
SyS_write+0xef/0x220 fs/read_write.c:581
entry_SYSCALL_64_fastpath+0x1f/0x96
RIP: 0033:0x444b69
RSP: 002b:00000000007eff78 EFLAGS: 00000293 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007ffe674fbec0 RCX: 0000000000444b69
RDX: 000000000000002a RSI: 0000000020217f75 RDI: 0000000000000005
RBP: 0000000000000000 R08: 0000000120080522 R09: 0000000120080522
R10: 0000000120080522 R11: 0000000000000293 R12: 0000000000402760
R13: 00000000004027f0 R14: 0000000000000000 R15: 0000000000000000
Code: 00 00 8b b5 1c fe ff ff 39 b5 30 fe ff ff 0f 8f 99 00 00 00 e8 ab 1a
44 fd 48 8b 85 48 fe ff ff 48 8d 78 78 48 89 f8 48 c1 e8 03 <42> 0f b6 14
28 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f
RIP: skb_segment+0x667/0x2fe0 net/core/skbuff.c:3566 RSP: ffff8801c84de9b8
---[ end trace 580392d61caec1a9 ]---


---
This bug is generated by a dumb bot. It may contain errors.
See https://goo.gl/tpsmEJ for details.
Direct all questions to syzk...@googlegroups.com.

syzbot will keep track of this bug report.
If you forgot to add the Reported-by tag, once the fix for this bug is
merged
into any tree, please reply to this email with:
#syz fix: exact-commit-title
If you want to test a patch for this bug, please reply with:
#syz test: git://repo/address.git branch
and provide the patch inline or as an attachment.
To mark this as a duplicate of another syzbot report, please reply with:
#syz dup: exact-subject-of-another-report
If it's a one-off invalid bug report, please reply with:
#syz invalid
Note: if the crash happens again, it will cause creation of a new bug
report.
Note: all commands must start from beginning of the line in the email body.

[Willem de Bruijn]

> syzkaller hit the following crash on
> 37759fa6d0fa9e4d6036d19ac12f555bfc0aeafd
> git://git.cmpxchg.org/linux-mmots.git/master
> compiler: gcc (GCC) 7.1.1 20170620
> .config is attached
> Raw console output is attached.
> C reproducer is attached
> syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> for information about syzkaller reproducers

Reproduced with the C reproducer on v4.15-rc1 and mainline
going back at least to v4.8, but not v4.7. SCTP GSO was
introduced in v4.8-rc1, so a patch in this set is likely the starting
point. Indeed crashes at 90017accff61 ("sctp: Add GSO support"),
but not at 90017accff61~4.

The reproducer with its sandbox removed shows this invocation in strace -f

# strace -f ./repro2
[... skipped ...]
socket(PF_INET, SOCK_STREAM, IPPROTO_IP) = 3
open("/dev/net/tun", O_RDONLY) = 4
fcntl(4, F_DUPFD, 3) = 5
socket(PF_PACKET, SOCK_RAW|SOCK_CLOEXEC, 8) = 6
ioctl(4, TUNSETIFF, 0x20e63000) = 0
ioctl(3, SIOCSIFFLAGS, {ifr_name="syz0",
ifr_flags=IFF_UP|IFF_PROMISC|IFF_ALLMULTI}) = 0
setsockopt(6, SOL_PACKET, 0xf /* PACKET_??? */, [4096], 4) = 0
ioctl(6, SIOCGIFINDEX, {ifr_name="syz0", ifr_index=24}) = 0
bind(6, {sa_family=AF_PACKET, proto=0000, if24, pkttype=PACKET_HOST,
addr(6)={1, aaaaaaaaaa00}, 20) = 0
dup2(6, 5) = 5
write(5, "\0\201\1\0\350\367\0\0\3\0E\364\0 \0d\0\0\7\2042\342\0\0\0
\177\0\0\1\0\t"..., 42

where 0xf in setsockopt is PACKET_VNET_HDR

So this is a packet socket writing something that apparently looks
like an SCTP packet, is only 42 bytes long, but has GSO set in its
virtio_net_hdr struct.

It crashes in skb_segment seemingly on a NULL list_skb.

(gdb) list *(skb_segment+0x2a4)
0xffffffff8167cc24 is in skb_segment (net/core/skbuff.c:3566).
3561 if (hsize < 0)
3562 hsize = 0;
3563 if (hsize > len || !sg)
3564 hsize = len;
3565
3566 if (!hsize && i >= nfrags && skb_headlen(list_skb) &&
3567 (skb_headlen(list_skb) == len || sg)) {
3568 BUG_ON(skb_headlen(list_skb) > len);
3569
3570 i = 0;

Likely there is a hidden assumption about SCTP GSO packets that does
not hold for such packets generated by PF_PACKET.

SCTP GSO introduced the GSO_BY_FRAGS mss value, so the code
takes a different path for SCTP packets generated by the SCTP stack.

PF_PACKET does not necessarily set gso_size to GSO_BY_FRAGS, so
does not take the branch that requires list_skb to be non-zero here:

if (unlikely(mss == GSO_BY_FRAGS)) {
len = list_skb->len;
} else {
len = head_skb->len - offset;
if (len > mss)
len = mss;
}

hsize = skb_headlen(head_skb) - offset;
if (hsize < 0)
hsize = 0;
if (hsize > len || !sg)
hsize = len;

if (!hsize && i >= nfrags && skb_headlen(list_skb) &&
(skb_headlen(list_skb) == len || sg)) {

Somewhat tangential, but any PF_PACKET socket can set this
magic gso_size value in its virtio_net_hdr, so if it is assumed to
be an SCTP GSO specific option, setting it for a TCP GSO packet
may also cause unexpected results.

The crash requires a kernel with CONFIG_IP_SCTP enabled.

[Willem de Bruijn]

> So this is a packet socket writing something that apparently looks
> like an SCTP packet, is only 42 bytes long, but has GSO set in its
> virtio_net_hdr struct.
>
> It crashes in skb_segment seemingly on a NULL list_skb.
>
> (gdb) list *(skb_segment+0x2a4)
> 0xffffffff8167cc24 is in skb_segment (net/core/skbuff.c:3566).
> 3561 if (hsize < 0)
> 3562 hsize = 0;
> 3563 if (hsize > len || !sg)
> 3564 hsize = len;
> 3565
> 3566 if (!hsize && i >= nfrags && skb_headlen(list_skb) &&
> 3567 (skb_headlen(list_skb) == len || sg)) {
> 3568 BUG_ON(skb_headlen(list_skb) > len);
> 3569
> 3570 i = 0;

It appears to be a packet that consists only of an sctp header.
sctp_gso_segment pulls the header before calling skb_segment,
after which hsize == skb_headlen(head_skb) == 0 and nfrags == 0.

This check avoids the crash, but still triggers an skb_warn_bad_offload
on return in __skb_gso_segment

@@ -45,6 +45,13 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb,
struct sk_buff *segs = ERR_PTR(-EINVAL);
struct sctphdr *sh;

+ if (!skb_has_frag_list(skb))
+ goto out;

A gso packet shorter than mss should perhaps just be dropped. The stack
does not generate these. tcp_gso_segment does have a test

if (unlikely(skb->len <= mss))
goto out;

but as mss is derived from gso_size, which a packet socket controls, this
may not be sufficient for this purpose.

[Xin Long]

Shouldn't this check be in skb_segment(), right after if (mss ==
GSO_BY_FRAGS), like
if (unlikely(mss == GSO_BY_FRAGS)) {
if (unlikely(!list_skb))
goto err;
len = list_skb->len;

as the fix of commit 3953c46c3ac7 ("sk_buff: allow segmenting based on
frag sizes").

[Xin Long]

On Sun, Dec 31, 2017 at 10:25 AM, Marcelo Ricardo Leitner
<marcelo...@gmail.com> wrote:
> On Sat, Dec 30, 2017 at 10:52:20PM -0200, Marcelo Ricardo Leitner wrote:
>> On Sat, Dec 30, 2017 at 08:42:41AM +0100, Willem de Bruijn wrote:
> [...]

>> > Somewhat tangential, but any PF_PACKET socket can set this
>> > magic gso_size value in its virtio_net_hdr, so if it is assumed to
>> > be an SCTP GSO specific option, setting it for a TCP GSO packet
>> > may also cause unexpected results.
>>

>> It seems virtio_net could use more sanity checks. When PACKET_VNET_HDR
>> is used, it will end up calling:
>> tpacket_rcv() {
>> ...
>> if (do_vnet) {
>> if (virtio_net_hdr_from_skb(skb, h.raw + macoff -
>> sizeof(struct virtio_net_hdr),
>> vio_le(), true)) {
>> spin_lock(&sk->sk_receive_queue.lock);
>> goto drop_n_account;
>> }
>> }
>>
>> and virtio_net_hdr_from_skb does:
>> if (skb_is_gso(skb)) {
>> ...
>> if (sinfo->gso_type & SKB_GSO_TCPV4)
>> hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
>> else if (sinfo->gso_type & SKB_GSO_TCPV6)
>> hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
>> else
>> return -EINVAL;
>>
>> Meaning that any gso_type other than TCP would be rejected, but this
>> SCTP one got through. Seems the header contains a sctp header, but the
>> gso_type set was actually pointing to TCP (otherwise it would have
>> been rejected). AFAICT if this packet had an ESP header, for example,
>> it could have hit esp4_gso_segment. Can you please confirm this?
>
> I added:
> --- a/net/sctp/offload.c
> +++ b/net/sctp/offload.c
> @@ -44,6 +44,18 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb,

> {
> struct sk_buff *segs = ERR_PTR(-EINVAL);
> struct sctphdr *sh;

> + int fail = 0;
> +
> + if (!(skb_shinfo(skb)->gso_type & SKB_GSO_SCTP)) {
> + printk("Bogus gso_type: %x\n", skb_shinfo(skb)->gso_type);
> + fail = 1;
> + }
> + if (skb_shinfo(skb)->gso_size != GSO_BY_FRAGS) {
> + printk("Bogus gso_size: %u\n", skb_shinfo(skb)->gso_size);
> + fail = 1;
> + }
> + if (fail)
> + goto out;
>
> sh = sctp_hdr(skb);
> if (!pskb_may_pull(skb, sizeof(*sh)))
>

> and with the reproducer, got:
> [ 54.255469] Bogus gso_type: 7
> [ 54.258801] Bogus gso_size: 63464
> [ 54.262532] ------------[ cut here ]------------
> [ 54.267703] syz0: caps=(0x00000800000058c1, 0x0000000000000000) len=32 data_len=0 gso_size=63464 gso_type=7 ip_summed0
> [ 54.279777] WARNING: CPU: 1 PID: 13005 at /root/linux/net/core/dev.c:2600 skb_warn_bad_offload+0xd6/0xec
I couldn't reproduce this call trace on net-next, maybe it's been fixed by:
commit 8d74e9f88d65af8bb2e095aff506aa6eac755ada
Author: Willem de Bruijn <wil...@google.com>
Date: Tue Dec 12 11:39:04 2017 -0500

net: avoid skb_warn_bad_offload on IS_ERR

[Willem de Bruijn]

>> and with the reproducer, got:
>> [ 54.255469] Bogus gso_type: 7
>> [ 54.258801] Bogus gso_size: 63464
>> [ 54.262532] ------------[ cut here ]------------
>> [ 54.267703] syz0: caps=(0x00000800000058c1, 0x0000000000000000) len=32 data_len=0 gso_size=63464 gso_type=7 ip_summed0
>> [ 54.279777] WARNING: CPU: 1 PID: 13005 at /root/linux/net/core/dev.c:2600 skb_warn_bad_offload+0xd6/0xec
> I couldn't reproduce this call trace on net-next, maybe it's been fixed by:
> commit 8d74e9f88d65af8bb2e095aff506aa6eac755ada
> Author: Willem de Bruijn <wil...@google.com>
> Date: Tue Dec 12 11:39:04 2017 -0500
>
> net: avoid skb_warn_bad_offload on IS_ERR

Yes, I forgot to mention that that has been fixed in net-next.

It does not address the crash, but does suppress the gratuitous
warning once we make segmentation return with error for bad
packets like these.

[Willem de Bruijn]

> It seems virtio_net could use more sanity checks. When PACKET_VNET_HDR
> is used, it will end up calling:
> tpacket_rcv() {
> ...
> if (do_vnet) {
> if (virtio_net_hdr_from_skb(skb, h.raw + macoff -
> sizeof(struct virtio_net_hdr),
> vio_le(), true)) {
> spin_lock(&sk->sk_receive_queue.lock);
> goto drop_n_account;
> }
> }
>
> and virtio_net_hdr_from_skb does:
> if (skb_is_gso(skb)) {
> ...
> if (sinfo->gso_type & SKB_GSO_TCPV4)
> hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> else if (sinfo->gso_type & SKB_GSO_TCPV6)
> hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> else
> return -EINVAL;

That is the receive path, but the send path is analogous. Just adds
UFO.

> Meaning that any gso_type other than TCP would be rejected, but this
> SCTP one got through. Seems the header contains a sctp header, but the
> gso_type set was actually pointing to TCP (otherwise it would have
> been rejected). AFAICT if this packet had an ESP header, for example,
> it could have hit esp4_gso_segment. Can you please confirm this?

I have not tested this yet, but it certainly seems plausible.

There is nothing ensuring consistency between gso_type and
the actual packet contents that are parsed to look up gso callbacks.

> I don't know of anywhere in the stack validating if the gso_type
> matches the header that actually is in there.
>
> The fix you mentioned is a good start, we want that one way or
> another, but I'm afraid this bug is bigger than sctp.

Good point. Packet sockets require CAP_NET_RAW, but this is also
taken for virtio, so we probably want more stringent entry tests here.

The alternative to harden the segmentation code itself with a gso_type
sanity check in every gso callback is more work and fragile.

Need to figure out whether a brief check for just TCP or UDP is sufficient
or we need a full flow dissector step to support tunnel headers and such.

[Marcelo Ricardo Leitner]

On Sat, Dec 30, 2017 at 10:52:20PM -0200, Marcelo Ricardo Leitner wrote:
> On Sat, Dec 30, 2017 at 08:42:41AM +0100, Willem de Bruijn wrote:
[...]

> > Somewhat tangential, but any PF_PACKET socket can set this
> > magic gso_size value in its virtio_net_hdr, so if it is assumed to
> > be an SCTP GSO specific option, setting it for a TCP GSO packet
> > may also cause unexpected results.
>

> It seems virtio_net could use more sanity checks. When PACKET_VNET_HDR
> is used, it will end up calling:
> tpacket_rcv() {
> ...
> if (do_vnet) {
> if (virtio_net_hdr_from_skb(skb, h.raw + macoff -
> sizeof(struct virtio_net_hdr),
> vio_le(), true)) {
> spin_lock(&sk->sk_receive_queue.lock);
> goto drop_n_account;
> }
> }
>
> and virtio_net_hdr_from_skb does:
> if (skb_is_gso(skb)) {
> ...
> if (sinfo->gso_type & SKB_GSO_TCPV4)
> hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> else if (sinfo->gso_type & SKB_GSO_TCPV6)
> hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> else
> return -EINVAL;
>

> Meaning that any gso_type other than TCP would be rejected, but this
> SCTP one got through. Seems the header contains a sctp header, but the
> gso_type set was actually pointing to TCP (otherwise it would have
> been rejected). AFAICT if this packet had an ESP header, for example,
> it could have hit esp4_gso_segment. Can you please confirm this?

I added:
--- a/net/sctp/offload.c
+++ b/net/sctp/offload.c
@@ -44,6 +44,18 @@ static struct sk_buff *sctp_gso_segment(struct sk_buff *skb,
{
struct sk_buff *segs = ERR_PTR(-EINVAL);
struct sctphdr *sh;
+ int fail = 0;
+
+ if (!(skb_shinfo(skb)->gso_type & SKB_GSO_SCTP)) {
+ printk("Bogus gso_type: %x\n", skb_shinfo(skb)->gso_type);
+ fail = 1;
+ }
+ if (skb_shinfo(skb)->gso_size != GSO_BY_FRAGS) {
+ printk("Bogus gso_size: %u\n", skb_shinfo(skb)->gso_size);
+ fail = 1;
+ }
+ if (fail)
+ goto out;

sh = sctp_hdr(skb);
if (!pskb_may_pull(skb, sizeof(*sh)))

and with the reproducer, got:
[ 54.255469] Bogus gso_type: 7
[ 54.258801] Bogus gso_size: 63464
[ 54.262532] ------------[ cut here ]------------
[ 54.267703] syz0: caps=(0x00000800000058c1, 0x0000000000000000) len=32 data_len=0 gso_size=63464 gso_type=7 ip_summed0
[ 54.279777] WARNING: CPU: 1 PID: 13005 at /root/linux/net/core/dev.c:2600 skb_warn_bad_offload+0xd6/0xec

gso_type 7 = SKB_GSO_TCPV4 | SKB_GSO_DODGY | SKB_GSO_TCP_ECN
as the warn indicated too.

Once this gets to sctp_gso_segment, it's too late to avoid the
warning. Would be nice if we could somehow filter this earlier in the
process.

Marcelo

[Marcelo Ricardo Leitner]

On Sat, Dec 30, 2017 at 08:42:41AM +0100, Willem de Bruijn wrote:

It seems virtio_net could use more sanity checks. When PACKET_VNET_HDR
is used, it will end up calling:
tpacket_rcv() {
...
if (do_vnet) {
if (virtio_net_hdr_from_skb(skb, h.raw + macoff -
sizeof(struct virtio_net_hdr),
vio_le(), true)) {
spin_lock(&sk->sk_receive_queue.lock);
goto drop_n_account;
}
}

and virtio_net_hdr_from_skb does:
if (skb_is_gso(skb)) {
...
if (sinfo->gso_type & SKB_GSO_TCPV4)
hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
else if (sinfo->gso_type & SKB_GSO_TCPV6)
hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
else
return -EINVAL;

Meaning that any gso_type other than TCP would be rejected, but this
SCTP one got through. Seems the header contains a sctp header, but the
gso_type set was actually pointing to TCP (otherwise it would have
been rejected). AFAICT if this packet had an ESP header, for example,
it could have hit esp4_gso_segment. Can you please confirm this?

I don't know of anywhere in the stack validating if the gso_type
matches the header that actually is in there.

The fix you mentioned is a good start, we want that one way or
another, but I'm afraid this bug is bigger than sctp.

Marcelo

[Willem de Bruijn]

> Good point. Packet sockets require CAP_NET_RAW, but this is also
> taken for virtio, so we probably want more stringent entry tests here.

That would be something like

#include <linux/if_vlan.h>
+#include <linux/skbuff.h>
#include <uapi/linux/virtio_net.h>
+#include <net/flow_dissector.h>

static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
const struct virtio_net_hdr *hdr,
@@ -12,14 +14,27 @@ static inline int virtio_net_hdr_to_skb(struct sk_buff *skb,
unsigned int gso_type = 0;

if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
+ struct flow_keys flow = { .basic = {0} };
+
+ if (!skb_flow_dissect(skb, &flow_keys_buf_dissector, &flow, 0))
+ return -EINVAL;
+
switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
case VIRTIO_NET_HDR_GSO_TCPV4:
+ if (flow.basic.n_proto != htons(ETH_P_IP) ||
+ flow.basic.ip_proto != IPPROTO_TCP)
+ return -EINVAL;
gso_type = SKB_GSO_TCPV4;
break;
case VIRTIO_NET_HDR_GSO_TCPV6:
+ if (flow.basic.n_proto != htons(ETH_P_IPV6) ||
+ flow.basic.ip_proto != IPPROTO_TCP)
+ return -EINVAL;
gso_type = SKB_GSO_TCPV6;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+ if (flow.basic.ip_proto != IPPROTO_UDP)
+ return -EINVAL;
gso_type = SKB_GSO_UDP;
break;
default:

but I think we can block these packets without adding a flow dissector
call for each untrusted packet (SKB_GSO_DODGY).

> The alternative to harden the segmentation code itself with a gso_type
> sanity check in every gso callback is more work and fragile.

Actually, changes just to inet_gso_segment and ipv6_gso_segment
will suffice:

bool udpfrag = false, fixedid = false, gso_partial, encap;

struct sk_buff *segs = ERR_PTR(-EINVAL);

+ unsigned int offset = 0, gso_type;
const struct net_offload *ops;
- unsigned int offset = 0;
struct iphdr *iph;
int proto, tot_len;
int nhoff;
@@ -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 analogous for IPv6. For a real patch I would deduplicate this
logic between them and move it to a separate helper function
(in a header file, then).

[Willem de Bruijn]

This approach would also need an skb->protocol check either in
virtio_net_hdr_to_skb or skb_mac_gso_segment.

[Willem de Bruijn]

It turns out that all paths already call skb_probe_transport_header,
which also does flow dissection, so has the necessary preconditions
for dissection checked: skb->protocol and network header have been
initialized.

I posted http://patchwork.ozlabs.org/patch/861874/ which adds
gso type and protocol tests based on that flow dissector pass.

Flow dissection was short circuited when csum offload already
set the transport header, so this does add a flow dissector call
to gso packets in practice, even if the datapath is not new.