Re: [GIT PULL v1 02/23] s390/vfio_ccw: Limit the number of channel program segments

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Sleeping in atomic context when processing NOT_OPER FSM event
- [High] Race condition leading to Use-After-Free / list corruption on channel program
- [High] Out-of-bounds read in ccwchain_calc_length
--

--- Patch [2]: [GIT PULL v1 02/23] s390/vfio_ccw: Limit the number of channel program segments ---
commit f04946000e5f4fc0876ae6db794b58a1b8744e08
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: Limit the number of channel program segments

The processing of channel programs, and the CCWs within them, is done
recursively. As such, there is an arbitrary (but not architectural)
limit to the number of CCWs that can exist in a single channel program.

The vfio-ccw logic breaks these channel programs into segments whenever
it encounters a Transfer-In-Channel (TIC) CCW, and the combined number
of segments count towards the global limit. Impose an equivalent limit
to the number of segments until such logic can be made non-recursive.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 086d1b54bdb05..1c2890d139c6b 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
[ ... ]
> @@ -441,6 +442,10 @@ static int ccwchain_handle_ccw(dma32_t cda, struct channel_program *cp)
>  	if (len < 0)
>  		return len;

[Severity: High]
This is a pre-existing issue, but can the call to ccwchain_calc_length() just
before this block result in an out-of-bounds read?

If a guest provides a channel program with 256 command-chained CCWs, the loop
in ccwchain_calc_length() reaches cnt == 256 and increments the ccw pointer
past the end of the array.

The loop condition while (cnt < CCWCHAIN_LEN_MAX + 1) then allows a 257th
iteration, which dereferences the out-of-bounds ccw pointer in the
ccw_is_chain(ccw) check.

> +	/* Limit number of chains in a single channel program */
> +	if (cp->ccwchain_count >= CCWCHAIN_COUNT_MAX)
> +		return -EINVAL;
> +
>  	/* Need alloc a new chain for this one. */
>  	chain = ccwchain_alloc(cp, len);
>  	if (!chain)
[ ... ]
> @@ -745,6 +750,7 @@ int cp_init(struct channel_program *cp, union orb *orb)
>  			vdev->dev,
>  			"Prefetching channel program even though prefetch not specified in ORB");
>  
> +	cp->ccwchain_count = 0;
>  	INIT_LIST_HEAD(&cp->ccwchain_list);
>  	memcpy(&cp->orb, orb, sizeof(*orb));

[Severity: High]
This is a pre-existing issue, but could this channel program initialization
race with a subchannel event, leading to a use-after-free or list corruption?

The I/O request path holds private->io_mutex and initializes or modifies
private->cp in fsm_io_request(). Concurrently, the subchannel event path
(like a device offline event) holds sch->lock and unconditionally frees
private->cp in fsm_notoper().

Since the two contexts use disjoint locks, is there anything preventing them
from racing?

[Severity: High]
This is another pre-existing issue, but can fsm_notoper() sleep while holding
a spinlock with interrupts disabled?

When a subchannel event occurs, vfio_ccw_sch_event() acquires sch->lock using
spin_lock_irqsave(). It then calls vfio_ccw_fsm_event(), which executes
fsm_notoper().

fsm_notoper() calls cp_free(), which can eventually reach
vfio_iommu_type1_unpin_pages(). That function acquires a sleepable mutex:

drivers/vfio/vfio_iommu_type1.c:vfio_iommu_type1_unpin_pages()
    ...
    mutex_lock(&iommu->lock);
    ...

Could this lead to a deadlock or crash if triggered by hardware or
virtualization events while a guest is operating?

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