Re: [PATCH v9 10/10] s390/vfio_ccw: implement a crw lock

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Concurrent lockless modification of eventfd triggers in `vfio_ccw_mdev_set_irqs` causes a Use-After-Free of the `eventfd_ctx`.
- [High] Concurrent execution of asynchronous CSS events and mdev removal causes a Use-After-Free of the `private` structure.
--

--- Patch [10]: [PATCH v9 10/10] s390/vfio_ccw: implement a crw lock ---
commit 1d80144375864b24376def1fe3aa7c470ed46eba
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a crw lock

This commit introduces a spinlock to serialize access to the CRW region,
which relies exclusively on asynchronous events from hardware. It ensures
cohesive reading of those payloads and manages the list of those payloads.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 757ff5b2556e3..ab6b518cc353f 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -118,11 +118,14 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  void vfio_ccw_crw_todo(struct work_struct *work)
>  {
>  	struct vfio_ccw_private *private;
> +	unsigned long flags;
>  
>  	private = container_of(work, struct vfio_ccw_private, crw_work);
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	if (!list_empty(&private->crw) && private->crw_trigger)
>  		eventfd_signal(private->crw_trigger);

[Severity: Critical]
This is a pre-existing issue, but can this concurrent lockless access to
private->crw_trigger lead to a use-after-free of the eventfd_ctx?

The ioctl writer in vfio_ccw_mdev_set_irqs() modifies and releases the eventfd
pointers locklessly:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_set_irqs() {
    ...
    if (*ctx)
        eventfd_ctx_put(*ctx);
    *ctx = NULL;
    ...
}

Concurrently, this worker thread reads the trigger pointer here without any
synchronization against the ioctl writer (no RCU, no matching locks).

Could the ioctl free the context immediately after the worker validates the
pointer but before it signals it? (This same pattern also appears in
vfio_ccw_sch_io_todo() with private->io_trigger).

> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  }

[ ... ]

> @@ -286,6 +289,7 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
>  			       unsigned int rsid)
>  {
>  	struct vfio_ccw_crw *crw;
> +	unsigned long flags;
>  
>  	/*
>  	 * If unable to allocate a CRW, just drop the event and
> @@ -303,7 +307,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
>  	crw->crw.erc = erc;
>  	crw->crw.rsid = rsid;
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	list_add_tail(&crw->next, &private->crw);
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  	queue_work(vfio_ccw_work_q, &private->crw_work);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can the private pointer be freed
before this lock is acquired, leading to a use-after-free?

Concurrent mdev removal clears the driver data and drops the device reference
without synchronizing against the CSS subsystem:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_remove() {
    ...
    dev_set_drvdata(&parent->dev, NULL);
    vfio_put_device(&private->vdev);
    ...
}

Concurrently, an asynchronous CSS event triggers vfio_ccw_chp_event(), which
fetches the private pointer locklessly just before it is cleared:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_chp_event() {
    ...
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
    ...
}

This path uses the potentially freed pointer to lock memory here and push a
freed work_struct onto the kernel workqueue.

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