Re: [BUG] double fget() in vhost/net (was Re: [PATCH] vfs: move fdput() to right place in ksys_sync_file_range())

From: Jason Wang <jasowang@redhat.com>
To: Al Viro <viro@zeniv.linux.org.uk>, Eric Biggers <ebiggers@kernel.org>
Cc: linux-fsdevel@vger.kernel.org,
	"David S. Miller" <davem@davemloft.net>,
	"Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [BUG] double fget() in vhost/net (was Re: [PATCH] vfs: move fdput() to right place in ksys_sync_file_range())
Date: Mon, 16 May 2022 12:17:56 +0800	[thread overview]
Message-ID: <02ba82ea-727f-1432-226e-ab2eb37d5c29@redhat.com> (raw)
In-Reply-To: <YoBzzxlYHYXEP3qj@zeniv-ca.linux.org.uk>

在 2022/5/15 11:30, Al Viro 写道:
> [tun/tap and vhost folks Cc'd]
>
> here's another piece of code assuming that repeated fget() will yield the
> same opened file: in vhost_net_set_backend() we have
>
>          sock = get_socket(fd);
>          if (IS_ERR(sock)) {
>                  r = PTR_ERR(sock);
>                  goto err_vq;
>          }
>
>          /* start polling new socket */
>          oldsock = vhost_vq_get_backend(vq);
>          if (sock != oldsock) {
> ...
>                  vhost_vq_set_backend(vq, sock);
> ...
>                  if (index == VHOST_NET_VQ_RX)
>                          nvq->rx_ring = get_tap_ptr_ring(fd);
>
> with
> static struct socket *get_socket(int fd)
> {
>          struct socket *sock;
>
>          /* special case to disable backend */
>          if (fd == -1)
>                  return NULL;
>          sock = get_raw_socket(fd);
>          if (!IS_ERR(sock))
>                  return sock;
>          sock = get_tap_socket(fd);
>          if (!IS_ERR(sock))
>                  return sock;
>          return ERR_PTR(-ENOTSOCK);
> }
> and
> static struct ptr_ring *get_tap_ptr_ring(int fd)
> {
>          struct ptr_ring *ring;
>          struct file *file = fget(fd);
>
>          if (!file)
>                  return NULL;
>          ring = tun_get_tx_ring(file);
>          if (!IS_ERR(ring))
>                  goto out;
>          ring = tap_get_ptr_ring(file);
>          if (!IS_ERR(ring))
>                  goto out;
>          ring = NULL;
> out:
>          fput(file);
>          return ring;
> }
>
> Again, there is no promise that fd will resolve to the same thing for
> lookups in get_socket() and in get_tap_ptr_ring().  I'm not familiar
> enough with the guts of drivers/vhost to tell how easy it is to turn
> into attack, but it looks like trouble.  If nothing else, the pointer
> returned by tun_get_tx_ring() is not guaranteed to be pinned down by
> anything - the reference to sock will _usually_ suffice, but that
> doesn't help any if we get a different socket on that second fget().
>
> One possible way to fix it would be the patch below; objections?
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 792ab5f23647..86ea7695241e 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1450,13 +1450,9 @@ static struct socket *get_raw_socket(int fd)
>   	return ERR_PTR(r);
>   }
>   
> -static struct ptr_ring *get_tap_ptr_ring(int fd)
> +static struct ptr_ring *get_tap_ptr_ring(struct file *file)
>   {
>   	struct ptr_ring *ring;
> -	struct file *file = fget(fd);
> -
> -	if (!file)
> -		return NULL;
>   	ring = tun_get_tx_ring(file);
>   	if (!IS_ERR(ring))
>   		goto out;
> @@ -1465,7 +1461,6 @@ static struct ptr_ring *get_tap_ptr_ring(int fd)
>   		goto out;
>   	ring = NULL;
>   out:
> -	fput(file);
>   	return ring;
>   }
>   
> @@ -1553,7 +1548,7 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
>   		if (r)
>   			goto err_used;
>   		if (index == VHOST_NET_VQ_RX)
> -			nvq->rx_ring = get_tap_ptr_ring(fd);
> +			nvq->rx_ring = get_tap_ptr_ring(sock->file);

sock could be NULL if we want to stop the vhost-net.

Other looks fine.

Thanks

>   
>   		oldubufs = nvq->ubufs;
>   		nvq->ubufs = ubufs;
>