diff mbox series

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

Message ID YoBzzxlYHYXEP3qj@zeniv-ca.linux.org.uk (mailing list archive)
State New, archived
Headers show
Series [BUG] double fget() in vhost/net (was Re: [PATCH] vfs: move fdput() to right place in ksys_sync_file_range()) | expand

Commit Message

Al Viro May 15, 2022, 3:30 a.m. UTC 
[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>
---

Comments

Michael S. Tsirkin May 15, 2022, 4:14 p.m. UTC | #1 
On Sun, May 15, 2022 at 03:30:23AM +0000, Al Viro wrote:
> [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>

Suspect you are right, didn't test yet. Jason?

> ---
> 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);
>  
>  		oldubufs = nvq->ubufs;
>  		nvq->ubufs = ubufs;
Jason Wang May 16, 2022, 4:17 a.m. UTC | #2 
在 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;
>
Michael S. Tsirkin May 16, 2022, 7:54 a.m. UTC | #3 
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;
> >
Jason Wang May 16, 2022, 8:42 a.m. UTC | #4 
On Mon, May 16, 2022 at 3:54 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> 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?

Sent.

Thanks

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

Patch

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);
 
 		oldubufs = nvq->ubufs;
 		nvq->ubufs = ubufs;