Re: general protection fault in skb_segment

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

On Sat, Dec 30, 2017 at 08:42:41AM +0100, Willem de Bruijn wrote:
> > 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.

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