Re: general protection fault in skb_segment

From: Willem de Bruijn
Date: Tue Jan 02 2018 - 10:49:05 EST


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