diff mbox series

[v2,3/4] vhost_net: validate virtio_net_hdr only if it exists

Message ID 20210622161533.1214662-3-dwmw2@infradead.org (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: tun: fix tun_xdp_one() for IFF_TUN mode | expand

Commit Message

David Woodhouse June 22, 2021, 4:15 p.m. UTC 
From: David Woodhouse <dwmw@amazon.co.uk>

When the underlying socket doesn't handle the virtio_net_hdr, the
existing code in vhost_net_build_xdp() would attempt to validate stack
noise, by copying zero bytes into the local copy of the header and then
validating that. Skip the whole pointless pointer arithmetic and partial
copy (of zero bytes) in this case.

Fixes: 0a0be13b8fe2 ("vhost_net: batch submitting XDP buffers to underlayer sockets")
Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 drivers/vhost/net.c | 43 ++++++++++++++++++++++---------------------
 1 file changed, 22 insertions(+), 21 deletions(-)

Comments

Jason Wang June 23, 2021, 3:48 a.m. UTC | #1 
在 2021/6/23 上午12:15, David Woodhouse 写道:
> From: David Woodhouse <dwmw@amazon.co.uk>
>
> When the underlying socket doesn't handle the virtio_net_hdr, the
> existing code in vhost_net_build_xdp() would attempt to validate stack
> noise, by copying zero bytes into the local copy of the header and then
> validating that. Skip the whole pointless pointer arithmetic and partial
> copy (of zero bytes) in this case.
>
> Fixes: 0a0be13b8fe2 ("vhost_net: batch submitting XDP buffers to underlayer sockets")
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   drivers/vhost/net.c | 43 ++++++++++++++++++++++---------------------
>   1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index df82b124170e..1e3652eb53af 100644
> --- 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,31 @@ 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) {
> +		struct virtio_net_hdr *gso = &hdr->gso;
> +
> +		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;
> +
> +		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;
> +		}
> +		len -= sock_hlen;
>   	}
>   
> -	len -= sock_hlen;
>   	copied = copy_page_from_iter(alloc_frag->page,
>   				     alloc_frag->offset + pad,
>   				     len, from);
diff mbox series

Patch

diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index df82b124170e..1e3652eb53af 100644
--- 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,31 @@  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) {
+		struct virtio_net_hdr *gso = &hdr->gso;
+
+		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;
+
+		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;
+		}
+		len -= sock_hlen;
 	}
 
-	len -= sock_hlen;
 	copied = copy_page_from_iter(alloc_frag->page,
 				     alloc_frag->offset + pad,
 				     len, from);