[PATCH 0/4] virtio: validate device-reported values across drivers

[PATCH 0/4] virtio: validate device-reported values across drivers

[Hari Mishal]

These four patches harden virtio-mem, virtio_input, and virtio_console 
against untrusted values coming from the device/backend side of the virtio
interface. The fourth fixes a use-after-free in virtio_console caused by a
missing refcount.

Hari Mishal (4):
  virtio-mem: validate device-reported block size
  virtio_input: validate device-reported multitouch slot count
  virtio_console: avoid NULL portdev dereference in in_intr()
  virtio_console: take a kref in find_port_by_vq() to fix port UAF

 drivers/char/virtio_console.c | 18 +++++++++++++++++-
 drivers/virtio/virtio_input.c |  4 ++++
 drivers/virtio/virtio_mem.c   |  7 +++++++
 3 files changed, 28 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/4] virtio-mem: validate device-reported block size

[Hari Mishal]

The device_block_size read from the virtio-mem config space is used
as a divisor and also in ALIGN_DOWN() further down the code path in
the driver without further validation. A zero value leads to a division
by zero, and a non-power-of-two value corrupts the ALIGN_DOWN() bitmask
arithmetic leading to a misreporting of guest usable guest ram, post
crash. Reject both at init time instead of trusting the device.

Assisted-by: gkh_clanker:t1000
Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
 drivers/virtio/virtio_mem.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 11c441501582..43d12ec7c323 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -2847,6 +2847,13 @@ static int virtio_mem_init(struct virtio_mem *vm)
 			&vm->plugged_size);
 	virtio_cread_le(vm->vdev, struct virtio_mem_config, block_size,
 			&vm->device_block_size);
+	if (!vm->device_block_size ||
+	    !is_power_of_2(vm->device_block_size)) {
+		dev_err(&vm->vdev->dev,
+			"invalid device block size: 0x%llx\n",
+			(unsigned long long)vm->device_block_size);
+		return -EINVAL;
+	}
 	virtio_cread_le(vm->vdev, struct virtio_mem_config, node_id,
 			&node_id);
 	vm->nid = virtio_mem_translate_node_id(vm, node_id);
-- 
2.43.0

Re: [PATCH 1/4] virtio-mem: validate device-reported block size

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing minimum threshold check for `device_block_size` leads to silent truncation of `nb_vm_blocks` to 16 bits, causing guest memory corruption.
--

--- Patch [1]: [PATCH 1/4] virtio-mem: validate device-reported block size ---
commit 1886b5144897e316d9afb5456f7874735bcb4c19
Author: Hari Mishal <harimishal1@gmail.com>

virtio-mem: validate device-reported block size

This commit adds validation to ensure the device-reported block size is
non-zero and a power of two, preventing division by zero and miscalculated
alignments.

> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index 11c4415015829..43d12ec7c3230 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -2847,6 +2847,13 @@ static int virtio_mem_init(struct virtio_mem *vm)
>  			&vm->plugged_size);
>  	virtio_cread_le(vm->vdev, struct virtio_mem_config, block_size,
>  			&vm->device_block_size);
> +	if (!vm->device_block_size ||
> +	    !is_power_of_2(vm->device_block_size)) {
> +		dev_err(&vm->vdev->dev,
> +			"invalid device block size: 0x%llx\n",
> +			(unsigned long long)vm->device_block_size);
> +		return -EINVAL;
> +	}

[Severity: High]
This is a pre-existing issue, but does this validation also need to enforce
a minimum threshold for device_block_size?

If a small power-of-two value like 16 or 32 is provided by the device, the
calculated block count can easily exceed 65535.

For example, a minimum sub-block size of 2MB divided by 16 yields 131,072
blocks. When this is passed to virtio_mem_send_plug_request(), the value is
silently truncated to 16 bits:

drivers/virtio/virtio_mem.c:virtio_mem_send_plug_request() {
    ...
    .u.plug.nb_blocks = cpu_to_virtio16(vm->vdev, nb_vm_blocks),
    ...
}

This truncation also happens in virtio_mem_send_unplug_request() and
virtio_mem_send_state_request() via the .u.unplug.nb_blocks and
.u.state.nb_blocks assignments.

Could this lead to the host processing a truncated request (e.g., plugging 0
blocks) while the guest kernel proceeds to add the full memory region,
causing a crash when the unbacked memory is accessed?

>  	virtio_cread_le(vm->vdev, struct virtio_mem_config, node_id,
>  			&node_id);
>  	vm->nid = virtio_mem_translate_node_id(vm, node_id);

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

Re: [PATCH 1/4] virtio-mem: validate device-reported block size

[Michael S. Tsirkin]

On Wed, Jul 15, 2026 at 04:22:40PM +0200, Hari Mishal wrote:
> The device_block_size read from the virtio-mem config space is used
> as a divisor and also in ALIGN_DOWN() further down the code path in
> the driver without further validation. A zero value leads to a division
> by zero, and a non-power-of-two value corrupts the ALIGN_DOWN() bitmask
> arithmetic leading to a misreporting of guest usable guest ram, post
> crash. Reject both at init time instead of trusting the device.
> 
> Assisted-by: gkh_clanker:t1000
> Signed-off-by: Hari Mishal <harimishal1@gmail.com>
> ---
>  drivers/virtio/virtio_mem.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index 11c441501582..43d12ec7c323 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -2847,6 +2847,13 @@ static int virtio_mem_init(struct virtio_mem *vm)
>  			&vm->plugged_size);
>  	virtio_cread_le(vm->vdev, struct virtio_mem_config, block_size,
>  			&vm->device_block_size);
> +	if (!vm->device_block_size ||
> +	    !is_power_of_2(vm->device_block_size)) {

0 is not a power of 2, why do we need to check twice?

> +		dev_err(&vm->vdev->dev,
> +			"invalid device block size: 0x%llx\n",
> +			(unsigned long long)vm->device_block_size);
> +		return -EINVAL;
> +	}
>  	virtio_cread_le(vm->vdev, struct virtio_mem_config, node_id,
>  			&node_id);
>  	vm->nid = virtio_mem_translate_node_id(vm, node_id);
> -- 
> 2.43.0

[PATCH 2/4] virtio_input: validate device-reported multitouch slot count

[Hari Mishal]

nslots is derived from the ABS_MT_SLOT maximum reported by the
virtio device. A device could report a bogus maximum (e.g. -1)
making nslots = 0, which input_mt_init_slots() does not reject;
it returns success without allocating any slot storage, silently
leaving the device registered as multitouch capable with no
backing state. Reject non-positive slot counts before calling
input_mt_init_slots().

Assisted-by: gkh_clanker:t1000
Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
 drivers/virtio/virtio_input.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
index deec24e8e682..2cc19782cdd3 100644
--- a/drivers/virtio/virtio_input.c
+++ b/drivers/virtio/virtio_input.c
@@ -312,6 +312,10 @@ static int virtinput_probe(struct virtio_device *vdev)
 
 		if (test_bit(ABS_MT_SLOT, vi->idev->absbit)) {
 			nslots = input_abs_get_max(vi->idev, ABS_MT_SLOT) + 1;
+			if (nslots <= 0) {
+				err = -EINVAL;
+				goto err_mt_init_slots;
+			}
 			err = input_mt_init_slots(vi->idev, nslots, 0);
 			if (err)
 				goto err_mt_init_slots;
-- 
2.43.0

Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent execution of virtqueue interrupts during device teardown causes a use-after-free of the input device.
- [High] If `input_register_device()` fails, the driver deletes virtqueues without first resetting the virtio device, violating the virtio specification.
- [Medium] The driver fails to detach unused buffers from the `vi->evt` queue before deleting the virtqueues, leading to leaked DMA mappings.
- [Medium] Missing null-termination for `vi->name` and `vi->serial` strings copied from the host.
--

--- Patch [2]: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count ---
commit 2551acf6fb14e59620dd29579313834ccc291ba0
Author: Hari Mishal <harimishal1@gmail.com>

virtio_input: validate device-reported multitouch slot count

The commit adds validation for the nslots value derived from the
device-reported ABS_MT_SLOT maximum. It prevents registering a
multitouch device with zero backing slots by rejecting non-positive
slot counts before calling input_mt_init_slots().

> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> index deec24e8e6828..2cc19782cdd33 100644
> --- a/drivers/virtio/virtio_input.c
> +++ b/drivers/virtio/virtio_input.c
> @@ -312,6 +312,10 @@ static int virtinput_probe(struct virtio_device *vdev)
>  
>  		if (test_bit(ABS_MT_SLOT, vi->idev->absbit)) {
>  			nslots = input_abs_get_max(vi->idev, ABS_MT_SLOT) + 1;
> +			if (nslots <= 0) {
> +				err = -EINVAL;
> +				goto err_mt_init_slots;
> +			}
>  			err = input_mt_init_slots(vi->idev, nslots, 0);
>  			if (err)
>  				goto err_mt_init_slots;

[Severity: High]
This is a pre-existing issue, but can concurrent execution of virtqueue
interrupts during device teardown cause a use-after-free of the input device?

Looking at virtinput_remove(), the teardown unregisters the input device
before stopping the hardware and synchronizing interrupts:

drivers/virtio/virtio_input.c:virtinput_remove() {
    ...
    input_unregister_device(vi->idev);
    virtio_reset_device(vdev);
    ...
}

If virtinput_recv_events() runs concurrently with virtinput_remove(), the
interrupt handler temporarily drops vi->lock and calls input_event(vi->idev).

Meanwhile, virtinput_remove() can free vi->idev if there are no active users,
since it doesn't call virtio_reset_device() or del_vqs() until after
unregistering the input device.

[Severity: High]
This is a pre-existing issue, but does the error path in virtinput_probe()
violate the virtio specification by deleting virtqueues without resetting
an active device?

During virtinput_probe(), the device is marked ready and activated:

drivers/virtio/virtio_input.c:virtinput_probe() {
    ...
    virtio_device_ready(vdev);
    vi->ready = true;
    err = input_register_device(vi->idev);
    if (err)
        goto err_input_register;
    ...
}

If input_register_device() fails, the code jumps to the error path and deletes
the virtqueues without first calling virtio_reset_device():

drivers/virtio/virtio_input.c:virtinput_probe() {
    ...
err_input_register:
    spin_lock_irqsave(&vi->lock, flags);
    vi->ready = false;
    spin_unlock_irqrestore(&vi->lock, flags);
err_mt_init_slots:
    input_free_device(vi->idev);
err_input_alloc:
    vdev->config->del_vqs(vdev);
    ...
}

Could this lead to memory corruption or host crashes due to DMA into
freed queues?

[Severity: Medium]
This is a pre-existing issue, but does the driver leak DMA mappings for the
vi->evt queue during teardown?

In virtinput_remove() and virtinput_freeze(), the driver detaches and frees
buffers from the vi->sts queue, but ignores the vi->evt queue:

drivers/virtio/virtio_input.c:virtinput_remove() {
    ...
    while ((buf = virtqueue_detach_unused_buf(vi->sts)) != NULL)
        kfree(buf);
    vdev->config->del_vqs(vdev);
    ...
}

The buffers in vi->evt were mapped for DMA when queued in virtinput_fill_evt().

Could deleting the virtqueues without detaching these unused buffers leave
their DMA mappings active indefinitely?

[Severity: Medium]
This is a pre-existing issue, but does the device name copy lack
null-termination if the host provides a full-length string?

In virtinput_probe(), the host-provided string is copied into the fixed-size
vi->name array:

drivers/virtio/virtio_input.c:virtinput_probe() {
    ...
    size = virtinput_cfg_select(vi, VIRTIO_INPUT_CFG_ID_NAME, 0);
    virtio_cread_bytes(vi->vdev, offsetof(struct virtio_input_config,
                                          u.string),
                       vi->name, min(size, sizeof(vi->name)));
    ...
}

If the host provides exactly 64 bytes of non-null characters, vi->name will
not be null-terminated.

Since this string is passed to vi->idev->name, could a subsequent sysfs read
of the device name read past the array boundary?

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

Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count

[Michael S. Tsirkin]

On Wed, Jul 15, 2026 at 04:22:41PM +0200, Hari Mishal wrote:
> nslots is derived from the ABS_MT_SLOT maximum reported by the
> virtio device. A device could report a bogus maximum (e.g. -1)
> making nslots = 0, which input_mt_init_slots() does not reject;
> it returns success without allocating any slot storage, silently
> leaving the device registered as multitouch capable with no
> backing state.

So let's disable multitouch instead?

> Reject non-positive slot counts before calling
> input_mt_init_slots().
> 
> Assisted-by: gkh_clanker:t1000
> Signed-off-by: Hari Mishal <harimishal1@gmail.com>
> ---
>  drivers/virtio/virtio_input.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
> index deec24e8e682..2cc19782cdd3 100644
> --- a/drivers/virtio/virtio_input.c
> +++ b/drivers/virtio/virtio_input.c
> @@ -312,6 +312,10 @@ static int virtinput_probe(struct virtio_device *vdev)
>  
>  		if (test_bit(ABS_MT_SLOT, vi->idev->absbit)) {
>  			nslots = input_abs_get_max(vi->idev, ABS_MT_SLOT) + 1;
> +			if (nslots <= 0) {
> +				err = -EINVAL;
> +				goto err_mt_init_slots;
> +			}
>  			err = input_mt_init_slots(vi->idev, nslots, 0);
>  			if (err)
>  				goto err_mt_init_slots;
> -- 
> 2.43.0

Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count

[Hari Mishal]

On Wed, Jul 15, 2026 at 5:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jul 15, 2026 at 04:22:41PM +0200, Hari Mishal wrote:
> > nslots is derived from the ABS_MT_SLOT maximum reported by the
> > virtio device. A device could report a bogus maximum (e.g. -1)
> > making nslots = 0, which input_mt_init_slots() does not reject;
> > it returns success without allocating any slot storage, silently
> > leaving the device registered as multitouch capable with no
> > backing state.
>
> So let's disable multitouch instead?
>

So rather than failing the whole probe, just warn and clear
ABS_MT_SLOT from absbit in that case, so the rest of the device
still registers? I took my lead from input_mt_init_slots(), which
has its own internal cap and returns -EINVAL when the device
reports more than 1024 slots.
Shall I modify that case to get the same "warn and disable
multitouch" for consistency, or is a hard failure better there since
it's a different type of bad device data?
Happy to fix it either way! Just want to confirm before sending V2.

Cheers,
Hari

Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count

[Michael S. Tsirkin]

On Wed, Jul 15, 2026 at 06:07:56PM +0200, Hari Mishal wrote:
> On Wed, Jul 15, 2026 at 5:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jul 15, 2026 at 04:22:41PM +0200, Hari Mishal wrote:
> > > nslots is derived from the ABS_MT_SLOT maximum reported by the
> > > virtio device. A device could report a bogus maximum (e.g. -1)
> > > making nslots = 0, which input_mt_init_slots() does not reject;
> > > it returns success without allocating any slot storage, silently
> > > leaving the device registered as multitouch capable with no
> > > backing state.
> >
> > So let's disable multitouch instead?
> >
> 
> So rather than failing the whole probe, just warn and clear
> ABS_MT_SLOT from absbit in that case, so the rest of the device
> still registers? I took my lead from input_mt_init_slots(), which
> has its own internal cap and returns -EINVAL when the device
> reports more than 1024 slots.
> Shall I modify that case to get the same "warn and disable
> multitouch" for consistency, or is a hard failure better there since
> it's a different type of bad device data?

I don't really know enough for sure but generally if the device can
kinda work it's better than not working, and adding
a capability to a device that driver can't use should
generally just ignore the capability, not fail probe.

> Happy to fix it either way! Just want to confirm before sending V2.
> 
> Cheers,
> Hari

Re: [PATCH 2/4] virtio_input: validate device-reported multitouch slot count

[Dmitry Torokhov]

On Wed, Jul 15, 2026 at 12:11:12PM -0400, Michael S. Tsirkin wrote:
> On Wed, Jul 15, 2026 at 06:07:56PM +0200, Hari Mishal wrote:
> > On Wed, Jul 15, 2026 at 5:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Wed, Jul 15, 2026 at 04:22:41PM +0200, Hari Mishal wrote:
> > > > nslots is derived from the ABS_MT_SLOT maximum reported by the
> > > > virtio device. A device could report a bogus maximum (e.g. -1)
> > > > making nslots = 0, which input_mt_init_slots() does not reject;
> > > > it returns success without allocating any slot storage, silently
> > > > leaving the device registered as multitouch capable with no
> > > > backing state.
> > >
> > > So let's disable multitouch instead?
> > >
> > 
> > So rather than failing the whole probe, just warn and clear
> > ABS_MT_SLOT from absbit in that case, so the rest of the device
> > still registers? I took my lead from input_mt_init_slots(), which
> > has its own internal cap and returns -EINVAL when the device
> > reports more than 1024 slots.
> > Shall I modify that case to get the same "warn and disable
> > multitouch" for consistency, or is a hard failure better there since
> > it's a different type of bad device data?
> 
> I don't really know enough for sure but generally if the device can
> kinda work it's better than not working, and adding
> a capability to a device that driver can't use should
> generally just ignore the capability, not fail probe.

What is the failure mode if we keep the ABS_MT_SLOT capability? Does the
kernel crash? And if this can cause crash then we should fix
input_mt_init_slots() to reject requests for 0 slots with -EINVAL.

Thanks.

-- 
Dmitry

[PATCH v2 2/4] virtio_input: validate device-reported multitouch slot count

[Hari Mishal]

nslots is derived from the ABS_MT_SLOT maximum reported by the virtio
device. A device could report a bogus maximum (e.g. -1) making nslots <=
0, and input_mt_init_slots() can independently fail too (e.g. it rejects
slot counts over 1024). Neither case was handled here, and
input_mt_init_slots() failing took down the whole probe, losing the rest
of the input device over a problem isolated to multitouch.

Instead of failing the probe, warn and disable the ABS_MT_SLOT
capability in either case, and let the rest of the device register
normally. This recovery assumes input_mt_init_slots() is called with
flags == 0: with nonzero flags it can fail after already mutating the
input_dev (e.g. via input_set_abs_params()), which this recovery does
not account for.

Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
v2: per review, disable multitouch instead of failing the whole
    probe when the reported slot count is invalid or
    input_mt_init_slots() otherwise fails.

 drivers/virtio/virtio_input.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/virtio/virtio_input.c b/drivers/virtio/virtio_input.c
index deec24e8e682..786f5150724f 100644
--- a/drivers/virtio/virtio_input.c
+++ b/drivers/virtio/virtio_input.c
@@ -312,9 +312,25 @@ static int virtinput_probe(struct virtio_device *vdev)
 
 		if (test_bit(ABS_MT_SLOT, vi->idev->absbit)) {
 			nslots = input_abs_get_max(vi->idev, ABS_MT_SLOT) + 1;
-			err = input_mt_init_slots(vi->idev, nslots, 0);
-			if (err)
-				goto err_mt_init_slots;
+			if (nslots <= 0) {
+				dev_warn(&vdev->dev,
+					 "invalid multitouch slot count %d, disabling multitouch\n",
+					 nslots);
+				__clear_bit(ABS_MT_SLOT, vi->idev->absbit);
+			} else {
+				/*
+				 * flags is 0: input_mt_init_slots() can only fail
+				 * before touching *idev, so the recovery below is
+				 * a full rollback.
+				 */
+				err = input_mt_init_slots(vi->idev, nslots, 0);
+				if (err) {
+					dev_warn(&vdev->dev,
+						 "failed to init %d multitouch slots (%d), disabling multitouch\n",
+						 nslots, err);
+					__clear_bit(ABS_MT_SLOT, vi->idev->absbit);
+				}
+			}
 		}
 	}
 
@@ -331,7 +347,6 @@ static int virtinput_probe(struct virtio_device *vdev)
 	spin_lock_irqsave(&vi->lock, flags);
 	vi->ready = false;
 	spin_unlock_irqrestore(&vi->lock, flags);
-err_mt_init_slots:
 	input_free_device(vi->idev);
 err_input_alloc:
 	vdev->config->del_vqs(vdev);
-- 
2.43.0

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

[Hari Mishal]

A port's virtqueue is not torn down immediately if the port itself is
hot-unplugged (unplug_port() only nulls port->portdev; the vq
callback stays registered until the whole device is removed).
If in_intr() fires for a port in that window it dereferences
port->portdev->vdev via is_rproc_serial(), crashing on the NULL
portdev.

Bail out early when portdev has already been cleared.

Assisted-by: gkh_clanker:t1000
Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
 drivers/char/virtio_console.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 198b97314168..faef362dae85 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);
+	if (!port->portdev) {
+		/* Port is being unplugged, ignore further data. */
+		spin_unlock_irqrestore(&port->inbuf_lock, flags);
+		return;
+	}
 	port->inbuf = get_inbuf(port);
 
 	/*
-- 
2.43.0

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

[sashiko-bot]

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

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

[Hari Mishal]

find_port_by_vq() returns a raw struct port pointer without taking a
reference on it, unlike find_port_by_devt_in_portdev() which does.
find_port_by_vq()'s only two callers, in_intr() and out_intr(), run
as virtqueue interrupt callbacks, entirely independent of and
possibly concurrently with unplug_port(), which itself runs from a
workqueue when the host sends a VIRTIO_CONSOLE_PORT_REMOVE control
message.

unplug_port() removes the port from portdev->ports under ports_lock,
then later drops its last reference with kref_put(), freeing it via
remove_port(). find_port_by_vq() also walks portdev->ports under
ports_lock, so if it finds the port still on the list, the list
removal, and therefore the eventual kref_put(), has not happened yet,
and taking a reference at that point is always safe. Without doing
so, in_intr()/out_intr() can be left holding a pointer to a port that
unplug_port() frees on another core before they are done using it.

Both triggers are host-controlled as the host decides when to send the
PORT_REMOVE control message and when to kick the port's data vq. So a
malicious backend could race the two on purpose, without any guest side
cooperation. The freed object is a generic kmalloc allocation containing
a wait_queue_head_t, which in_intr()/out_intr() pass to
wake_up_interruptible() after touching the stale pointer.
wake_up_interruptible() invokes a function pointer read out of the
wait queue's entries. If the freed slab slot is reclaimed with
attacker influenced content before that call, then this is an arbitrary
function call primitive rather than just undefined behaviour.

Take a reference in find_port_by_vq() while still holding ports_lock,
matching find_port_by_devt_in_portdev(), and release it in in_intr()
and out_intr() once they are done with the port.

Signed-off-by: Hari Mishal <harimishal1@gmail.com>
---
 drivers/char/virtio_console.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index faef362dae85..1b7593684ed9 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -304,6 +304,11 @@ static struct port *find_port_by_id(struct ports_device *portdev, u32 id)
 	return port;
 }
 
+/*
+ * The port object's reference is incremented now and
+ * it is the caller's responsibility to decrement it
+ */
+
 static struct port *find_port_by_vq(struct ports_device *portdev,
 				    struct virtqueue *vq)
 {
@@ -312,8 +317,10 @@ static struct port *find_port_by_vq(struct ports_device *portdev,
 
 	spin_lock_irqsave(&portdev->ports_lock, flags);
 	list_for_each_entry(port, &portdev->ports, list)
-		if (port->in_vq == vq || port->out_vq == vq)
+		if (port->in_vq == vq || port->out_vq == vq) {
+			kref_get(&port->kref);
 			goto out;
+		}
 	port = NULL;
 out:
 	spin_unlock_irqrestore(&portdev->ports_lock, flags);
@@ -1706,6 +1713,7 @@ static void out_intr(struct virtqueue *vq)
 	}
 
 	wake_up_interruptible(&port->waitqueue);
+	kref_put(&port->kref, remove_port);
 }
 
 static void in_intr(struct virtqueue *vq)
@@ -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;
 	}
 	port->inbuf = get_inbuf(port);
@@ -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);
 }
 
 static void control_intr(struct virtqueue *vq)
-- 
2.43.0

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

[sashiko-bot]

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