[PATCH] virtio_console: guard read-side virtqueue access during unplug

[PATCH] virtio_console: guard read-side virtqueue access during unplug

[Yaohan Li]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2761 bytes --]

The virtio-console write path has an unplug fence, but the read path
can still access port->in_vq after device removal has destroyed its
virtqueues. port_has_data() and fill_readbuf() take inbuf_lock without
checking whether the owning port device has gone away.

Clear portdev while holding inbuf_lock during unplug, and check it
under that lock before reading from or re-adding a buffer to the
input virtqueue. Discard an input buffer instead of requeuing it if
the port was unplugged during a userspace copy.

A KASAN-enabled QEMU virtio-serial test reproduced the stale
virtqueue access when read raced with device unbind. The same
injection-assisted sequence did not report a use-after-free with
this change applied.

Fixes: 31610434bc352 ("Virtio console driver")
Assisted-by: ZCode:GLM-4
Signed-off-by: Yaohan Li <shunsheng1218@gmail.com>
---
 drivers/char/virtio_console.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 019bcae81af5c..e97e0daa34395 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -541,10 +541,16 @@ static bool port_has_data(struct port *port)
 
 	ret = false;
 	spin_lock_irqsave(&port->inbuf_lock, flags);
+
+	/* Port is going away. The vqs are already gone. */
+	if (!port->portdev)
+		goto out_unlock;
+
 	port->inbuf = get_inbuf(port);
 	if (port->inbuf)
 		ret = true;
 
+out_unlock:
 	spin_unlock_irqrestore(&port->inbuf_lock, flags);
 	return ret;
 }
@@ -725,6 +731,16 @@ static ssize_t fill_readbuf(struct port *port, u8 __user *out_buf,
 		spin_lock_irqsave(&port->inbuf_lock, flags);
 		port->inbuf = NULL;
 
+		/*
+		 * The port may have been unplugged while we were copying
+		 * data out of the buffer: the vqs are then already gone.
+		 */
+		if (!port->portdev) {
+			spin_unlock_irqrestore(&port->inbuf_lock, flags);
+			free_buf(buf, true);
+			return out_count;
+		}
+
 		if (add_inbuf(port->in_vq, buf) < 0)
 			dev_warn(port->dev, "failed add_buf\n");
 
@@ -1568,6 +1584,18 @@ static void unplug_port(struct port *port)
 	 * a close on an open port later from sending a stale control
 	 * message.
 	 */
+	/*
+	 * Null out portdev under inbuf_lock as well, so the read path
+	 * (port_has_data(), fill_readbuf()) cannot race with the vq
+	 * teardown: it checks port->portdev inside the same lock and
+	 * bails out if NULL, preventing any virtqueue access to an
+	 * already torn-down virtqueue — mirroring the outvq_lock fence
+	 * below for the write path.
+	 */
+	spin_lock_irq(&port->inbuf_lock);
+	port->portdev = NULL;
+	spin_unlock_irq(&port->inbuf_lock);
+
 	spin_lock_irq(&port->outvq_lock);
 	port->portdev = NULL;
 	spin_unlock_irq(&port->outvq_lock);
-- 
2.34.1

Re: [PATCH] virtio_console: guard read-side virtqueue access during unplug

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] virtio_console: double free and UAF of port->inbuf in fill_readbuf()
- [Medium] virtio_console: lock gap in unplug_port() leaks port->inbuf on read race
--

commit a643b15852e7b766cdcc04cfb5d89a66c569bc43
Author: Yaohan Li <shunsheng1218@gmail.com>

virtio_console: guard read-side virtqueue access during unplug

This commit adds port->portdev checks under port->inbuf_lock in
port_has_data() and fill_readbuf(), and clears port->portdev under
port->inbuf_lock in unplug_port() to prevent read-side access to input
virtqueues during unplug.

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 019bcae81af5c..e97e0daa34395 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c

[ ... ]

> @@ -725,6 +731,16 @@ static ssize_t fill_readbuf(struct port *port, u8 __user *out_buf,
>  		spin_lock_irqsave(&port->inbuf_lock, flags);
>  		port->inbuf = NULL;
>  
> +		/*
> +		 * The port may have been unplugged while we were copying
> +		 * data out of the buffer: the vqs are then already gone.
> +		 */
> +		if (!port->portdev) {
> +			spin_unlock_irqrestore(&port->inbuf_lock, flags);
> +			free_buf(buf, true);
> +			return out_count;
> +		}

[Severity: Critical]
Can this free_buf(buf, true) call cause a double-free of buf when
fill_readbuf() races with unplug_port()?

In fill_readbuf(), port_has_data(port) acquires port->inbuf_lock, sets
port->inbuf = get_inbuf(port), and drops port->inbuf_lock while leaving
port->inbuf pointing to buf during the unlocked copy_to_user() or memcpy():

drivers/char/virtio_console.c:fill_readbuf() {
	if (!out_count || !port_has_data(port))
		return 0;

	buf = port->inbuf;
	out_count = min(out_count, buf->len - buf->offset);

	if (to_user) {
		ssize_t ret;

		ret = copy_to_user(out_buf, buf->buf + buf->offset, out_count);
		...
	}
	...
	buf->offset += out_count;
}

If unplug_port() runs concurrently while fill_readbuf() is unlocked, it calls
remove_port_data(port) -> discard_port_data(port) under port->inbuf_lock
before setting port->portdev = NULL:

drivers/char/virtio_console.c:discard_port_data() {
	if (!port->portdev) {
		/* Device has been unplugged.  vqs are already gone. */
		return;
	}
	buf = get_inbuf(port);

	err = 0;
	while (buf) {
		port->stats.bytes_discarded += buf->len - buf->offset;
		if (add_inbuf(port->in_vq, buf) < 0) {
			err++;
			free_buf(buf, false);
		}
		port->inbuf = NULL;
		buf = get_inbuf(port);
	}
}

Because port->inbuf still holds buf during the copy, get_inbuf(port) in
discard_port_data() returns that same active buf and either frees it via
free_buf(buf, false) (when port->in_vq is broken during device removal) or
re-queues it to port->in_vq (where remove_vqs() detaches and frees it), while
copy_to_user() is still reading from buf->buf or before fill_readbuf() loads
buf = port->inbuf (which is now NULL).

When fill_readbuf() finishes copying the buffer, it re-acquires
port->inbuf_lock, sees !port->portdev, and calls free_buf(buf, true) on the
same buf a second time.

> @@ -1568,6 +1584,18 @@ static void unplug_port(struct port *port)
>  	 * a close on an open port later from sending a stale control
>  	 * message.
>  	 */
> +	/*
> +	 * Null out portdev under inbuf_lock as well, so the read path
> +	 * (port_has_data(), fill_readbuf()) cannot race with the vq
> +	 * teardown: it checks port->portdev inside the same lock and
> +	 * bails out if NULL, preventing any virtqueue access to an
> +	 * already torn-down virtqueue — mirroring the outvq_lock fence
> +	 * below for the write path.
> +	 */
> +	spin_lock_irq(&port->inbuf_lock);
> +	port->portdev = NULL;
> +	spin_unlock_irq(&port->inbuf_lock);

[Severity: Medium]
Does clearing port->portdev in a separate lock acquisition after
remove_port_data(port) leave a race window that can leak port->inbuf?

In unplug_port(), remove_port_data(port) acquires port->inbuf_lock, runs
discard_port_data(port), and drops port->inbuf_lock before unplug_port()
re-acquires port->inbuf_lock here to set port->portdev = NULL:

drivers/char/virtio_console.c:remove_port_data() {
	spin_lock_irq(&port->inbuf_lock);
	/* Remove unused data this port might have received. */
	discard_port_data(port);
	spin_unlock_irq(&port->inbuf_lock);

	spin_lock_irq(&port->outvq_lock);
	reclaim_consumed_buffers(port);
	spin_unlock_irq(&port->outvq_lock);
}

If a concurrent reader calls port_has_data(port) in the gap after
discard_port_data(port) unlocks port->inbuf_lock and before port->portdev is
set to NULL, port_has_data(port) sees port->portdev != NULL and detaches a
buffer from port->in_vq into port->inbuf.

Once port->portdev is set to NULL, if port_fops_read() exits on
!port->guest_connected or fill_readbuf() only partially consumes the buffer
(or fails with -EFAULT), that detached buffer remains in port->inbuf and
discard_port_data() in port_fops_release() will bail out on !port->portdev
without freeing it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179378543196.71890.7555371342727156729@gmail.com?part=1