Re: general protection fault in skb_segment

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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