Re: [PATCH v8 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:
- [High] The driver unconditionally attempts to read a full 2048-byte chunk (256 CCWs) from guest memory, which will fail if a valid, shorter channel program is located near the end of mapped guest memory.
--

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

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

The routine ccwchain_calc_length() counts the number of channel
command words (CCWs) that are chained together in a single channel
program, and rejects anything larger than CCWCHAIN_LEN_MAX (256) CCWs.
The loop is restructured to avoid checking memory outside the valid range.

Cc: stable@vger.kernel.org

> 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++) {

[Severity: High]
This is a pre-existing issue, but since we are modifying the CCW bounds
checking here, could the unconditional 2K DMA read in ccwchain_handle_ccw()
inadvertently reject valid channel programs?

The driver unconditionally attempts to read a full 2048-byte chunk (256 CCWs)
from guest memory before calculating the actual length:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_handle_ccw() {
    ...
    /* Copy 2K (the most we support today) of possible CCWs */
    ret = vfio_dma_rw(vdev, gcda, cp->guest_cp,
                      CCWCHAIN_LEN_MAX * sizeof(struct ccw1), false);
    if (ret)
        return ret;
    ...
    /* Count the CCWs in the current chain */
    len = ccwchain_calc_length(gcda, cp);
    ...
}

If a guest uses a strict vIOMMU mapping and places a valid, shorter channel
program near the end of a mapped IOVA region, won't this read cross into an
unmapped page and fail with -EFAULT? This appears to render the device
partially or fully unusable for guests configuring their mappings this way.

>  		/*
>  		 * We want to keep counting if the current CCW has the
>  		 * command-chaining flag enabled, or if it is a TIC CCW

[ ... ]

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