Re: [GIT PULL v1 09/23] s390/vfio_ccw: Selectively expand io_mutex

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

Pre-existing issues:
- [High] Concurrent modification of the `private->crw` linked list without =
proper synchronization, risking list corruption.
- [High] Missing synchronization between CSS callbacks and `vfio_ccw_mdev_r=
emove()` causes use-after-free of `private`.
- [Medium] Valid CRW events are permanently dropped and lost if userspace p=
rovides an invalid read offset.
--

--- Patch [9]: [GIT PULL v1 09/23] s390/vfio_ccw: Selectively expand io_mut=
ex ---
commit 0dffa46dd03162ef88bfa6bbfa42490f8f336394
Author: Eric Farman <[email protected]>

s390/vfio_ccw: Selectively expand io_mutex

The io_mutex was defined to serialize the io_regions, but then has
also sort of been associated with the I/O themselves because of
the close relationship they share. This patch expands the scope of
io_mutex to close these remaining windows.

Cc: [email protected]

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_=
chp.c
> index f3015132d4b5f..9269b54f5cfd4 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -98,13 +98,13 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_c=
cw_private *private,
>  	if (pos + count > sizeof(*region))
>  		return -EINVAL;
> =20
> +	mutex_lock(&private->io_mutex);
>  	crw =3D list_first_entry_or_null(&private->crw,
>  				       struct vfio_ccw_crw, next);
> =20
>  	if (crw)
>  		list_del(&crw->next);

[Severity: High]
This isn't a bug introduced by this patch, but does this list_del() lack
proper synchronization?

It appears vfio_ccw_crw_region_read() dequeues from private->crw while
only holding the sleepable io_mutex. If an atomic channel path event
triggers vfio_ccw_chp_event() concurrently, it will call
vfio_ccw_queue_crw(), which locklessly adds to private->crw using
list_add_tail().

Since io_mutex is ignored by the atomic queue path, can this concurrent
access corrupt the linked list if userspace aggressively reads the CRW
region while hardware events are generated?

> -	mutex_lock(&private->io_mutex);
>  	if (i >=3D private->num_regions) {
>  		ret =3D -EINVAL;
>  		goto out;
>  	}

[Severity: Medium]
This is a pre-existing issue, but are valid CRW events permanently dropped
if userspace provides an invalid read offset?

The bounds check for the region index occurs after the event has already
been dequeued from private->crw. If the offset is invalid, the event seems
to be discarded and later freed on failure without delivery.

Should state mutations happen after all arguments are validated to prevent
this resource loss?

[ ... ]
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_=
drv.c
> index c197ad5ab5805..757ff5b2556e3 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a missing synchronization step
between CSS callbacks and vfio_ccw_mdev_remove() that causes a
use-after-free?

In drivers/s390/cio/vfio_ccw_ops.c, vfio_ccw_mdev_remove() clears drvdata
and drops the final reference to the device:

vfio_ccw_mdev_remove() {
    ...
    dev_set_drvdata(&parent->dev, NULL);
    vfio_put_device(&private->vdev);
}

If an administrator initiates an mdev removal concurrently with a hardware
event, vfio_ccw_sch_event() in drivers/s390/cio/vfio_ccw_drv.c reads the
private pointer locklessly via dev_get_drvdata():

vfio_ccw_sch_event() {
    ...
    struct vfio_ccw_private *private =3D dev_get_drvdata(&parent->dev);
}

If the CSS callback is delayed after reading the pointer, could it execute
vfio_ccw_fsm_event() on the freed private structure?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803160924.2368=
[email protected]?part=3D9