Re: [PATCH v8 01/10] 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 recursion in CCW chain processing allows a malicious guest to overflow the host kernel stack.
- [High] Off-by-one out-of-bounds read in `ccwchain_calc_length()`.
--

--- Patch [1]: [PATCH v8 01/10] s390/vfio_ccw: free all memory if cp_init() fails ---
commit 7b177e824046c97e78d70a5cb8daa7a37e59333e
Author: Eric Farman <farman@linux.ibm.com>

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

This commit addresses a memory leak in the channel program cleanup logic.
By moving the cleanup outside of the recursive handling functions, it
ensures all segments created by Transfer in Channel operations are
properly freed when initialization fails.

> 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 recursive call chain allow a
guest to trigger unbounded recursion and exhaust the host kernel stack?

Looking at the code execution path:

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

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_loop_tic()
   ret = ccwchain_handle_ccw(tic->cda, cp);

If a guest constructs a channel program with a long sequence of Transfer
In Channel (TIC) instructions, each pointing to a distinct CCW chain
memory address, tic_target_chain_exists() will return false.

Could this cause ccwchain_loop_tic() to recursively call
ccwchain_handle_ccw() for each segment without any depth limits, leading
to a guest-to-host denial of service?


[Severity: High]
This isn't a bug introduced by this patch, but is there an off-by-one
out-of-bounds read in ccwchain_calc_length() which is called earlier in
this function?

ccwchain_handle_ccw() calls ccwchain_calc_length(), which has the following
loop condition:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_calc_length() {
    ...
    } while (cnt < CCWCHAIN_LEN_MAX + 1);
    ...
}

Since guest_cp is allocated with exactly CCWCHAIN_LEN_MAX elements (256),
when cnt becomes 257 during the iteration, evaluating the ccw pointer
accesses guest_cp[256].

If a guest submits a chain of exactly CCWCHAIN_LEN_MAX length with the
command-chaining flag enabled on the last element, could this read past the
end of the allocation and potentially oops if it hits an unmapped page?

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

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