Re: [PATCH v2 4/7] s390/vfio_ccw: ensure first IDAW remains constant

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but concurrent execution of a device reset or DMA unmap with an active I/O request leads to a critical use-after-free and data corruption of the channel program structures.
- [High] This problem wasn't introduced by this patch, but the IDAL read length calculation in `get_guest_idal()` for Format-1 IDAWs incorrectly calculates the length as if they were 64-bit IDAWs, leading to an out-of-bounds read from guest memory.
--

--- Patch [4]: [PATCH v2 4/7] s390/vfio_ccw: ensure first IDAW remains constant ---
commit d9b51975a2a2546c96fb0396ccf078c5c296fe37
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: ensure first IDAW remains constant

This patch ensures the first IDAW in a list remains constant by verifying
its address between reads, and returning an error if a mismatch is found.
This helps maintain a consistent set of IDAWs for I/O operations.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index ce61858c82eb6..c6f0264fb5d33 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -533,6 +533,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
>  		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
>  	dma64_t *idaws;
>  	dma32_t *idaws_f1;
> +	u64 first_idaw;
>  	int idal_len = idaw_nr * sizeof(*idaws);
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
This is a pre-existing issue, but the IDAL read length calculation in
get_guest_idal() for Format-1 IDAWs incorrectly calculates the length as if
they were 64-bit IDAWs, leading to an out-of-bounds read from guest memory.

Because Format-1 IDAWs are actually 32 bits (4 bytes) long, this instructs
vfio_dma_rw() to read exactly twice the valid IDAL size.

If the IDAL resides near the boundary of the guest's mapped IOMMU DMA region,
this over-read spans into an unmapped area, causing vfio_dma_rw() to reject
the read and fail the legitimate channel program.

Does this code need to calculate the length based on the actual IDAW size
instead of the host's 8-byte target struct dma64_t?

>  	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
>  	int idaw_mask = ~(idaw_size - 1);

[ ... ]

Additionally, there is another regression present in the state machine:

[Severity: Critical]
This is a pre-existing issue, but concurrent execution of a device reset or
DMA unmap with an active I/O request leads to a use-after-free and data
corruption of the channel program structures.

If an unprivileged guest triggers an I/O operation, invoking
vfio_ccw_mdev_write_io_region() in the host, this function takes
private->io_mutex and pushes VFIO_CCW_EVENT_IO_REQ to the state machine,
invoking fsm_io_request().

Here, private->state is set to VFIO_CCW_STATE_CP_PROCESSING, and
cp_init()/cp_prefetch() are called without holding any locks:

drivers/s390/cio/vfio_ccw_fsm.c:fsm_io_request()
    private->state = VFIO_CCW_STATE_CP_PROCESSING;
    //...
    io_region->ret_code = cp_init(&private->cp, orb);
    //...
    io_region->ret_code = cp_prefetch(&private->cp);

Concurrently, if the guest alters its IOMMU mappings, it triggers
vfio_ccw_dma_unmap() on the host. This callback checks cp_iova_pinned() without
holding any locks and then executes vfio_ccw_mdev_reset(), which pushes
VFIO_CCW_EVENT_CLOSE to the state machine without acquiring private->io_mutex:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_reset()
    vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);

Since the state is currently CP_PROCESSING, this routes to fsm_close(), which
unconditionally frees the structures concurrently with the active initialization
routine:

drivers/s390/cio/vfio_ccw_fsm.c:fsm_close()
    cp_free(&private->cp);

Can this sequence actually occur and cause cp_free() to free the ccwchain
memory while cp_prefetch() is actively modifying it? Should these events be
serialized to prevent this?

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