On Mon, May 16, 2022 at 04:42:13PM +0800, Jason Wang wrote: > From: Al Viro <[email protected]> > > 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 <[email protected]> > Signed-off-by: Jason Wang <[email protected]>
Acked-by: Michael S. Tsirkin <[email protected]> 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 _______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
