Re: [GIT PULL v1 05/23] s390/vfio_ccw: Calculate idal length based on idaw type

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

Pre-existing issues:
- [High] Untranslated guest physical addresses (or garbage data) are passed=
 to the physical host channel subsystem for skipped commands, causing spuri=
ous channel-program checks.
--

--- Patch [5]: [GIT PULL v1 05/23] s390/vfio_ccw: Calculate idal length bas=
ed on idaw type ---
commit e748c56d38e70498395b0ca91606fb9902835452
Author: Eric Farman <[email protected]>

s390/vfio_ccw: Calculate idal length based on idaw type

This commit fixes the guest IDAL length calculation in get_guest_idal() by
ensuring the size is based on the IDAW type (Format-1 vs Format-2) rather
than unconditionally assuming Format-2 (64-bit).

Link: https://lore.kernel.org/r/[email protected]/

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_c=
p.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)
>  }
> =20
>  #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(u3=
2))
> =20
>  /*
>   * Helpers to operate ccwchain.

[ ... ]

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

[Severity: High]
This is a pre-existing issue, but looking at ccwchain_fetch_ccw() in this
same file, there appears to be a problem when evaluating skipped commands:

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

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

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

When pa->pa_nr is 0 for a command that does not transfer data (such as a
read with the SKIP flag set), page_array_idal_create_words() will perform
no operations.

Since ccw->cda is unconditionally set to the idaws array and CCW_FLAG_IDA
is set, does this cause the host channel subsystem to fetch untranslated
guest physical addresses (or garbage data)?

For skipped read operations, the s390 hardware architecture still requires
IDAWs to be fetched and validity checked. Could this result in spurious
channel-program checks being injected into valid guest I/O operations?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803160924.2368=
[email protected]?part=3D5