Messages in this thread Patch in this message | | | From | Willem de Bruijn <> | Date | Tue, 2 Jan 2018 16:48:17 +0100 | 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).
| |