Re: general protection fault in skb_segment
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: 2018-01-02 15:49:02
Also in:
linux-sctp, lkml
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).