Re: [PATCH v9 03/10] s390/vfio_ccw: fix out of bounds check on CCW array

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] A preexisting logic flaw allows a guest to cause the host's Channel Subsystem to execute out-of-bounds memory as a Channel Command Word (CCW), potentially leading to arbitrary host memory corruption via DMA.
--

--- Patch [3]: [PATCH v9 03/10] s390/vfio_ccw: fix out of bounds check on CCW array ---
commit 622cd15d4520f97b7f1bb4f1d15867ddea2a23a6
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: fix out of bounds check on CCW array

This commit modifies the loop in ccwchain_calc_length() to fix an
out-of-bounds array access that could examine the 257th CCW before failing.
It changes a do..while loop to a for loop to strictly bound the CCW array
examination.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 1c2890d139c6b..af632f9d54535 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
[ ... ]
> @@ -377,11 +377,9 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)
>  static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  {
>  	struct ccw1 *ccw = cp->guest_cp;
> -	int cnt = 0;
> -
> -	do {
> -		cnt++;
> +	int cnt;
>  
> +	for (cnt = 1; cnt <= CCWCHAIN_LEN_MAX; cnt++, ccw++) {
>  		/*
>  		 * We want to keep counting if the current CCW has the
>  		 * command-chaining flag enabled, or if it is a TIC CCW
> @@ -391,15 +389,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  		 * after the TIC, depending on the results of its operation.
>  		 */
>  		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
> -			break;
> -
> -		ccw++;
> -	} while (cnt < CCWCHAIN_LEN_MAX + 1);
> -
> -	if (cnt == CCWCHAIN_LEN_MAX + 1)
> -		cnt = -EINVAL;
> +			return cnt;

[Severity: Critical]
This is a pre-existing issue, but does this calculation adequately account for
the hardware's behavior of skipping a CCW when a Status Modifier is presented?

If a guest submits a channel program where a CCW that can generate a Status
Modifier (like a Search CCW) has the command-chaining (CC) flag enabled and is
followed by a TIC CCW that lacks the CC flag and jumps to a different chain,
the loop will stop counting at the TIC because both !ccw_is_chain(ccw) and
!is_tic_within_range(...) are true.

In this scenario, ccwchain_calc_length() returns a length of exactly 2. The
host would then allocate 16 bytes for chain->ch_ccw from the DMA heap. When the
hardware executes the first CCW and encounters a Status Modifier, it skips the
TIC and fetches the next CCW from 16 bytes into the allocation, which is
out-of-bounds of the host's DMA heap allocation.

By grooming the host heap, could a guest place controlled data adjacent to this
allocation, which the hardware would then execute as an untranslated CCW,
potentially leading to arbitrary host memory corruption via DMA?

> +	}
>  
> -	return cnt;
> +	return -EINVAL;
>  }

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