netdev - Re: general protection fault in skb

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 2 Jan 2018 16:48:17 +0100
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Cc:     syzbot <syzbot+fee64147a25aecd48055@...kaller.appspotmail.com>,
        David Miller <davem@...emloft.net>,
        LKML <linux-kernel@...r.kernel.org>, linux-sctp@...r.kernel.org,
        Network Development <netdev@...r.kernel.org>,
        Neil Horman <nhorman@...driver.com>,
        syzkaller-bugs@...glegroups.com,
        Vladislav Yasevich <vyasevich@...il.com>
Subject: Re: general protection fault in skb_segment

> 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).