Re: [PATCH v1 3/6] s390/vfio_ccw: free all memory if cp_init() fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Concurrent execution of `cp_free()` can lead to a double-free of ccwchain structures and use-after-free of unpinned pages.
--

--- Patch [3]: [PATCH v1 3/6] s390/vfio_ccw: free all memory if cp_init() fails ---
commit f65cc16ffb4e4ab4d98caf412ccd788513657f79
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: free all memory if cp_init() fails

This commit refactors the cleanup of ccwchain segments during
initialization. It moves the memory freeing outside of the recursive
ccwchain handling logic to prevent memory leaks when cp_init() fails.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 76632b18fc37c..74b1f25e01e72 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c

[ ... ]

> @@ -740,7 +754,7 @@ int cp_init(struct channel_program *cp, union orb *orb)
>  	memcpy(&cp->orb, orb, sizeof(*orb));
>  
>  	/* Build a ccwchain for the first CCW segment */
> -	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
> +	ret = ccwchain_build_ccws(orb->cmd.cpa, cp);
>  
>  	if (!ret)
>  		cp->initialized = true;

[Severity: Critical]
This is a pre-existing issue, but could the concurrent execution of
cp_free() on this initialized channel program lead to a double-free of
ccwchain structures and a use-after-free of unpinned pages?

If a guest VM issues a channel program to an mdev to cause a subchannel
interrupt and then immediately triggers a vfio device reset ioctl, could
vfio_ccw_sch_io_todo() and fsm_close() race?

In drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo(), the workqueue
path reads private->state and calls cp_free() without acquiring io_mutex
or sch->lock:

        if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
                cp_free(&private->cp);
                cp_is_finished = true;
        }

Concurrently, fsm_close() can acquire sch->lock, successfully disable the
subchannel, set private->state = VFIO_CCW_STATE_STANDBY, drop the lock,
and call cp_free().

If vfio_ccw_sch_io_todo() reads the state as VFIO_CCW_STATE_CP_PENDING
before fsm_close() updates it, both threads might execute cp_free()
simultaneously.

In drivers/s390/cio/vfio_ccw_cp.c:cp_free(), there is a lockless check:

        if (!cp->initialized)
                return;

        cp->initialized = false;

Does this lockless time-of-check to time-of-use allow both threads to 
proceed and call kfree() on the same ccwchain structures, potentially
leading to a host kernel panic?

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