[PATCH v1 0/6] s390/vfio_ccw fixes

[PATCH v1 0/6] s390/vfio_ccw fixes

[Eric Farman]

This series addresses some pre-existing issues found in the
s390 vfio_ccw (dasd passthrough) driver.

Eric Farman (6):
  s390/vfio_ccw: fix out of bounds check on CCW array
  s390/vfio_ccw: limit the number of channel program segments
  s390/vfio_ccw: free all memory if cp_init() fails
  s390/vfio_ccw: copy maximum possible IDAL from guest
  s390/vfio_ccw: ensure index for read/write regions are within range
  s390/vfio_ccw: lock I/O resources alongside I/O regions

 drivers/s390/cio/vfio_ccw_chp.c     |  2 +-
 drivers/s390/cio/vfio_ccw_cp.c      | 58 +++++++++++++++++++++--------
 drivers/s390/cio/vfio_ccw_cp.h      |  7 ++++
 drivers/s390/cio/vfio_ccw_drv.c     |  2 +
 drivers/s390/cio/vfio_ccw_fsm.c     |  5 +++
 drivers/s390/cio/vfio_ccw_ops.c     | 13 ++++++-
 drivers/s390/cio/vfio_ccw_private.h |  2 +-
 7 files changed, 70 insertions(+), 19 deletions(-)

-- 
2.53.0

[PATCH v1 1/6] s390/vfio_ccw: fix out of bounds check on CCW array

[Eric Farman]

The routine ccwchain_calc_length() counts the number of channel
command words (CCWs) that are chained together in a single channel
program, and rejects anything larger than CCWCHAIN_LEN_MAX (256) CCWs.

The loop itself is "do..while (count < 257)", and while the logic in
is_cpa_within_range() correctly adjusts between the 0-index array of
CCWs and the count of CCWs starting at 1, this means it would look
at a possible 257th CCW before ending the loop and (correctly)
returning an error.

Fix this by limiting the loop to 256 CCWs such that only indexes 0-255
are examined.

Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 7561aa7d3e01..80c3d87f5482 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -393,9 +393,9 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
 			break;
 
 		ccw++;
-	} while (cnt < CCWCHAIN_LEN_MAX + 1);
+	} while (cnt < CCWCHAIN_LEN_MAX);
 
-	if (cnt == CCWCHAIN_LEN_MAX + 1)
+	if (cnt >= CCWCHAIN_LEN_MAX)
 		cnt = -EINVAL;
 
 	return cnt;
-- 
2.53.0

Re: [PATCH v1 1/6] s390/vfio_ccw: fix out of bounds check on CCW array

[sashiko-bot]

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

New issues:
- [High] The patch introduces an off-by-one regression that incorrectly rejects valid channel programs consisting of exactly CCWCHAIN_LEN_MAX (256) CCWs.

Pre-existing issues:
- [High] Leaked ccwchain structures when a subsequent TIC processing fails.
--

--- Patch [1]: [PATCH v1 1/6] s390/vfio_ccw: fix out of bounds check on CCW array ---
commit ccfdffefc50adaae37db8e069d19bc3ab3f7721c
Author: Eric Farman <farman@linux.ibm.com>
s390/vfio_ccw: fix out of bounds check on CCW array

This commit limits the channel command word counting loop to 256 iterations
to prevent out-of-bounds array access. It changes the loop condition and
the subsequent error check to reject chains larger than 256 words.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 7561aa7d3e017..80c3d87f54822 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -393,9 +393,9 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  			break;
>  
>  		ccw++;
> -	} while (cnt < CCWCHAIN_LEN_MAX + 1);
> +	} while (cnt < CCWCHAIN_LEN_MAX);
>  
> -	if (cnt == CCWCHAIN_LEN_MAX + 1)
> +	if (cnt >= CCWCHAIN_LEN_MAX)

[Severity: High]
Does this code incorrectly reject valid channel programs consisting of
exactly 256 command words?

If a guest issues a valid request with exactly 256 command words in a chain,
the loop will correctly process all of them and cnt will reach 256. The
new check evaluates to true since 256 >= 256, incorrectly conflating
this valid termination with an over-length chain and unconditionally
returning -EINVAL.

>  		cnt = -EINVAL;
>  
>  	return cnt;

[Severity: High]
This is a pre-existing issue, but does this code leak ccwchain structures
if a subsequent transfer in channel fails?

Looking at ccwchain_handle_ccw(), if a guest issues a request with
multiple transfer in channel commands, and an earlier command is valid but
a later one fails, the valid chain allocated during ccwchain_loop_tic()
is added to cp->ccwchain_list.

When the invalid command fails, ccwchain_handle_ccw() catches the error
and only frees its own top-level chain:

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

	if (ret)
		ccwchain_free(chain);
...
}

The error is returned to cp_init(), which leaves cp->initialized as false:

drivers/s390/cio/vfio_ccw_cp.c:cp_init() {
...
	INIT_LIST_HEAD(&cp->ccwchain_list);
...
	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);

	if (!ret)
		cp->initialized = true;
...
}

Because cp->initialized is false, cp_free() ignores the list during
device teardown:

drivers/s390/cio/vfio_ccw_cp.c:cp_free() {
...
	if (!cp->initialized)
		return;
...
}

When the next request arrives, cp_init() calls INIT_LIST_HEAD again,
which permanently orphans the leaked chains on the list.

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

[PATCH v1 2/6] s390/vfio_ccw: limit the number of channel program segments

[Eric Farman]

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.

Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 5 +++++
 drivers/s390/cio/vfio_ccw_cp.h | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 80c3d87f5482..76632b18fc37 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -319,6 +319,9 @@ static struct ccwchain *ccwchain_alloc(struct channel_program *cp, int len)
 {
 	struct ccwchain *chain;
 
+	if (cp->ccwchain_count >= CCWCHAIN_COUNT_MAX)
+		return NULL;
+
 	chain = kzalloc_obj(*chain);
 	if (!chain)
 		return NULL;
@@ -332,6 +335,7 @@ static struct ccwchain *ccwchain_alloc(struct channel_program *cp, int len)
 		goto out_err;
 
 	list_add_tail(&chain->next, &cp->ccwchain_list);
+	cp->ccwchain_count++;
 
 	return chain;
 
@@ -731,6 +735,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));
 
diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
index fc31eb699807..dc91a317ef19 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -23,6 +23,11 @@
  */
 #define CCWCHAIN_LEN_MAX	256
 
+/*
+ * Maximum number of chains
+ */
+#define CCWCHAIN_COUNT_MAX	16
+
 /**
  * struct channel_program - manage information for channel program
  * @ccwchain_list: list head of ccwchains
@@ -38,6 +43,7 @@ struct channel_program {
 	union orb orb;
 	bool initialized;
 	struct ccw1 *guest_cp;
+	int ccwchain_count;
 };
 
 int cp_init(struct channel_program *cp, union orb *orb);
-- 
2.53.0

Re: [PATCH v1 2/6] s390/vfio_ccw: limit the number of channel program segments

[sashiko-bot]

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

Pre-existing issues:
- [High] A memory leak in the channel program translation error path allows an unprivileged guest to deterministically exhaust host kernel memory.
--

--- Patch [2]: [PATCH v1 2/6] s390/vfio_ccw: limit the number of channel program segments ---
commit 60b67e17bbec7656c3dc68a47e90805965d6fb93
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 80c3d87f54822..76632b18fc37c 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -319,6 +319,9 @@ static struct ccwchain *ccwchain_alloc(struct channel_program *cp, int len)
>  {
>  	struct ccwchain *chain;
>  
> +	if (cp->ccwchain_count >= CCWCHAIN_COUNT_MAX)
> +		return NULL;
> +
>  	chain = kzalloc_obj(*chain);
>  	if (!chain)
>  		return NULL;

[Severity: High]
This is a pre-existing issue, but does this new limit make a memory leak
trivial for an unprivileged guest to trigger?

If a guest constructs a channel program with multiple Transfer-In-Channel
(TIC) CCWs, an early TIC might successfully allocate and link a chain into
cp->ccwchain_list.

If a subsequent TIC in the same loop hits this new CCWCHAIN_COUNT_MAX
limit, ccwchain_alloc() will return NULL, and the error will propagate
back to cp_init():

drivers/s390/cio/vfio_ccw_cp.c:cp_init() {
    ...
    /* Build a ccwchain for the first CCW segment */
    ret = ccwchain_build_ccws(orb->cmd.cpa, cp);

    if (!ret)
        cp->initialized = true;

    return ret;
}

Since cp->initialized remains false on error, when the subsystem later
calls cp_free(), the cleanup is skipped:

drivers/s390/cio/vfio_ccw_cp.c:cp_free() {
    if (!cp->initialized)
        return;
    ...
}

The next I/O request then unconditionally wipes the list, permanently
leaking the orphaned chains:

drivers/s390/cio/vfio_ccw_cp.c:cp_init() {
    ...
    INIT_LIST_HEAD(&cp->ccwchain_list);
    ...
}

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

[PATCH v1 3/6] s390/vfio_ccw: free all memory if cp_init() fails

[Eric Farman]

The routine cp_free() is called to unpin/free any memory once an I/O
is completed successfully, or if cp_prefetch() fails. But if cp_init()
fails, and cp->initialized is not enabled, the same routine cannot be
used to free all the memory.

An attempt to address this exists in ccwchain_handle_ccw(), where a
single call to ccwchain_free() is made for the currently-processed
CCW segment. But this will leak other segments (created as a result
of a Transfer in Channel) that had been allocated as part of the same
channel program.

Address this by performing the cleanup outside of the recursive
ccwchain_handle_ccw()/ccwchain_loop_tic() logic.

Fixes: 8b515be512a2 ("vfio-ccw: Fix memory leak and don't call cp_free in cp_init")
Cc: stable@vger.kernel.org
Cc: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 76632b18fc37..74b1f25e01e7 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -459,9 +459,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);
 
-	if (ret)
-		ccwchain_free(chain);
-
 	return ret;
 }
 
@@ -490,6 +487,23 @@ static int ccwchain_loop_tic(struct ccwchain *chain, struct channel_program *cp)
 	return 0;
 }
 
+static int ccwchain_build_ccws(dma32_t cda, struct channel_program *cp)
+{
+	struct ccwchain *chain, *temp;
+	int ret;
+
+	ret = ccwchain_handle_ccw(cda, cp);
+
+	if (ret) {
+		/* Cleanup if an error occurred */
+		list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
+			ccwchain_free(chain);
+		}
+	}
+
+	return ret;
+}
+
 static int ccwchain_fetch_tic(struct ccw1 *ccw,
 			      struct channel_program *cp)
 {
@@ -740,7 +754,7 @@ int cp_init(struct channel_program *cp, union orb *orb)
 	memcpy(&cp->orb, orb, sizeof(*orb));
 
 	/* Build a ccwchain for the first CCW segment */
-	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
+	ret = ccwchain_build_ccws(orb->cmd.cpa, cp);
 
 	if (!ret)
 		cp->initialized = true;
-- 
2.53.0

Re: [PATCH v1 3/6] s390/vfio_ccw: free all memory if cp_init() fails

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Concurrent execution of `cp_free()` can lead to a double-free of ccwchain structures and use-after-free of unpinned pages.
--

--- Patch [3]: [PATCH v1 3/6] s390/vfio_ccw: free all memory if cp_init() fails ---
commit f65cc16ffb4e4ab4d98caf412ccd788513657f79
Author: Eric Farman <farman@linux.ibm.com>

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

This commit refactors the cleanup of ccwchain segments during
initialization. It moves the memory freeing outside of the recursive
ccwchain handling logic to prevent memory leaks when cp_init() fails.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 76632b18fc37c..74b1f25e01e72 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c

[ ... ]

> @@ -740,7 +754,7 @@ int cp_init(struct channel_program *cp, union orb *orb)
>  	memcpy(&cp->orb, orb, sizeof(*orb));
>  
>  	/* Build a ccwchain for the first CCW segment */
> -	ret = ccwchain_handle_ccw(orb->cmd.cpa, cp);
> +	ret = ccwchain_build_ccws(orb->cmd.cpa, cp);
>  
>  	if (!ret)
>  		cp->initialized = true;

[Severity: Critical]
This is a pre-existing issue, but could the concurrent execution of
cp_free() on this initialized channel program lead to a double-free of
ccwchain structures and a use-after-free of unpinned pages?

If a guest VM issues a channel program to an mdev to cause a subchannel
interrupt and then immediately triggers a vfio device reset ioctl, could
vfio_ccw_sch_io_todo() and fsm_close() race?

In drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo(), the workqueue
path reads private->state and calls cp_free() without acquiring io_mutex
or sch->lock:

        if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
                cp_free(&private->cp);
                cp_is_finished = true;
        }

Concurrently, fsm_close() can acquire sch->lock, successfully disable the
subchannel, set private->state = VFIO_CCW_STATE_STANDBY, drop the lock,
and call cp_free().

If vfio_ccw_sch_io_todo() reads the state as VFIO_CCW_STATE_CP_PENDING
before fsm_close() updates it, both threads might execute cp_free()
simultaneously.

In drivers/s390/cio/vfio_ccw_cp.c:cp_free(), there is a lockless check:

        if (!cp->initialized)
                return;

        cp->initialized = false;

Does this lockless time-of-check to time-of-use allow both threads to 
proceed and call kfree() on the same ccwchain structures, potentially
leading to a host kernel panic?

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

[PATCH v1 4/6] s390/vfio_ccw: copy maximum possible IDAL from guest

[Eric Farman]

An Indirect Data Address word is always 2K/4K aligned (depending on
format/type), except for the first word in a list. This unaligned
word makes calculating the number of addresses in a list challenging.
The current code attempts to be efficient about this by reading the
first word before making its calculations, but it introduces
inefficiencies trying to do the math on supposedly equal values.

Since an IDAL list cannot cross a 2K/4K boundary, copy the maximum
possible list in a way similar to guest_cp, and use that as the source
for populating the host IDAL.

Fixes: 01aa26c672c0 ("s390/cio: Combine direct and indirect CCW paths")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c  | 27 +++++++++++++++++----------
 drivers/s390/cio/vfio_ccw_cp.h  |  1 +
 drivers/s390/cio/vfio_ccw_ops.c |  9 ++++++++-
 3 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 74b1f25e01e7..dac53e26509e 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -523,16 +523,24 @@ static int ccwchain_fetch_tic(struct ccw1 *ccw,
 	return -EFAULT;
 }
 
+static int calc_max_idal_len(struct ccw1 *ccw, struct channel_program *cp)
+{
+	int idal_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
+	int idal_mask = ~(idal_size - 1);
+	int idal_len = idal_size - (ccw->cda & ~idal_mask);
+
+	/* This overestimates for Format-1 or 2K-Format-2 IDAWs */
+	return idal_len / 8;
+}
+
 static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int idaw_nr)
 {
-	struct vfio_device *vdev =
-		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
 	dma64_t *idaws;
 	dma32_t *idaws_f1;
 	int idal_len = idaw_nr * sizeof(*idaws);
 	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
 	int idaw_mask = ~(idaw_size - 1);
-	int i, ret;
+	int i;
 
 	idaws = kzalloc_objs(*idaws, idaw_nr, GFP_DMA | GFP_KERNEL);
 	if (!idaws)
@@ -540,11 +548,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
 
 	if (ccw_is_idal(ccw)) {
 		/* Copy IDAL from guest */
-		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), idaws, idal_len, false);
-		if (ret) {
-			kfree(idaws);
-			return ERR_PTR(ret);
-		}
+		memcpy(idaws, cp->guest_idal, idal_len);
 	} else {
 		/* Fabricate an IDAL based off CCW data address */
 		if (cp->orb.cmd.c64) {
@@ -586,7 +590,7 @@ static int ccw_count_idaws(struct ccw1 *ccw,
 	struct vfio_device *vdev =
 		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
 	u64 iova;
-	int size = cp->orb.cmd.c64 ? sizeof(u64) : sizeof(u32);
+	int size = calc_max_idal_len(ccw, cp);
 	int ret;
 	int bytes = 1;
 
@@ -596,10 +600,13 @@ static int ccw_count_idaws(struct ccw1 *ccw,
 	if (ccw_is_idal(ccw)) {
 		/* Read first IDAW to check its starting address. */
 		/* All subsequent IDAWs will be 2K- or 4K-aligned. */
-		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), &iova, size, false);
+		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda),
+				  cp->guest_idal, size, false);
 		if (ret)
 			return ret;
 
+		iova = cp->guest_idal[0];
+
 		/*
 		 * Format-1 IDAWs only occupy the first 32 bits,
 		 * and bit 0 is always off.
diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
index dc91a317ef19..f33fea569b14 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -43,6 +43,7 @@ struct channel_program {
 	union orb orb;
 	bool initialized;
 	struct ccw1 *guest_cp;
+	dma64_t *guest_idal;
 	int ccwchain_count;
 };
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 45ec722d25ea..afe9448c165e 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -55,9 +55,13 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
 	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
 
+	private->cp.guest_idal = kzalloc_objs(dma64_t, 512);
+	if (!private->cp.guest_idal)
+		goto out_free_private;
+
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
-		goto out_free_private;
+		goto out_free_idal;
 
 	private->io_region = kmem_cache_zalloc(vfio_ccw_io_region,
 					       GFP_KERNEL | GFP_DMA);
@@ -89,6 +93,8 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
 	kmem_cache_free(vfio_ccw_io_region, private->io_region);
 out_free_cp:
 	kfree(private->cp.guest_cp);
+out_free_idal:
+	kfree(private->cp.guest_idal);
 out_free_private:
 	mutex_destroy(&private->io_mutex);
 	return -ENOMEM;
@@ -141,6 +147,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
 	kmem_cache_free(vfio_ccw_io_region, private->io_region);
 	kfree(private->cp.guest_cp);
+	kfree(private->cp.guest_idal);
 	mutex_destroy(&private->io_mutex);
 }
 
-- 
2.53.0

Re: [PATCH v1 4/6] s390/vfio_ccw: copy maximum possible IDAL from guest

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] calc_max_idal_len() returns a word count instead of a byte length, causing truncated guest memory reads and silent guest memory corruption.
--

--- Patch [4]: [PATCH v1 4/6] s390/vfio_ccw: copy maximum possible IDAL from guest ---
commit 3ab642e4c4a6646b5b0bec62ab380c4cc0ea0ac2
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: copy maximum possible IDAL from guest

An Indirect Data Address word is always 2K/4K aligned (depending on
format/type), except for the first word in a list. This unaligned
word makes calculating the number of addresses in a list challenging.
The current code attempts to be efficient about this by reading the
first word before making its calculations, but it introduces
inefficiencies trying to do the math on supposedly equal values.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 74b1f25e01e72..dac53e26509e5 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -523,16 +523,24 @@ static int ccwchain_fetch_tic(struct ccw1 *ccw,
>  	return -EFAULT;
>  }
>  
> +static int calc_max_idal_len(struct ccw1 *ccw, struct channel_program *cp)
> +{
> +	int idal_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
> +	int idal_mask = ~(idal_size - 1);
> +	int idal_len = idal_size - (ccw->cda & ~idal_mask);
> +
> +	/* This overestimates for Format-1 or 2K-Format-2 IDAWs */
> +	return idal_len / 8;
> +}
> +
>  static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int idaw_nr)
>  {
> -	struct vfio_device *vdev =
> -		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
>  	dma64_t *idaws;
>  	dma32_t *idaws_f1;
>  	int idal_len = idaw_nr * sizeof(*idaws);
>  	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
>  	int idaw_mask = ~(idaw_size - 1);
> -	int i, ret;
> +	int i;
>  
>  	idaws = kzalloc_objs(*idaws, idaw_nr, GFP_DMA | GFP_KERNEL);
>  	if (!idaws)
> @@ -540,11 +548,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
>  
>  	if (ccw_is_idal(ccw)) {
>  		/* Copy IDAL from guest */
> -		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), idaws, idal_len, false);
> -		if (ret) {
> -			kfree(idaws);
> -			return ERR_PTR(ret);
> -		}
> +		memcpy(idaws, cp->guest_idal, idal_len);
>  	} else {
>  		/* Fabricate an IDAL based off CCW data address */
>  		if (cp->orb.cmd.c64) {

[ ... ]

> @@ -586,7 +590,7 @@ static int ccw_count_idaws(struct ccw1 *ccw,
>  	struct vfio_device *vdev =
>  		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
>  	u64 iova;
> -	int size = cp->orb.cmd.c64 ? sizeof(u64) : sizeof(u32);
> +	int size = calc_max_idal_len(ccw, cp);
>  	int ret;
>  	int bytes = 1;
>  
> @@ -596,10 +600,13 @@ static int ccw_count_idaws(struct ccw1 *ccw,
>  	if (ccw_is_idal(ccw)) {
>  		/* Read first IDAW to check its starting address. */
>  		/* All subsequent IDAWs will be 2K- or 4K-aligned. */
> -		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), &iova, size, false);
> +		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda),
> +				  cp->guest_idal, size, false);

[Severity: High]
Does this pass a word count instead of a byte count to vfio_dma_rw()?

calc_max_idal_len() returns a word count because it divides the remaining bytes
by 8. When this value is stored in size and passed to vfio_dma_rw(), it seems
it will read at most 1/8th of the required IDAL data.

Later in get_guest_idal(), the code copies the full required byte length from
the partially-filled cp->guest_idal buffer:

	memcpy(idaws, cp->guest_idal, idal_len);

Could this result in copying stale or uninitialized data to the host IDAL,
causing the hardware to perform DMA to incorrect guest physical addresses?

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

[PATCH v1 5/6] s390/vfio_ccw: ensure index for read/write regions are within range

[Eric Farman]

The introduction of the capability chain rightly clamped the
region indexes to the range of the capabilities itself, but
neglected to do so for the existing read/write regions which
should also be enforced.

Fixes: db8e5d17ac03 ("vfio-ccw: add capabilities chain")
Cc: stable@vger.kernel.org
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index afe9448c165e..63cf5850bd50 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -245,6 +245,8 @@ static ssize_t vfio_ccw_mdev_read(struct vfio_device *vdev,
 	if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions)
 		return -EINVAL;
 
+	index = array_index_nospec(index, VFIO_CCW_NUM_REGIONS + private->num_regions);
+
 	switch (index) {
 	case VFIO_CCW_CONFIG_REGION_INDEX:
 		return vfio_ccw_mdev_read_io_region(private, buf, count, ppos);
@@ -297,6 +299,8 @@ static ssize_t vfio_ccw_mdev_write(struct vfio_device *vdev,
 	if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions)
 		return -EINVAL;
 
+	index = array_index_nospec(index, VFIO_CCW_NUM_REGIONS + private->num_regions);
+
 	switch (index) {
 	case VFIO_CCW_CONFIG_REGION_INDEX:
 		return vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
-- 
2.53.0

Re: [PATCH v1 5/6] s390/vfio_ccw: ensure index for read/write regions are within range

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Flawed usage of `array_index_nospec()` incorrectly clamps out-of-bounds indices to 0 before a subtraction, leading to a speculative integer underflow and out-of-bounds array access.
--

--- Patch [5]: [PATCH v1 5/6] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit f00821fcd31baacca490cf51ca9a9206a56fdf45
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: ensure index for read/write regions are within range

The introduction of the capability chain clamped the region indexes to the
range of the capabilities itself, but neglected to do so for the existing
read/write regions. This commit enforces the same checks for those regions.

> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -245,6 +245,8 @@ static ssize_t vfio_ccw_mdev_read(struct vfio_device *vdev,
>  	if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions)
>  		return -EINVAL;
>  
> +	index = array_index_nospec(index, VFIO_CCW_NUM_REGIONS + private->num_regions);
> +
>  	switch (index) {
>  	case VFIO_CCW_CONFIG_REGION_INDEX:
>  		return vfio_ccw_mdev_read_io_region(private, buf, count, ppos);
>  	default:
>  		index -= VFIO_CCW_NUM_REGIONS;
>  		return private->region[index].ops->read(private, buf, count,
>  							ppos);

[Severity: High]
Does this introduce a speculative out-of-bounds access regression?

If an attacker provides an out-of-bounds index, the architectural bounds
check is bypassed during speculative execution. The array_index_nospec()
mitigation will clamp the out-of-bounds index to 0 to resolve data
dependencies safely.

However, if the CPU mispredicts the switch statement and executes the
default case, the clamped index of 0 is subtracted by
VFIO_CCW_NUM_REGIONS. This causes an integer underflow, leading to a
speculative read of private->region with a negative index, followed by
a speculative indirect call via ops->read().

Could we perform the array_index_nospec() masking after the subtraction,
specifically clamping against private->num_regions instead?

[ ... ]

> @@ -297,6 +299,8 @@ static ssize_t vfio_ccw_mdev_write(struct vfio_device *vdev,
>  	if (index >= VFIO_CCW_NUM_REGIONS + private->num_regions)
>  		return -EINVAL;
>  
> +	index = array_index_nospec(index, VFIO_CCW_NUM_REGIONS + private->num_regions);
> +
>  	switch (index) {
>  	case VFIO_CCW_CONFIG_REGION_INDEX:
>  		return vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
>  	default:
>  		index -= VFIO_CCW_NUM_REGIONS;
>  		return private->region[index].ops->write(private, buf, count,
>  							 ppos);

[Severity: High]
Will this suffer from the same speculative integer underflow as the read
path?

If array_index_nospec() clamps an out-of-bounds index to 0, the
subsequent subtraction will result in an underflow, allowing a
speculative out-of-bounds read and an indirect call through ops->write().

Additionally, it looks like this identical anti-speculation pattern is
already present in other locations such as
vfio_ccw_mdev_ioctl_get_region_info(),
intel_vgpu_ioctl_get_region_info(), and vfio_pci_ioctl_get_region_info().

Do those locations also need to be updated?

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

[PATCH v1 6/6] s390/vfio_ccw: lock I/O resources alongside I/O regions

[Eric Farman]

The memory regions shared with userspace for vfio-ccw operations
are correctly accessed under a lock, but there are a handful of
related structures that are associated with the same lifespan of
a given SSCH (and thus the written-to memory region).

Some of these cases are done asynchronously from the guest
(e.g., hot-unplug of a device or channel path event), and so
should be protected in some similar way. Since a subchannel can
only have one I/O active at a time, redefine the I/O mutex from
protecting the region, to all the resources associated with the I/O.

Fixes: 4f76617378ee ("vfio-ccw: protect the I/O region")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_chp.c     | 2 +-
 drivers/s390/cio/vfio_ccw_drv.c     | 2 ++
 drivers/s390/cio/vfio_ccw_fsm.c     | 5 +++++
 drivers/s390/cio/vfio_ccw_private.h | 2 +-
 4 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 38c176cf6295..872620a9488d 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -90,13 +90,13 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 	if (pos + count > sizeof(*region))
 		return -EINVAL;
 
+	mutex_lock(&private->io_mutex);
 	crw = list_first_entry_or_null(&private->crw,
 				       struct vfio_ccw_crw, next);
 
 	if (crw)
 		list_del(&crw->next);
 
-	mutex_lock(&private->io_mutex);
 	region = private->region[i].data;
 
 	if (crw)
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 1a095085bc72..385af7daca3b 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -292,7 +292,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
 	crw->crw.erc = erc;
 	crw->crw.rsid = rsid;
 
+	mutex_lock(&private->io_mutex);
 	list_add_tail(&crw->next, &private->crw);
+	mutex_unlock(&private->io_mutex);
 	queue_work(vfio_ccw_work_q, &private->crw_work);
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 4d7988ea47ef..96f23da88a39 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -171,7 +171,9 @@ static void fsm_notoper(struct vfio_ccw_private *private,
 	private->state = VFIO_CCW_STATE_NOT_OPER;
 
 	/* This is usually handled during CLOSE event */
+	mutex_lock(&private->io_mutex);
 	cp_free(&private->cp);
+	mutex_unlock(&private->io_mutex);
 }
 
 /*
@@ -410,7 +412,10 @@ static void fsm_close(struct vfio_ccw_private *private,
 
 	private->state = VFIO_CCW_STATE_STANDBY;
 	spin_unlock_irq(&sch->lock);
+
+	mutex_lock(&private->io_mutex);
 	cp_free(&private->cp);
+	mutex_unlock(&private->io_mutex);
 	return;
 
 err_unlock:
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 0501d4bbcdbd..8f3792fdd31b 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -88,7 +88,7 @@ struct vfio_ccw_parent {
  * @state: internal state of the device
  * @completion: synchronization helper of the I/O completion
  * @io_region: MMIO region to input/output I/O arguments/results
- * @io_mutex: protect against concurrent update of I/O regions
+ * @io_mutex: protect against concurrent update of I/O resources
  * @region: additional regions for other subchannel operations
  * @cmd_region: MMIO region for asynchronous I/O commands other than START
  * @schib_region: MMIO region for SCHIB information
-- 
2.53.0

Re: [PATCH v1 6/6] s390/vfio_ccw: lock I/O resources alongside I/O regions

[sashiko-bot]

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

New issues:
- [High] The patch introduces 'sleeping in atomic context' bugs by adding mutex_lock() calls in paths where a spinlock is held and interrupts are disabled.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but concurrent execution of `cp_free` can lead to a double free because `vfio_ccw_sch_io_todo` fails to hold `io_mutex`.
--

--- Patch [6]: [PATCH v1 6/6] s390/vfio_ccw: lock I/O resources alongside I/O regions ---
commit 60e83e6e496f63d77b199d14299ecacbd61f8fb1
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: lock I/O resources alongside I/O regions
    
The commit expands the scope of the I/O mutex from protecting just the
memory regions to also protecting related resources like channel programs
and subchannel events. It adds mutex locking around CRW queuing and
channel program freeing.

Cc: stable@vger.kernel.org

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1a095085bc728..385af7daca3b4 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -292,7 +292,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
>  	crw->crw.erc = erc;
>  	crw->crw.rsid = rsid;
>  
> +	mutex_lock(&private->io_mutex);

[Severity: High]
Does this cause a sleep-in-atomic bug?

The CIO core invokes the chp_event callback with the subchannel lock held
and interrupts disabled:

drivers/s390/cio/chsc.c:__s390_subchannel_vary_chpid() {
    spin_lock_irqsave(&sch->lock, flags);
    if (sch->driver && sch->driver->chp_event)
        sch->driver->chp_event(sch, &link, on ? CHP_VARY_ON : CHP_VARY_OFF);
    ...
}

This eventually calls vfio_ccw_queue_crw(), which now attempts to acquire
the sleeping io_mutex while the spinlock is held.

>  	list_add_tail(&crw->next, &private->crw);
> +	mutex_unlock(&private->io_mutex);
>  	queue_work(vfio_ccw_work_q, &private->crw_work);
>  }

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d7988ea47ef0..96f23da88a396 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -171,7 +171,9 @@ static void fsm_notoper(struct vfio_ccw_private *private,
>  	private->state = VFIO_CCW_STATE_NOT_OPER;
>  
>  	/* This is usually handled during CLOSE event */
> +	mutex_lock(&private->io_mutex);

[Severity: High]
Can this also trigger a sleep-in-atomic panic?

Similar to the CRW queuing path, fsm_notoper() is reached from
vfio_ccw_sch_event(), which holds the same subchannel spinlock:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_event() {
    spin_lock_irqsave(&sch->lock, flags);
    ...
    vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of locking in
vfio_ccw_sch_io_todo() allow a double-free race with this cp_free() call?

While this patch adds io_mutex protection here in fsm_notoper(), the
I/O completion workqueue conditionally calls cp_free() without acquiring
io_mutex:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo() {
    ...
    if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
        cp_free(&private->cp);
    ...
}

Because cp_free() checks and modifies cp->initialized non-atomically:

drivers/s390/cio/vfio_ccw_cp.c:cp_free() {
    if (!cp->initialized)
        return;
    cp->initialized = false;
    ...
}

could these two paths execute the freeing logic simultaneously if a
subchannel becomes not operational exactly as its I/O completes?

>  	cp_free(&private->cp);
> +	mutex_unlock(&private->io_mutex);
>  }

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