Re: [PATCH v3 1/8] s390/vfio_ccw: free all memory if cp_init() fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Unbounded mutual recursion in channel program parsing leads to host kernel stack overflow.
- [High] Out-of-bounds read in `ccwchain_calc_length` due to an off-by-one error.
--

--- Patch [1]: [PATCH v3 1/8] s390/vfio_ccw: free all memory if cp_init() fails ---
commit e7fe5fe9c1f027187ba27b2948379cc2bd88397e
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: free all memory if cp_init() fails

The routine cp_free() is called to unpin/free any memory once an I/O
is completed successfully, or if cp_prefetch() fails. But if cp_init()
fails, and cp->initialized is not enabled, the same routine cannot be
used to free all the memory. This patch performs the cleanup outside of
the recursive ccwchain_handle_ccw()/ccwchain_loop_tic() logic.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 7561aa7d3e017..086d1b54bdb05 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -455,9 +455,6 @@ static int ccwchain_handle_ccw(dma32_t cda, struct channel_program *cp)
>  	/* Loop for tics on this new chain. */
>  	ret = ccwchain_loop_tic(chain, cp);

[Severity: Critical]
This is a pre-existing issue, but does this mutual recursion between
ccwchain_handle_ccw() and ccwchain_loop_tic() have a depth limit?

If a guest VM constructs a channel program with a long sequence of
transfer in channel commands that each jump to a distinct guest physical
address, tic_target_chain_exists() will return false.

This causes ccwchain_loop_tic() to recursively call ccwchain_handle_ccw():

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_loop_tic() {
        ...
        if (tic_target_chain_exists(tic, cp))
                continue;

        ret = ccwchain_handle_ccw(tic->cda, cp);
        ...
}

Since there doesn't appear to be a limit on the recursion depth here,
could this exhaust the host kernel stack?

> -	if (ret)
> -		ccwchain_free(chain);
> -
>  	return ret;
>  }

[ ... ]

[Severity: High]
This is also a pre-existing issue, but is there a potential out-of-bounds
read in ccwchain_calc_length() due to an off-by-one error?

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_calc_length() {
        ...
        do {
                cnt++;
                if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
                        break;
                ccw++;
        } while (cnt < CCWCHAIN_LEN_MAX + 1);
        ...
}

Since cp->guest_cp is allocated with exactly CCWCHAIN_LEN_MAX elements,
if a guest provides a channel program where all 256 items have the
command-chaining flag enabled, the loop executes 256 times.

On the 257th iteration, cnt is 257 and ccw points to cp->guest_cp[256],
which is past the end of the allocation. The loop condition evaluates
to true (256 < 257), allowing ccw_is_chain(ccw) to dereference the
out-of-bounds pointer.

Could this read hit an unmapped guard page and crash the host?

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