Re: [PATCH v4 4/9] s390/vfio_ccw: ensure first IDAW remains constant

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `get_guest_idal` reads past the end of the guest's IDAL buffer for format-1 IDALs, potentially causing spurious DMA faults.
- [High] Untranslated guest IDALs are passed to the host channel subsystem when data transfer is skipped, causing hardware Program Checks.
--

--- Patch [4]: [PATCH v4 4/9] s390/vfio_ccw: ensure first IDAW remains constant ---
commit ff6c62bce82b594d19f9d6e95c9229ea752d6614
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: ensure first IDAW remains constant

The first IDAW in a list does not need to be on a 2K/4K boundary like all
others, and so is read separately to accurately calculate the size of the
buffer needed to read the full IDAL. This commit validates that the address
found in the first IDAW is unchanged between reads.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index af632f9d54535..6275794751cb4 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -523,6 +523,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
>  		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
>  	dma64_t *idaws;
>  	dma32_t *idaws_f1;
> +	u64 first_idaw;
>  	int idal_len = idaw_nr * sizeof(*idaws);
        ^^^^

[Severity: High]
This is a pre-existing issue, but does get_guest_idal() read past the end of
the guest's IDAL buffer for format-1 IDALs?

It appears that idal_len is calculated using sizeof(*idaws), which is always 8
bytes (sizeof(dma64_t)), even when the guest uses 32-bit (format-1) IDAWs.

Later in get_guest_idal(), vfio_dma_rw() is called with this idal_len:

	if (ccw_is_idal(ccw)) {
		/* Copy IDAL from guest */
		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), idaws, idal_len, false);

If a guest channel program uses Format-1 (32-bit) IDAWs, they are only 4 bytes
each. Calling vfio_dma_rw() with idaw_nr * 8 reads 100% past the end of the
guest's IDAL buffer.

If the guest's 32-bit IDAL ends near a page boundary and the adjacent page is
unmapped or restricted, this over-read could cross into unmapped guest memory
and fail, potentially spuriously rejecting a valid guest I/O request with a
DMA translation error.

>  	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
>  	int idaw_mask = ~(idaw_size - 1);

[ ... ]

[Severity: High]
This is also a pre-existing issue, but does ccwchain_fetch_ccw() pass
untranslated guest IDALs to the host channel subsystem when data transfer is
skipped?

Looking at ccwchain_fetch_ccw(), if ccw_does_data_transfer(ccw) is false
(e.g., a READ CCW with the SKIP flag), pa->pa_nr is set to 0:

	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);

Consequently, page_array_idal_create_words() loops 0 times, leaving the idaws
buffer populated with raw guest IOVAs instead of valid host absolute addresses.

Since vfio-ccw forces orb.cmd.c64 = 1 (Format-2, 64-bit IDAWs) when issuing the
SSCH to the host channel subsystem, the hardware will fetch these untranslated
guest IOVAs. If the guest used 32-bit IDAWs, they are read as garbage 64-bit
pointers.

Will the hardware validate these invalid addresses during execution and trigger
a Channel Program Check, spuriously failing the guest's SKIP I/O request?

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