From: Al Viro <viro at zeniv.linux.org.uk>
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 at zeniv.linux.org.uk>
Signed-off-by: Jason Wang <jasowang at redhat.com>
---
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
On Mon, May 16, 2022 at 04:42:13PM +0800, Jason Wang wrote:> From: Al Viro <viro at zeniv.linux.org.uk> > > 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 at zeniv.linux.org.uk> > Signed-off-by: Jason Wang <jasowang at redhat.com>Acked-by: Michael S. Tsirkin <mst at redhat.com> 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