Re: [PATCH v3 2/6] virtio_ccw: always take irq_lock in the classic interrupt handler

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The virtio interrupt handlers pass retrieved virtqueue pointers directly to `vring_interrupt()` without a NULL check. A malicious or buggy host can trigger a kernel panic by providing an invalid indicator or interrupt vector.
- [High] Lost configuration change notifications due to clearing the indicator bit after processing.
--

commit 40545ed73d4381a07a7934fddf3ac60ecd1f90d7
Author: Karl Mehltretter <kmehltretter@gmail.com>
Subject: virtio_ccw: always take irq_lock in the classic interrupt handler

This commit unconditionally takes the read lock on vcdev->irq_lock in 
virtio_ccw_int_handler() so the write side can properly synchronize with
the callbacks even when notification hardening is disabled.

> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index b90299205698b..552d779980129 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -1204,13 +1204,11 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,

[ ... ]

>  	for_each_set_bit(i, indicators(vcdev),
>  			 sizeof(*indicators(vcdev)) * BITS_PER_BYTE) {
>  		/* The bit clear must happen before the vring kick. */
> @@ -1219,9 +1217,7 @@ static void virtio_ccw_int_handler(struct ccw_device *cdev,
>  		vq = virtio_ccw_vq_by_ind(vcdev, i);
>  		vring_interrupt(0, vq);

[Severity: High]
This is a pre-existing issue, but is it possible for virtio_ccw_vq_by_ind()
to return a NULL virtqueue pointer if a malicious or buggy host provides an
invalid indicator?

Passing an unvalidated NULL pointer directly to vring_interrupt() can cause a
kernel panic when it is dereferenced inside more_used():

drivers/virtio/virtio_ring.c:vring_interrupt() {
    ...
    if (!more_used(vq)) {
        pr_debug("virtqueue interrupt with no work for %p\n", vq);
    ...
}

Similarly, in virtio_airq_handler(), airq_iv_get_ptr() can return NULL which
is unconditionally passed to vring_interrupt().

Could we add a NULL check before calling the interrupt handler?

>  	}
> -#ifdef CONFIG_VIRTIO_HARDEN_NOTIFICATION
>  	read_unlock(&vcdev->irq_lock);
> -#endif
>  	if (test_bit(0, indicators2(vcdev))) {
>  		virtio_config_changed(&vcdev->vdev);
>  		clear_bit(0, indicators2(vcdev));
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but could this sequence lead to
lost configuration change notifications?

Because indicators2 is shared DMA memory updated by the host, if the host
triggers another configuration change in the window between test_bit() and
clear_bit(), it sets the bit in memory and raises a new interrupt.

However, the guest will then blindly execute clear_bit() and wipe out the
newly set notification flag. When the newly queued interrupt handler runs,
it will find the bit cleared and do nothing, permanently losing the event.

Would using test_and_clear_bit() before calling virtio_config_changed()
close this race window?

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