Re: [PATCH 4/4] virtio_console: take a kref in find_port_by_vq() to fix port UAF

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix: `in_intr()` can still dereference a freed `hvc_struct` (`port->cons.hvc`) due to a race with `unplug_port()`.
- [High] A TOCTOU data race on `port->portdev` between `in_intr()` and `unplug_port()` can lead to a NULL pointer dereference.
- [High] File operations (e.g., `port_fops_read`, `port_fops_write`) can access freed virtqueue and portdev memory if device removal occurs concurrently.
--

--- Patch [4]: [PATCH 4/4] virtio_console: take a kref in find_port_by_vq() to fix port UAF ---
commit 135dcd5e5260baeaa35a46f85c302542d073ddaf
Author: Hari Mishal <harimishal1@gmail.com>

virtio_console: take a kref in find_port_by_vq() to fix port UAF

This commit fixes a use-after-free of the port structure by taking a
kref in find_port_by_vq() under the ports_lock. It matches the behavior
of find_port_by_devt_in_portdev() and safely drops the reference at the
end of the interrupt handlers.

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index faef362dae853..1b7593684ed9d 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c

[ ... ]

> @@ -1723,6 +1731,7 @@ static void in_intr(struct virtqueue *vq)
>  	if (!port->portdev) {
>  		/* Port is being unplugged, ignore further data. */
>  		spin_unlock_irqrestore(&port->inbuf_lock, flags);
> +		kref_put(&port->kref, remove_port);
>  		return;
>  	}

[Severity: High]
This is a pre-existing issue, but is there a time-of-check to time-of-use
(TOCTOU) data race on port->portdev here?

In unplug_port(), port->portdev is locklessly set to NULL at the end of the
function. Concurrently, in_intr() checks if (!port->portdev) while holding
inbuf_lock. If the check passes before unplug_port() sets it to NULL,
in_intr() will proceed and later execute this check:

if (!port->guest_connected && !is_rproc_serial(port->portdev->vdev))

Could this force a reload of port->portdev, read NULL, and cause a kernel
panic when accessing ->vdev?

[ ... ]

> @@ -1756,6 +1765,8 @@ static void in_intr(struct virtqueue *vq)
>  
>  	if (is_console_port(port) && hvc_poll(port->cons.hvc))
>  		hvc_kick();
> +
> +	kref_put(&port->kref, remove_port);
>  }

[Severity: High]
This is a pre-existing issue, but could in_intr() still dereference a freed
hvc_struct due to a race with unplug_port()?

When the host sends a VIRTIO_CONSOLE_PORT_REMOVE control message,
unplug_port() calls hvc_remove(port->cons.hvc), freeing the hvc_struct.
Concurrently, if the host kicks the data virtqueue, in_intr() passes the
portdev check, drops inbuf_lock, and evaluates:

if (is_console_port(port) && hvc_poll(port->cons.hvc))

Since port->cons.hvc is never cleared and this check is lockless, can this
dereference the freed memory and lead to memory corruption?

[Severity: High]
This is also a pre-existing issue, but can file operations like
port_fops_write() access freed virtqueue and portdev memory if device removal
occurs concurrently?

If a user-space process initiates port_fops_write(), it eventually executes:

buf = alloc_buf(port->portdev->vdev, count, 0);

Concurrently, if the host removes the virtio device, virtcons_remove() is
called. It unplugs the ports, calls remove_vqs(portdev), and frees portdev
via kfree(portdev) without waiting for existing open file descriptors to be
closed.

Even with the kref protecting struct port, could the file operation resume,
pass checks, and access port->portdev->vdev or call functions on freed
memory like virtqueue_add_outbuf() on port->out_vq?

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