Re: [PATCH v8 09/10] s390/vfio_ccw: selectively expand io_mutex

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `notoper_work` work item is not canceled when the mediated device is released, leading to a Use-After-Free of the `vfio_ccw_private` structure and `io_mutex` corruption.

Pre-existing issues:
- [High] Incomplete synchronization of the `private->crw` list leads to a data race and potential list corruption between atomic writers and mutex-holding readers.
- [High] The device state (`private->state`) is vulnerable to corruption because the newly protected `io_mutex` critical sections race with lockless atomic context writers.
--

--- Patch [9]: [PATCH v8 09/10] s390/vfio_ccw: selectively expand io_mutex ---
commit 033ce2042d965485876fe10c634ebbb9d1df4992
Author: Eric Farman <farman@linux.ibm.com>
s390/vfio_ccw: selectively expand io_mutex

The io_mutex was defined to serialize the io_regions, but has also been
associated with the I/O themselves. This patch expands the scope of io_mutex
to close remaining race windows, and introduces a workqueue element for
cp_free() to avoid acquiring mutexes in interrupt context.

> 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_ccw_private *private,
>  	if (pos + count > sizeof(*region))
>  		return -EINVAL;
>  
> +	mutex_lock(&private->io_mutex);
>  	crw = list_first_entry_or_null(&private->crw,
>  				       struct vfio_ccw_crw, next);
>  
>  	if (crw)
>  		list_del(&crw->next);
>  
> -	mutex_lock(&private->io_mutex);
>  	if (i >= private->num_regions) {
>  		ret = -EINVAL;
>  		goto out;

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but does
this lock movement fully resolve the race on the private->crw list?

While this patch protects list deletion in vfio_ccw_crw_region_read() with
io_mutex, it appears the list can still be modified concurrently without
locks. For example, vfio_ccw_queue_crw() adds items to this list locklessly:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_queue_crw() {
	...
	crw->crw.rsid = rsid;

	list_add_tail(&crw->next, &private->crw);
	queue_work(vfio_ccw_work_q, &private->crw_work);
	...
}

Because vfio_ccw_queue_crw() can be called from atomic contexts (like
vfio_ccw_chp_event()), it cannot acquire the sleepable io_mutex. Can this
result in list corruption between the lockless atomic producer and the
mutex-holding reader?

[ ... ]

> 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
> @@ -91,6 +91,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  
>  	is_final = !(scsw_actl(&irb->scsw) &
>  		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
> +	mutex_lock(&private->io_mutex);
>  	if (scsw_is_solicited(&irb->scsw)) {
>  		cp_update_scsw(&private->cp, &irb->scsw);
>  		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
> @@ -98,9 +99,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  			cp_is_finished = true;
>  		}
>  	}
> -	mutex_lock(&private->io_mutex);
>  	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
> -	mutex_unlock(&private->io_mutex);
>  
>  	/*
>  	 * Reset to IDLE only if processing of a channel program
> @@ -110,6 +109,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  	 */
>  	if (cp_is_finished)
>  		private->state = VFIO_CCW_STATE_IDLE;
> +	mutex_unlock(&private->io_mutex);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but could
this state update be overwritten by concurrent lockless modifications?

With the newly expanded io_mutex, vfio_ccw_sch_io_todo() reads and modifies
private->state under the lock. However, fsm_notoper() updates the state
without acquiring io_mutex:

drivers/s390/cio/vfio_ccw_fsm.c:fsm_notoper() {
	...
	css_sched_sch_todo(sch, SCH_TODO_UNREG);
	private->state = VFIO_CCW_STATE_NOT_OPER;
	...
}

If a device goes non-operational concurrently with the completion of a
channel program, could vfio_ccw_sch_io_todo() overwrite the NOT_OPER state
with IDLE, discarding the transition?

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d47a3c7b9a06..5fd94e9d5c618 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -170,6 +170,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
>  	css_sched_sch_todo(sch, SCH_TODO_UNREG);
>  	private->state = VFIO_CCW_STATE_NOT_OPER;
>  
> +	/* This routine could be called from IRQ context, so defer */
>  	queue_work(vfio_ccw_work_q, &private->notoper_work);
>  }

[Severity: High]
Is it possible for private->notoper_work to outlive the private structure
and cause a use-after-free?

If a VFIO_CCW_EVENT_NOT_OPER event occurs while the device is in the STANDBY
state (for instance, from an offline channel path before the device is
opened), fsm_notoper() unconditionally queues notoper_work.

However, if the mediated device is then removed without ever being opened,
vfio_ccw_mdev_release_dev() appears to skip canceling this work:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_release_dev() {
	...
	/*
	 * Ensure these work items are fully drained, so none can
	 * fire after being released. The notoper_work struct is
	 * only meaningful if the device had been opened, which
	 * means it would have been cleaned in an earlier close.
	 */
	cancel_work_sync(&private->io_work);
	cancel_work_sync(&private->crw_work);
	...
}

Because the notoper_work is never canceled here, could it execute after the
private structure is freed, leading to memory corruption when
vfio_ccw_notoper_todo() attempts to acquire io_mutex?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728013509.1551753-1-farman@linux.ibm.com?part=9