CVE-2023-1838: [PATCH] vhost_net: fix double fget()

* [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 [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]

---

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®; }

-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 ® 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 [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®; }

-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 ® 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 [email protected]

Signed-off-by: Jason Wang [email protected]

Acked-by: Michael S. Tsirkin [email protected]

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 [email protected] wrote: >

On Mon, May 16, 2022 at 04:44:19AM -0400, Michael S. Tsirkin wrote:

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.

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 [email protected] Signed-off-by: Jason Wang [email protected]

Acked-by: Michael S. Tsirkin [email protected]

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 [email protected] 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).