Re: [GIT PULL v1 08/23] s390/vfio_ccw: Move cp cleanup out of not operational

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent execution of `cp_free()` can lead to a double-free of channel program resources due to missing locking.
- [High] A TOCTOU race between subchannel events and mdev removal allows an asynchronous event to queue work on a freed vfio_ccw_private structure.
--

--- Patch [8]: [GIT PULL v1 08/23] s390/vfio_ccw: Move cp cleanup out of not operational ---
commit aa01cda35a6813e56642eb80e4a1779189d3de13
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: Move cp cleanup out of not operational

This commit moves the cleanup of channel program resources out of the
not-operational state machine event and defers it to a workqueue. This avoids
potential locking issues with the cio subchannel lock when the device is lost.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1a095085bc728..c197ad5ab5805 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c

[ ... ]

> @@ -125,6 +125,15 @@ void vfio_ccw_crw_todo(struct work_struct *work)
>  		eventfd_signal(private->crw_trigger);
>  }
>  
> +void vfio_ccw_notoper_todo(struct work_struct *work)
> +{
> +	struct vfio_ccw_private *private;
> +
> +	private = container_of(work, struct vfio_ccw_private, notoper_work);
> +
> +	cp_free(&private->cp);

[Severity: High]
This is a pre-existing issue, but making the cleanup asynchronous
significantly widens the vulnerability window for a double-free.

Since vfio_ccw_notoper_todo() does not acquire io_mutex before calling
cp_free(), what happens if userspace concurrently closes the VFIO device file
descriptor?

If a device becomes not-operational, triggering vfio_ccw_sch_event() which
queues vfio_ccw_notoper_todo(), and concurrently vfio_ccw_mdev_close_device()
calls fsm_close() -> cp_free(), both threads could enter cp_free()
simultaneously.

Because the !cp->initialized check in drivers/s390/cio/vfio_ccw_cp.c:cp_free()
is non-atomic, both threads could bypass it and proceed to double-free the
channel program resources.

Could we acquire private->io_mutex before calling cp_free() here?

> +}

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d7988ea47ef0..4d47a3c7b9a06 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -170,8 +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 is usually handled during CLOSE event */
> -	cp_free(&private->cp);
> +	queue_work(vfio_ccw_work_q, &private->notoper_work);

[Severity: High]
This isn't a bug introduced by this patch, but transforming this cleanup into
a queue_work() call exposes a specific use-after-free vector via a TOCTOU
race.

Looking at vfio_ccw_sch_event() in drivers/s390/cio/vfio_ccw_drv.c, the
private pointer is read locklessly via dev_get_drvdata(). If the thread is
preempted after obtaining this pointer, could a concurrent sysfs mdev_remove
call execute vfio_ccw_mdev_remove()?

If vfio_ccw_mdev_remove() clears the drvdata, flushes workqueues, and drops
the final reference to private, the memory is freed.

When vfio_ccw_sch_event() resumes, it would dereference the freed private
pointer to call fsm_notoper(), which then executes queue_work() on the freed
memory, potentially corrupting the global system workqueue.

Is there a way to ensure the lifetime of the private structure during this
event processing?

>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803160924.236807-1-imbrenda@linux.ibm.com?part=8