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

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 chan=
nel program
- [High] Out-of-bounds read in ccwchain_calc_length
--

--- Patch [2]: [GIT PULL v1 02/23] s390/vfio_ccw: Limit the number of chann=
el program segments ---
commit f04946000e5f4fc0876ae6db794b58a1b8744e08
Author: Eric Farman <[email protected]>

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_c=
p.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 c=
hannel_program *cp)
>  	if (len < 0)
>  		return len;

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

If a guest provides a channel program with 256 command-chained CCWs, the lo=
op
in ccwchain_calc_length() reaches cnt =3D=3D 256 and increments the ccw poi=
nter
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 >=3D CCWCHAIN_COUNT_MAX)
> +		return -EINVAL;
> +
>  	/* Need alloc a new chain for this one. */
>  	chain =3D ccwchain_alloc(cp, len);
>  	if (!chain)
[ ... ]
> @@ -745,6 +750,7 @@ int cp_init(struct channel_program *cp, union orb *or=
b)
>  			vdev->dev,
>  			"Prefetching channel program even though prefetch not specified in OR=
B");
> =20
> +	cp->ccwchain_count =3D 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 corruptio=
n?

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 holdi=
ng
a spinlock with interrupts disabled?

When a subchannel event occurs, vfio_ccw_sch_event() acquires sch->lock usi=
ng
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?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803160924.2368=
[email protected]?part=3D2