vhost_net: remove virtio_net_hdr validation, let tun/tap do it themselves

When the underlying socket isn't configured with a virtio_net_hdr, the
existing code in vhost_net_build_xdp() would attempt to validate
uninitialised data, by copying zero bytes (sock_hlen) into the local
copy of the header and then trying to validate that.

Fixing it is somewhat non-trivial because the tun device might put a
struct tun_pi *before* the virtio_net_hdr, which makes it hard to find.
So just stop messing with someone else's data in vhost_net_build_xdp(),
and let tap and tun validate it for themselves, as they do in the
non-XDP case anyway.

This means that the 'gso' member of struct tun_xdp_hdr can die, leaving
only 'int buflen'.

The socket header of sock_hlen is still copied separately from the
data payload because there may be a gap between them to ensure suitable
alignment of the latter.

Fixes: 0a0be13b8fe2 ("vhost_net: batch submitting XDP buffers to underlayer sockets")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>

diff --git a/drivers/net/tap.c b/drivers/net/tap.c
index 2170a0d3d34ce5271e70b531bbbb1473d1e3a5e8..d1b1f1de374eb52ef390b8aaa56164f1a6914a06 100644 (file)
--- a/drivers/net/tap.c
+++ b/drivers/net/tap.c
@@ -1132,16 +1132,35 @@ static const struct file_operations tap_fops = {
 static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
 {
        struct tun_xdp_hdr *hdr = xdp->data_hard_start;
-       struct virtio_net_hdr *gso = &hdr->gso;
+       struct virtio_net_hdr *gso = NULL;
        int buflen = hdr->buflen;
        int vnet_hdr_len = 0;
        struct tap_dev *tap;
        struct sk_buff *skb;
        int err, depth;
 
-       if (q->flags & IFF_VNET_HDR)
+       if (q->flags & IFF_VNET_HDR) {
                vnet_hdr_len = READ_ONCE(q->vnet_hdr_sz);
+               if (xdp->data != xdp->data_hard_start + sizeof(*hdr) + vnet_hdr_len) {
+                       err = -EINVAL;
+                       goto err;
+               }
+
+               gso = (void *)&hdr[1];
 
+               if ((gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
+                    tap16_to_cpu(q, gso->csum_start) +
+                    tap16_to_cpu(q, gso->csum_offset) + 2 >
+                            tap16_to_cpu(q, gso->hdr_len))
+                       gso->hdr_len = cpu_to_tap16(q,
+                                tap16_to_cpu(q, gso->csum_start) +
+                                tap16_to_cpu(q, gso->csum_offset) + 2);
+
+               if (tap16_to_cpu(q, gso->hdr_len) > xdp->data_end - xdp->data) {
+                       err = -EINVAL;
+                       goto err;
+               }
+       }
        skb = build_skb(xdp->data_hard_start, buflen);
        if (!skb) {
                err = -ENOMEM;
@@ -1155,7 +1174,7 @@ static int tap_get_user_xdp(struct tap_queue *q, struct xdp_buff *xdp)
        skb_reset_mac_header(skb);
        skb->protocol = eth_hdr(skb)->h_proto;
 
-       if (vnet_hdr_len) {
+       if (gso) {
                err = virtio_net_hdr_to_skb(skb, gso, tap_is_little_endian(q));
                if (err)
                        goto err_kfree;

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 9acd448e6dfc768d13e6b8dc1159111d96162d17..1b553f79adb023d8d7ccb9d6e75d29fb34f476f1 100644 (file)
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -2331,6 +2331,7 @@ static int tun_xdp_one(struct tun_struct *tun,
 {
        unsigned int datasize = xdp->data_end - xdp->data;
        struct tun_xdp_hdr *hdr = xdp->data_hard_start;
+       void *tun_hdr = &hdr[1];
        struct virtio_net_hdr *gso = NULL;
        struct bpf_prog *xdp_prog;
        struct sk_buff *skb = NULL;
@@ -2340,8 +2341,22 @@ static int tun_xdp_one(struct tun_struct *tun,
        bool skb_xdp = false;
        struct page *page;
 
-       if (tun->flags & IFF_VNET_HDR)
-               gso = &hdr->gso;
+       if (tun->flags & IFF_VNET_HDR) {
+               gso = tun_hdr;
+               tun_hdr += sizeof(*gso);
+
+               if (tun_hdr > xdp->data) {
+                       atomic_long_inc(&tun->rx_frame_errors);
+                       return -EINVAL;
+               }
+
+               if ((gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
+                   tun16_to_cpu(tun, gso->csum_start) + tun16_to_cpu(tun, gso->csum_offset) + 2 > tun16_to_cpu(tun, gso->hdr_len))
+                       gso->hdr_len = cpu_to_tun16(tun, tun16_to_cpu(tun, gso->csum_start) + tun16_to_cpu(tun, gso->csum_offset) + 2);
+
+               if (tun16_to_cpu(tun, gso->hdr_len) > datasize)
+                       return -EINVAL;
+       }
 
        xdp_prog = rcu_dereference(tun->xdp_prog);
        if (xdp_prog) {
@@ -2389,7 +2404,7 @@ build:
        }
 
        skb_reserve(skb, xdp->data - xdp->data_hard_start);
-       skb_put(skb, xdp->data_end - xdp->data);
+       skb_put(skb, datasize);
 
        if (gso && virtio_net_hdr_to_skb(skb, gso, tun_is_little_endian(tun))) {
                atomic_long_inc(&tun->rx_frame_errors);

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index b92a7144ed90de65768bbc140408b2ac14e74f68..7cae18151c60364fb33db2aa890c7186b901a461 100644 (file)
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -690,7 +690,6 @@ static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
                                             dev);
        struct socket *sock = vhost_vq_get_backend(vq);
        struct page_frag *alloc_frag = &net->page_frag;
-       struct virtio_net_hdr *gso;
        struct xdp_buff *xdp = &nvq->xdp[nvq->batched_xdp];
        struct tun_xdp_hdr *hdr;
        size_t len = iov_iter_count(from);
@@ -715,29 +714,18 @@ static int vhost_net_build_xdp(struct vhost_net_virtqueue *nvq,
                return -ENOMEM;
 
        buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
-       copied = copy_page_from_iter(alloc_frag->page,
-                                    alloc_frag->offset +
-                                    offsetof(struct tun_xdp_hdr, gso),
-                                    sock_hlen, from);
-       if (copied != sock_hlen)
-               return -EFAULT;
-
        hdr = buf;
-       gso = &hdr->gso;
-
-       if ((gso->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) &&
-           vhost16_to_cpu(vq, gso->csum_start) +
-           vhost16_to_cpu(vq, gso->csum_offset) + 2 >
-           vhost16_to_cpu(vq, gso->hdr_len)) {
-               gso->hdr_len = cpu_to_vhost16(vq,
-                              vhost16_to_cpu(vq, gso->csum_start) +
-                              vhost16_to_cpu(vq, gso->csum_offset) + 2);
-
-               if (vhost16_to_cpu(vq, gso->hdr_len) > len)
-                       return -EINVAL;
+       if (sock_hlen) {
+               copied = copy_page_from_iter(alloc_frag->page,
+                                            alloc_frag->offset +
+                                            sizeof(struct tun_xdp_hdr),
+                                            sock_hlen, from);
+               if (copied != sock_hlen)
+                       return -EFAULT;
+
+               len -= sock_hlen;
        }
 
-       len -= sock_hlen;
        copied = copy_page_from_iter(alloc_frag->page,
                                     alloc_frag->offset + pad,
                                     len, from);

diff --git a/include/linux/if_tun.h b/include/linux/if_tun.h
index 8a7debd3f663936877cf01abb716e24cee1b6137..8d78b6bbc228e5bc6519d12a2b7eca400a81a2aa 100644 (file)
--- a/include/linux/if_tun.h
+++ b/include/linux/if_tun.h
@@ -21,7 +21,6 @@ struct tun_msg_ctl {
 
 struct tun_xdp_hdr {
        int buflen;
-       struct virtio_net_hdr gso;
 };
 
 #if defined(CONFIG_TUN) || defined(CONFIG_TUN_MODULE)