Re: [PATCH v1 1/6] s390/vfio_ccw: fix out of bounds check on CCW array

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces an off-by-one regression that incorrectly rejects valid channel programs consisting of exactly CCWCHAIN_LEN_MAX (256) CCWs.

Pre-existing issues:
- [High] Leaked ccwchain structures when a subsequent TIC processing fails.
--

--- Patch [1]: [PATCH v1 1/6] s390/vfio_ccw: fix out of bounds check on CCW array ---
commit ccfdffefc50adaae37db8e069d19bc3ab3f7721c
Author: Eric Farman <farman@linux.ibm.com>
s390/vfio_ccw: fix out of bounds check on CCW array

This commit limits the channel command word counting loop to 256 iterations
to prevent out-of-bounds array access. It changes the loop condition and
the subsequent error check to reject chains larger than 256 words.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 7561aa7d3e017..80c3d87f54822 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -393,9 +393,9 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  			break;
>  
>  		ccw++;
> -	} while (cnt < CCWCHAIN_LEN_MAX + 1);
> +	} while (cnt < CCWCHAIN_LEN_MAX);
>  
> -	if (cnt == CCWCHAIN_LEN_MAX + 1)
> +	if (cnt >= CCWCHAIN_LEN_MAX)

[Severity: High]
Does this code incorrectly reject valid channel programs consisting of
exactly 256 command words?

If a guest issues a valid request with exactly 256 command words in a chain,
the loop will correctly process all of them and cnt will reach 256. The
new check evaluates to true since 256 >= 256, incorrectly conflating
this valid termination with an over-length chain and unconditionally
returning -EINVAL.

>  		cnt = -EINVAL;
>  
>  	return cnt;

[Severity: High]
This is a pre-existing issue, but does this code leak ccwchain structures
if a subsequent transfer in channel fails?

Looking at ccwchain_handle_ccw(), if a guest issues a request with
multiple transfer in channel commands, and an earlier command is valid but
a later one fails, the valid chain allocated during ccwchain_loop_tic()
is added to cp->ccwchain_list.

When the invalid command fails, ccwchain_handle_ccw() catches the error
and only frees its own top-level chain:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_handle_ccw() {
...
	ret = ccwchain_loop_tic(chain, cp);

	if (ret)
		ccwchain_free(chain);
...
}

The error is returned to cp_init(), which leaves cp->initialized as false:

drivers/s390/cio/vfio_ccw_cp.c:cp_init() {
...
	INIT_LIST_HEAD(&cp->ccwchain_list);
...
	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);

	if (!ret)
		cp->initialized = true;
...
}

Because cp->initialized is false, cp_free() ignores the list during
device teardown:

drivers/s390/cio/vfio_ccw_cp.c:cp_free() {
...
	if (!cp->initialized)
		return;
...
}

When the next request arrives, cp_init() calls INIT_LIST_HEAD again,
which permanently orphans the leaked chains on the list.

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