* [PATCH] vhost_net: fix double fget()
@ 2022-05-16 8:42 Jason Wang
2022-05-16 8:44 ` Michael S. Tsirkin
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2022-05-16 8:42 UTC (permalink / raw)
To: viro, mst, jasowang, kvm
Cc: virtualization, netdev, linux-kernel, linux-fsdevel, ebiggers, davem
From: Al Viro <viro@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@zeniv.linux.org.uk>
Signed-off-by: Jason Wang <jasowang@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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost_net: fix double fget()
2022-05-16 8:42 [PATCH] vhost_net: fix double fget() Jason Wang
@ 2022-05-16 8:44 ` Michael S. Tsirkin
2022-05-17 22:00 ` Al Viro
0 siblings, 1 reply; 5+ messages in thread
From: Michael S. Tsirkin @ 2022-05-16 8:44 UTC (permalink / raw)
To: Jason Wang
Cc: viro, kvm, virtualization, netdev, linux-kernel, linux-fsdevel,
ebiggers, davem
On Mon, May 16, 2022 at 04:42:13PM +0800, Jason Wang wrote:
> From: Al Viro <viro@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@zeniv.linux.org.uk>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Acked-by: Michael S. Tsirkin <mst@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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost_net: fix double fget()
2022-05-16 8:44 ` Michael S. Tsirkin
@ 2022-05-17 22:00 ` Al Viro
2022-05-18 4:10 ` Jason Wang
2022-05-18 5:22 ` Michael S. Tsirkin
0 siblings, 2 replies; 5+ messages in thread
From: Al Viro @ 2022-05-17 22:00 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel,
linux-fsdevel, ebiggers, davem
On Mon, May 16, 2022 at 04:44:19AM -0400, Michael S. Tsirkin wrote:
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> and this is stable material I guess.
It is, except that commit message ought to be cleaned up. Something
along the lines of
----
Fix double fget() in vhost_net_set_backend()
Descriptor table is a shared resource; two fget() on the same descriptor
may return different struct file references. get_tap_ptr_ring() is
called after we'd found (and pinned) the socket we'll be using and it
tries to find the private tun/tap data structures associated with it.
Redoing the lookup by the same file descriptor we'd used to get the
socket is racy - we need to same struct file.
Thanks to Jason for spotting a braino in the original variant of patch -
I'd missed the use of fd == -1 for disabling backend, and in that case
we can end up with sock == NULL and sock != oldsock.
----
Does the above sound sane for commit message? And which tree would you
prefer it to go through? I can take it in vfs.git#fixes, or you could
take it into your tree...
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost_net: fix double fget()
2022-05-17 22:00 ` Al Viro
@ 2022-05-18 4:10 ` Jason Wang
2022-05-18 5:22 ` Michael S. Tsirkin
1 sibling, 0 replies; 5+ messages in thread
From: Jason Wang @ 2022-05-18 4:10 UTC (permalink / raw)
To: Al Viro
Cc: Michael S. Tsirkin, kvm, virtualization, netdev, linux-kernel,
linux-fsdevel, Eric Biggers, davem
On Wed, May 18, 2022 at 6:00 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, May 16, 2022 at 04:44:19AM -0400, Michael S. Tsirkin wrote:
> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > and this is stable material I guess.
>
> It is, except that commit message ought to be cleaned up. Something
> along the lines of
>
> ----
> Fix double fget() in vhost_net_set_backend()
>
> Descriptor table is a shared resource; two fget() on the same descriptor
> may return different struct file references. get_tap_ptr_ring() is
> called after we'd found (and pinned) the socket we'll be using and it
> tries to find the private tun/tap data structures associated with it.
> Redoing the lookup by the same file descriptor we'd used to get the
> socket is racy - we need to same struct file.
>
> Thanks to Jason for spotting a braino in the original variant of patch -
> I'd missed the use of fd == -1 for disabling backend, and in that case
> we can end up with sock == NULL and sock != oldsock.
> ----
>
> Does the above sound sane for commit message?
Yes.
> And which tree would you
> prefer it to go through? I can take it in vfs.git#fixes, or you could
> take it into your tree...
>
Consider Michael gave an ack, it would be fine if you want to take via
your tree.
Thanks
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] vhost_net: fix double fget()
2022-05-17 22:00 ` Al Viro
2022-05-18 4:10 ` Jason Wang
@ 2022-05-18 5:22 ` Michael S. Tsirkin
1 sibling, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2022-05-18 5:22 UTC (permalink / raw)
To: Al Viro
Cc: Jason Wang, kvm, virtualization, netdev, linux-kernel,
linux-fsdevel, ebiggers, davem
On Tue, May 17, 2022 at 10:00:03PM +0000, Al Viro wrote:
> On Mon, May 16, 2022 at 04:44:19AM -0400, Michael S. Tsirkin wrote:
> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > and this is stable material I guess.
>
> It is, except that commit message ought to be cleaned up. Something
> along the lines of
>
> ----
> Fix double fget() in vhost_net_set_backend()
>
> Descriptor table is a shared resource; two fget() on the same descriptor
> may return different struct file references. get_tap_ptr_ring() is
> called after we'd found (and pinned) the socket we'll be using and it
> tries to find the private tun/tap data structures associated with it.
> Redoing the lookup by the same file descriptor we'd used to get the
> socket is racy - we need to same struct file.
>
> Thanks to Jason for spotting a braino in the original variant of patch -
> I'd missed the use of fd == -1 for disabling backend, and in that case
> we can end up with sock == NULL and sock != oldsock.
> ----
>
> Does the above sound sane for commit message? And which tree would you
> prefer it to go through? I can take it in vfs.git#fixes, or you could
> take it into your tree...
Acked-by: Michael S. Tsirkin <mst@redhat.com>
for the new message and merging through your tree.
--
MST
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-05-18 5:23 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-16 8:42 [PATCH] vhost_net: fix double fget() Jason Wang
2022-05-16 8:44 ` Michael S. Tsirkin
2022-05-17 22:00 ` Al Viro
2022-05-18 4:10 ` Jason Wang
2022-05-18 5:22 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).