Re: [PATCH v3 1/6] virtio_ccw: fix synchronize_cbs() after interrupt fallback

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] virtio_ccw_reset() silently fails to reset the device if ccw allocation fails, risking use-after-free and DMA corruption.
- [High] virtio_ccw_del_vqs silently aborts on memory allocation failure, permanently leaking all virtqueues and their DMA memory.
- [High] Silent allocation failure in virtio_ccw_drop_indicator leaves stale virtqueue pointers in global routing tables, leading to UAF when adapter interrupts fire.
--

commit bd43fb4d1f3ef8003538155a318dcf69a27e4625
Author: Karl Mehltretter <kmehltretter@gmail.com>

virtio_ccw: fix synchronize_cbs() after interrupt fallback

virtio_ccw_synchronize_cbs() assumes the device uses adapter interrupts
when vcdev->airq_info is not NULL. If registering adapter indicators
fails, virtio_ccw_find_vqs() falls back to classic interrupts but can
leave airq_info set. This patch uses vcdev->is_thinint to properly
select the lock.

> diff --git a/drivers/s390/virtio/virtio_ccw.c b/drivers/s390/virtio/virtio_ccw.c
> index bab6cad3fd5ce..b90299205698b 100644
> --- a/drivers/s390/virtio/virtio_ccw.c
> +++ b/drivers/s390/virtio/virtio_ccw.c
> @@ -1062,7 +1062,7 @@ static void virtio_ccw_synchronize_cbs(struct virtio_device *vdev)
>  	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>  	struct airq_info *info = vcdev->airq_info;
>  
> -	if (info) {
> +	if (vcdev->is_thinint && info) {
>  		/*
>  		 * This device uses adapter interrupts: synchronize with
>  		 * vring_interrupt() called by virtio_airq_handler()

[ ... ]

> static void virtio_ccw_drop_indicator(struct virtio_ccw_device *vcdev,
> 				      struct ccw1 *ccw)
> {
> 	int ret;
> 	struct virtio_thinint_area *thinint_area = NULL;
> 	struct airq_info *airq_info = vcdev->airq_info;
> 	dma64_t *indicatorp = NULL;
> 
> 	if (vcdev->is_thinint) {
> 		thinint_area = ccw_device_dma_zalloc(vcdev->cdev,
> 						     sizeof(*thinint_area),
> 						     &ccw->cda);
> 		if (!thinint_area)
> 			return;

[Severity: High]
This is a pre-existing issue, but returning early here on allocation failure
skips deregistering indicators and removing stale pointers from the global
airq_areas array.

If ccw_device_dma_zalloc() fails, the caller virtio_ccw_del_vqs() continues
and frees all virtqueues, while the global airq_areas array retains pointers
to them.

A subsequent adapter interrupt in virtio_airq_handler() could then fetch the
stale pointer:

	vring_interrupt(0, (void *)airq_iv_get_ptr(info->aiv, ai));

and trigger a use-after-free on the freed virtqueue memory.

Does this early return need to gracefully handle indicator deregistration
when allocation fails?

[ ... ]

> static void virtio_ccw_del_vqs(struct virtio_device *vdev)
> {
> 	struct virtqueue *vq, *n;
> 	struct ccw1 *ccw;
> 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> 
> 	ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw), NULL);
> 	if (!ccw)
> 		return;

[Severity: High]
This isn't a bug introduced by this patch, but returning early on allocation
failure skips the teardown loop entirely.

This leaks all virtqueues and their DMA memory. Since the virtqueues are not
deregistered from the host or removed from the global airq_areas routing
array, the virtio core will subsequently free the parent virtio_ccw_device.

A later adapter interrupt could invoke vring_interrupt() on the leaked
virtqueue, holding a stale pointer to the freed device and potentially
causing a use-after-free.

Should this fallback to a safe teardown path if ccw allocation fails?

[ ... ]

> static void virtio_ccw_reset(struct virtio_device *vdev)
> {
> 	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> 	struct ccw1 *ccw;
> 
> 	ccw = ccw_device_dma_zalloc(vcdev->cdev, sizeof(*ccw), NULL);
> 	if (!ccw)
> 		return;

[Severity: Critical]
This is a pre-existing issue, but this silent return bypasses sending the
CCW_CMD_VDEV_RESET command to the host if allocation fails.

The virtio core assumes the device is fully quiesced after virtio_ccw_reset()
returns and proceeds to free virtqueues and data buffers. Since the device on
the host was never actually reset, the hardware state machine remains active
and may continue to execute pending DMA operations into the now-freed memory,
causing critical data corruption.

How should we ensure the device is safely quiesced if ccw allocation fails?

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