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 |
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;
在 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; >
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; > >
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 --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;
[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> ---