From: Willem de Bruijn <will...@google.com>
Validate gso packet type and headers on kernel entry. Reuse the info
gathered by skb_probe_transport_header.
Syzbot found two bugs by passing bad gso packets in packet sockets.
Untrusted user packets are limited to a small set of gso types in
virtio_net_hdr_to_skb. But segmentation occurs on packet contents.
Syzkaller was able to enter gso callbacks that are not hardened
against untrusted user input.
User packets can also have corrupted headers, tripping up segmentation
logic that expects sane packets from the trusted protocol stack.
Hardening all segmentation paths against all bad packets is error
prone and slows down the common path, so validate on kernel entry.
Introduce skb_probe_transport_header_hard to unconditionally probe,
even if skb_partial_csum_set has already set an offset. That is under
user control, so do not trust it. I did not see a measurable change
in TCP_STREAM performance.
Move tpacket probe call after virtio_net_hdr_to_skb has set gso_type.
Fixes: bfd5f4a3d605 ("packet: Add GSO/csum offload support.")
Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
Fixes: f942dc2552b8 ("xen network backend driver")
Link: http://lkml.kernel.org/r/<001a1137452496ffc305617e5...@google.com>
Reported-by: syzbot+fee64147a25aecd48...@syzkaller.appspotmail.com
Signed-off-by: Willem de Bruijn <will...@google.com>
---
I can resubmit as separate patches for packet, virtio, xen. That
is cleaner and easier to review, but more work to backport, I figured.
An alternative fix is to narrowly address the two specific bugs
syzkaller found. The link above sketches a check in inet_gso_segment
for gso_type.
---
drivers/net/tap.c | 5 ++++-
drivers/net/tun.c | 24 +++++++++++----------
drivers/net/xen-netback/netback.c | 5 ++---
include/linux/skbuff.h | 44 +++++++++++++++++++++++++++++++++------
net/packet/af_packet.c | 12 ++++++++---
5 files changed, 66 insertions(+), 24 deletions(-)
diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 0a886fda0129..b0b7dab1d4a8 100644
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -715,7 +715,10 @@ static ssize_t tap_get_user(struct tap_queue *q, struct
msghdr *m,
goto err_kfree;
}
- skb_probe_transport_header(skb, ETH_HLEN);
+ if (!skb_probe_transport_header_hard(skb, ETH_HLEN)) {
+ err = -EINVAL;
+ goto err;
+ }
/* Move network header to the right position for VLAN tagged packets */
if ((skb->protocol == htons(ETH_P_8021Q) ||
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 4f4a842a1c9c..68a0fc55c723 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1670,16 +1670,8 @@ static ssize_t tun_get_user(struct tun_struct *tun,
struct tun_file *tfile,
}
}
- if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun))) {
- this_cpu_inc(tun->pcpu_stats->rx_frame_errors);
- kfree_skb(skb);
- if (frags) {
- tfile->napi.skb = NULL;
- mutex_unlock(&tfile->napi_mutex);
- }
-
- return -EINVAL;
- }
+ if (virtio_net_hdr_to_skb(skb, &gso, tun_is_little_endian(tun)))
+ goto frame_error;
switch (tun->flags & TUN_TYPE_MASK) {
case IFF_TUN:
@@ -1721,7 +1713,8 @@ static ssize_t tun_get_user(struct tun_struct *tun,
struct tun_file *tfile,
}
skb_reset_network_header(skb);
- skb_probe_transport_header(skb, 0);
+ if (!skb_probe_transport_header_hard(skb, 0))
+ goto frame_error;
if (skb_xdp) {
struct bpf_prog *xdp_prog;
@@ -1785,6 +1778,15 @@ static ssize_t tun_get_user(struct tun_struct *tun,
struct tun_file *tfile,
tun_flow_update(tun, rxhash, tfile);
return total_len;
+
+frame_error:
+ this_cpu_inc(tun->pcpu_stats->rx_frame_errors);
+ kfree_skb(skb);
+ if (frags) {
+ tfile->napi.skb = NULL;
+ mutex_unlock(&tfile->napi_mutex);
+ }
+ return -EINVAL;
}
static ssize_t tun_chr_write_iter(struct kiocb *iocb, struct iov_iter *from)
diff --git a/drivers/net/xen-netback/netback.c
b/drivers/net/xen-netback/netback.c
index a27daa23c9dc..ae59a96bc11b 100644
--- a/drivers/net/xen-netback/netback.c
+++ b/drivers/net/xen-netback/netback.c
@@ -1159,7 +1159,8 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
skb->protocol = eth_type_trans(skb, skb->dev);
skb_reset_network_header(skb);
- if (checksum_setup(queue, skb)) {
+ if (checksum_setup(queue, skb) ||
+ !skb_probe_transport_header_hard(skb, 0)) {
netdev_dbg(queue->vif->dev,
"Can't setup checksum in net_tx_action\n");
/* We have to set this flag to trigger the callback */
@@ -1169,8 +1170,6 @@ static int xenvif_tx_submit(struct xenvif_queue *queue)
continue;
}
- skb_probe_transport_header(skb, 0);
-
/* If the packet is GSO then we will have just set up the
* transport header offset in checksum_setup so it's now
* straightforward to calculate gso_segs.
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a38c80e9f91e..0d931feb236f 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -37,6 +37,7 @@
#include <linux/sched/clock.h>
#include <net/flow_dissector.h>
#include <linux/splice.h>
+#include <linux/in.h>
#include <linux/in6.h>
#include <linux/if_packet.h>
#include <net/flow.h>
@@ -2335,17 +2336,48 @@ static inline void skb_pop_mac_header(struct sk_buff
*skb)
skb->mac_header = skb->network_header;
}
-static inline void skb_probe_transport_header(struct sk_buff *skb,
- const int offset_hint)
+static inline bool skb_validate_dodgy_gso(struct sk_buff *skb,
+ struct flow_keys *keys)
+{
+ switch (skb_shinfo(skb)->gso_type) {
+ case 0:
+ return true;
+ case SKB_GSO_TCPV4 | SKB_GSO_DODGY:
+ case SKB_GSO_TCPV4 | SKB_GSO_DODGY | SKB_GSO_TCP_ECN:
+ return keys->basic.n_proto == htons(ETH_P_IP) &&
+ keys->basic.ip_proto == IPPROTO_TCP;
+ case SKB_GSO_TCPV6 | SKB_GSO_DODGY:
+ case SKB_GSO_TCPV6 | SKB_GSO_DODGY | SKB_GSO_TCP_ECN:
+ return keys->basic.n_proto == htons(ETH_P_IPV6) &&
+ keys->basic.ip_proto == IPPROTO_TCP;
+ case SKB_GSO_UDP | SKB_GSO_DODGY:
+ return keys->basic.ip_proto == IPPROTO_UDP;
+ default:
+ return false;
+ }
+}
+
+static inline bool skb_probe_transport_header_hard(struct sk_buff *skb,
+ const int offset_hint)
{
struct flow_keys keys;
- if (skb_transport_header_was_set(skb))
- return;
- else if (skb_flow_dissect_flow_keys(skb, &keys, 0))
+ if (skb_flow_dissect_flow_keys(skb, &keys, 0)) {
skb_set_transport_header(skb, keys.control.thoff);
- else
+ return skb_validate_dodgy_gso(skb, &keys);
+ } else {
skb_set_transport_header(skb, offset_hint);
+ return !skb_shinfo(skb)->gso_type;
+ }
+}
+
+static inline void skb_probe_transport_header(struct sk_buff *skb,
+ const int offset_hint)
+{
+ if (skb_transport_header_was_set(skb))
+ return;
+
+ skb_probe_transport_header_hard(skb, offset_hint);
}
static inline void skb_mac_header_rebuild(struct sk_buff *skb)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index da215e5c1399..e30b8c43d3bc 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2542,8 +2542,6 @@ static int tpacket_fill_skb(struct packet_sock *po,
struct sk_buff *skb,
len = ((to_write > len_max) ? len_max : to_write);
}
- skb_probe_transport_header(skb, 0);
-
return tp_len;
}
@@ -2744,6 +2742,11 @@ static int tpacket_snd(struct packet_sock *po, struct
msghdr *msg)
goto tpacket_error;
}
+ if (!skb_probe_transport_header_hard(skb, 0)) {
+ tp_len = -EINVAL;
+ goto tpacket_error;
+ }
+
skb->destructor = tpacket_destruct_skb;
__packet_set_status(po, ph, TP_STATUS_SENDING);
packet_inc_pending(&po->tx_ring);
@@ -2935,7 +2938,10 @@ static int packet_snd(struct socket *sock, struct msghdr
*msg, size_t len)
len += sizeof(vnet_hdr);
}
- skb_probe_transport_header(skb, reserve);
+ if (!skb_probe_transport_header_hard(skb, reserve)) {
+ err = -EINVAL;
+ goto out_free;
+ }
if (unlikely(extra_len == 4))
skb->no_fcs = 1;
--
2.16.0.rc1.238.g530d649a79-goog
