Re: [PATCH 3/4] virtio_console: avoid NULL portdev dereference in in_intr()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly introduced `!port->portdev` check in `in_intr()` suffers from a Time-Of-Check to Time-Of-Use (TOCTOU) race condition and fails to robustly prevent the NULL pointer dereference.
- [Medium] Bailing out early from `in_intr()` without consuming the used virtqueue buffers leaves stale data in the queue, contaminating future port instances that reuse the same ID.

Pre-existing issues:
- [High] The patch fails to address a fundamental Use-After-Free (UAF) vulnerability in `in_intr()` and `out_intr()`. The helper `find_port_by_vq()` returns a `port` without taking a reference count, allowing concurrent hot-unplug to free the `port` while interrupt handlers are executing.
--

--- Patch [3]: [PATCH 3/4] virtio_console: avoid NULL portdev dereference in in_intr() ---
commit 53028db078d1834cd8a61fa35f5ddb4495c447b8
Author: Hari Mishal <harimishal1@gmail.com>

virtio_console: avoid NULL portdev dereference in in_intr()

This commit attempts to fix a null pointer dereference in in_intr() caused
when a port is hot-unplugged. It adds an early exit check for portdev being
null under the inbuf_lock to ignore further data during the unplug window.

> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 198b973141680..faef362dae853 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
[ ... ]
> @@ -1720,6 +1720,11 @@ static void in_intr(struct virtqueue *vq)
>  	}
>  
>  	spin_lock_irqsave(&port->inbuf_lock, flags);

[Severity: High]
This is a pre-existing issue, but does find_port_by_vq() return the port
pointer without acquiring a reference via kref_get()? 

If unplug_port() runs concurrently, can it drop the final reference and free
the port structure right before we try to take the lock here?

CPU0                                 CPU1
in_intr()                            unplug_port()
  port = find_port_by_vq(...)
                                       kref_put(&port->kref, remove_port);
  spin_lock_irqsave(...) 

Would this result in a use-after-free on the port structure?

> +	if (!port->portdev) {

[Severity: High]
Does this check reliably prevent a null pointer dereference? 

Since unplug_port() sets port->portdev = NULL locklessly outside of
inbuf_lock, could a concurrent unplug set it to null immediately after this
check passes?

Because get_inbuf() is called next, might the compiler reload port->portdev
from memory for the later is_rproc_serial(port->portdev->vdev) check,
crashing on the newly null pointer?

> +		/* Port is being unplugged, ignore further data. */
> +		spin_unlock_irqrestore(&port->inbuf_lock, flags);
> +		return;

[Severity: Medium]
Does returning here without consuming the virtqueue buffers leave stale data
in the queue? 

Since virtqueues persist across hot-unplugs for the same port ID, could a
newly hot-plugged port reuse this virtqueue and read leaked data from the
previous session? 

Should this path consume the buffers, perhaps by calling
flush_bufs(vq, false), before returning?

> +	}
>  	port->inbuf = get_inbuf(port);
>  
>  	/*

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