Re: [PATCH v9 05/10] s390/vfio_ccw: calculate idal length based on idaw type

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Unprotected state modification of `private->state` in the workqueue handler causing a data race.
- [Medium] Untranslated guest IOVAs are passed to the hardware for skipped data transfers.
--

--- Patch [5]: [PATCH v9 05/10] s390/vfio_ccw: calculate idal length based on idaw type ---
commit 02ef53680a2ab9a5c4dcb63aa0840ed5676edddc
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: calculate idal length based on idaw type

Sashiko pointed out that get_guest_idal() unconditionally calculates
the length of the IDAL presuming everything is a Format-2 IDAW.
The output of vfio-ccw is always Format-2, but the input can be either
Format-1 (31-bit addresses) or Format-2 (64-bit addresses). As a result,
the size of the guest IDAL may be incorrect and should be trimmed down.

Link: https://lore.kernel.org/r/20260720203400.7328E1F000E9@smtp.kernel.org/

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 6275794751cb4..5ef082b8289af 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -233,6 +233,7 @@ static void convert_ccw0_to_ccw1(struct ccw1 *source, unsigned long len)
>  }
>  
>  #define idal_is_2k(_cp) (!(_cp)->orb.cmd.c64 || (_cp)->orb.cmd.i2k)
> +#define get_idaw_size(_cp) ((_cp)->orb.cmd.c64 ? sizeof(u64) : sizeof(u32))
>  
>  /*
>   * Helpers to operate ccwchain.

[ ... ]

> @@ -593,7 +594,7 @@ static int ccw_count_idaws(struct ccw1 *ccw,
>  	struct vfio_device *vdev =
>  		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
>  	u64 iova;
> -	int size = cp->orb.cmd.c64 ? sizeof(u64) : sizeof(u32);
> +	int size = get_idaw_size(cp);
>  	int ret;
>  	int bytes = 1;
>  

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential address translation
issue later in this file. In ccwchain_fetch_ccw(), when a guest submits a
channel program with a CCW that skips data transfer (e.g., READ with
CCW_FLAG_SKIP):

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_fetch_ccw() {
	...
	if (ccw_does_data_transfer(ccw)) {
		ret = page_array_pin(pa, vdev, idal_is_2k(cp));
		if (ret < 0)
			goto out_unpin;
	} else {
		pa->pa_nr = 0;
	}

	ccw->cda = virt_to_dma32(idaws);
	ccw->flags |= CCW_FLAG_IDA;

	/* Populate the IDAL with pinned/translated addresses from page */
	page_array_idal_create_words(pa, idaws);
	...
}

Does this bypass translation and pass untranslated guest IOVAs to the
hardware? If ccw_does_data_transfer() returns 0, the idaws buffer
might be filled with raw guest physical addresses, which the hardware will
interpret as host physical addresses and throw an addressing exception.


[Severity: Medium]
This is also a pre-existing issue, but is it safe to modify private->state
without holding the io_mutex or sch->lock?

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo() {
	...
	if (cp_is_finished)
		private->state = VFIO_CCW_STATE_IDLE;
	...
}

If vfio_ccw_sch_io_todo() executes concurrently with an asynchronous event
like vfio_ccw_sch_event(), could this locklessly overwrite the state to
VFIO_CCW_STATE_IDLE after the event handler has already set it to
VFIO_CCW_STATE_NOT_OPER under the sch->lock?

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