On Mon, May 16, 2022 at 04:42:13PM +0800, Jason Wang wrote:
> From: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
and this is stable material I guess.
> ---
> drivers/vhost/net.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 28ef323882fb..0bd7d91de792 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -1449,13 +1449,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;
> @@ -1464,7 +1460,6 @@ static struct ptr_ring *get_tap_ptr_ring(int fd)
> goto out;
> ring = NULL;
> out:
> - fput(file);
> return ring;
> }
>
> @@ -1551,8 +1546,12 @@ static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
> r = vhost_net_enable_vq(n, vq);
> if (r)
> goto err_used;
> - if (index == VHOST_NET_VQ_RX)
> - nvq->rx_ring = get_tap_ptr_ring(fd);
> + if (index == VHOST_NET_VQ_RX) {
> + if (sock)
> + nvq->rx_ring = get_tap_ptr_ring(sock->file);
> + else
> + nvq->rx_ring = NULL;
> + }
>
> oldubufs = nvq->ubufs;
> nvq->ubufs = ubufs;
> --
> 2.25.1
