Re: [GIT PULL v1 04/23] 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:
- [High] Guest memory over-read in get_guest_idal() for Format-1 IDALs requests twice the actual size of the guest's IDAL, potentially causing valid channel programs to be rejected.
- [High] When processing a CCW with the SKIP flag and IDA flag, the driver fails to translate the guest IDAWs into valid host physical addresses, potentially causing a host Channel Program Check.
--

--- Patch [4]: [GIT PULL v1 04/23] s390/vfio_ccw: Ensure first IDAW remains constant ---
commit fb0447391e53154ceda5e61ab696ff1e49d88abb
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: Ensure first IDAW remains constant

The first IDAW in a list does not need to be on a 2K/4K boundary
like all others, and so is read separately to accurately calculate
the size of the buffer needed to read the full IDAL. Verify that
the address found in the first IDAW is unchanged between reads,
to ensure a consistent set of IDAWs being worked with.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index af632f9d54535..6275794751cb4 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -523,6 +523,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 isn't a bug introduced by this patch, but does this over-calculate the
length for Format-1 IDALs?

Since Format-1 IDAWs are only 4 bytes long, but sizeof(*idaws) is 8 bytes,
idal_len ends up being twice the actual size.

When vfio_dma_rw() is called a few lines later:

    ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), idaws, idal_len, false);

could this attempt to read past the allocated IDAL size in guest memory? If
the over-read crosses into an unmapped page, it might spuriously reject a
valid guest channel program.

[ ... ]

I also noticed another issue in the same file.

[Severity: High]
This is a pre-existing issue, but do we correctly handle guest IDAW
translation when a CCW has both the SKIP and IDA flags set?

For such commands, ccw_does_data_transfer() returns 0, bypassing page
pinning:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_fetch_ccw() {
    ...
    if (!ccw_does_data_transfer(ccw)) {
        ...
        pa->pa_nr = 0;
    }
    ...
    page_array_idal_create_words(pa, idaws);
}

Because pa->pa_nr is 0, page_array_idal_create_words() returns without doing
anything, leaving the raw guest virtual addresses (IOVAs) in the idaws array.

Will the host channel subsystem hardware fetch these invalid physical
addresses and raise an access exception (Program Check), failing the valid
guest request?

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