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

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, May 16, 2022 at 12:17:56PM +0800, Jason Wang wrote:
> 
> 在 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.

Can you cook up a correct patch then please?

> Other looks fine.
> 
> Thanks
> 
> 
> >   		oldubufs = nvq->ubufs;
> >   		nvq->ubufs = ubufs;
> >