Re: [PATCH v1 4/6] s390/vfio_ccw: copy maximum possible IDAL from guest

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] calc_max_idal_len() returns a word count instead of a byte length, causing truncated guest memory reads and silent guest memory corruption.
--

--- Patch [4]: [PATCH v1 4/6] s390/vfio_ccw: copy maximum possible IDAL from guest ---
commit 3ab642e4c4a6646b5b0bec62ab380c4cc0ea0ac2
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: copy maximum possible IDAL from guest

An Indirect Data Address word is always 2K/4K aligned (depending on
format/type), except for the first word in a list. This unaligned
word makes calculating the number of addresses in a list challenging.
The current code attempts to be efficient about this by reading the
first word before making its calculations, but it introduces
inefficiencies trying to do the math on supposedly equal values.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 74b1f25e01e72..dac53e26509e5 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -523,16 +523,24 @@ static int ccwchain_fetch_tic(struct ccw1 *ccw,
>  	return -EFAULT;
>  }
>  
> +static int calc_max_idal_len(struct ccw1 *ccw, struct channel_program *cp)
> +{
> +	int idal_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
> +	int idal_mask = ~(idal_size - 1);
> +	int idal_len = idal_size - (ccw->cda & ~idal_mask);
> +
> +	/* This overestimates for Format-1 or 2K-Format-2 IDAWs */
> +	return idal_len / 8;
> +}
> +
>  static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int idaw_nr)
>  {
> -	struct vfio_device *vdev =
> -		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
>  	dma64_t *idaws;
>  	dma32_t *idaws_f1;
>  	int idal_len = idaw_nr * sizeof(*idaws);
>  	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
>  	int idaw_mask = ~(idaw_size - 1);
> -	int i, ret;
> +	int i;
>  
>  	idaws = kzalloc_objs(*idaws, idaw_nr, GFP_DMA | GFP_KERNEL);
>  	if (!idaws)
> @@ -540,11 +548,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
>  
>  	if (ccw_is_idal(ccw)) {
>  		/* Copy IDAL from guest */
> -		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), idaws, idal_len, false);
> -		if (ret) {
> -			kfree(idaws);
> -			return ERR_PTR(ret);
> -		}
> +		memcpy(idaws, cp->guest_idal, idal_len);
>  	} else {
>  		/* Fabricate an IDAL based off CCW data address */
>  		if (cp->orb.cmd.c64) {

[ ... ]

> @@ -586,7 +590,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 = calc_max_idal_len(ccw, cp);
>  	int ret;
>  	int bytes = 1;
>  
> @@ -596,10 +600,13 @@ static int ccw_count_idaws(struct ccw1 *ccw,
>  	if (ccw_is_idal(ccw)) {
>  		/* Read first IDAW to check its starting address. */
>  		/* All subsequent IDAWs will be 2K- or 4K-aligned. */
> -		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), &iova, size, false);
> +		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda),
> +				  cp->guest_idal, size, false);

[Severity: High]
Does this pass a word count instead of a byte count to vfio_dma_rw()?

calc_max_idal_len() returns a word count because it divides the remaining bytes
by 8. When this value is stored in size and passed to vfio_dma_rw(), it seems
it will read at most 1/8th of the required IDAL data.

Later in get_guest_idal(), the code copies the full required byte length from
the partially-filled cp->guest_idal buffer:

	memcpy(idaws, cp->guest_idal, idal_len);

Could this result in copying stale or uninitialized data to the host IDAL,
causing the hardware to perform DMA to incorrect guest physical addresses?

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