[PATCH v5 00/10] s390/vfio_ccw fixes

[PATCH v5 00/10] s390/vfio_ccw fixes

[Eric Farman]

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

v1: https://lore.kernel.org/r/20260714232208.1683788-1-farman@linux.ibm.com/
v2: https://lore.kernel.org/r/20260720201931.976660-1-farman@linux.ibm.com/
v3: https://lore.kernel.org/r/20260723174751.1180334-1-farman@linux.ibm.com/
v4: https://lore.kernel.org/r/20260725152705.3958100-1-farman@linux.ibm.com/
v4->v5:
 - [MR] Added r-b where provided (thank you!)
 - [NEW][MR] Call cancel_work_sync() for existing workqueue structs
 - [MR] Add cancel for new notoper_work workqueue
 - [MR] Commentary change to definition of crw_lock
 - [EF] Acquire/release crw_lock with irqsave/restore

Eric Farman (10):
  s390/vfio_ccw: free all memory if cp_init() fails
  s390/vfio_ccw: limit the number of channel program segments
  s390/vfio_ccw: fix out of bounds check on CCW array
  s390/vfio_ccw: ensure first IDAW remains constant
  s390/vfio_ccw: calculate idal length based on idaw type
  s390/vfio_ccw: ensure index for read/write regions are within range
  s390/vfio_ccw: cancel existing workqueues
  s390/vfio_ccw: move cp cleanup out of not operational
  s390/vfio_ccw: implement a channel program mutex
  s390/vfio_ccw: implement a crw lock

 drivers/s390/cio/vfio_ccw_async.c   | 16 +++++
 drivers/s390/cio/vfio_ccw_chp.c     | 31 ++++++++--
 drivers/s390/cio/vfio_ccw_cp.c      | 96 ++++++++++++++++++++++-------
 drivers/s390/cio/vfio_ccw_cp.h      | 10 +++
 drivers/s390/cio/vfio_ccw_drv.c     | 21 +++++++
 drivers/s390/cio/vfio_ccw_fsm.c     | 23 ++++---
 drivers/s390/cio/vfio_ccw_ops.c     | 26 ++++++--
 drivers/s390/cio/vfio_ccw_private.h | 10 +++
 8 files changed, 192 insertions(+), 41 deletions(-)

-- 
2.53.0

[PATCH v5 01/10] 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
Reviewed-by: Farhan Ali<alifm@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@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 7561aa7d3e01..086d1b54bdb0 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -455,9 +455,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;
 }
 
@@ -486,6 +483,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)
 {
@@ -735,7 +749,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 v5 01/10] s390/vfio_ccw: free all memory if cp_init() fails

[sashiko-bot]

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

Pre-existing issues:
- [High] Unbounded mutual recursion between `ccwchain_handle_ccw` and `ccwchain_loop_tic` can lead to kernel stack exhaustion.
- [High] Out-of-bounds read of `cp->guest_cp` in `ccwchain_calc_length`.
--

--- Patch [1]: [PATCH v5 01/10] s390/vfio_ccw: free all memory if cp_init() fails ---
commit fee68998110983d79a8ac5ec5666d53fcd26abad
Author: Eric Farman <farman@linux.ibm.com>

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

The commit addresses a memory leak in the channel program initialization
process. It moves the cleanup of allocated CCW segments out of the
recursive ccwchain_handle_ccw() logic into a new wrapper function
ccwchain_build_ccws(). This ensures that all segments are freed if an
error occurs.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 7561aa7d3e017..086d1b54bdb05 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -455,9 +455,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);

[Severity: High]
This is a pre-existing issue, but can this unbounded mutual recursion lead to
kernel stack exhaustion?

If a malicious guest constructs a channel program consisting of a long sequence
of forward-jumping Transfer in Channel (TIC) commands that do not loop back to
previously processed chains, it could trigger unchecked mutual recursion.

When the guest issues an SSCH instruction, the host kernel intercepts it and
evaluates the program. ccwchain_handle_ccw() calls ccwchain_loop_tic(), which
in turn calls ccwchain_handle_ccw() for any new segments:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_loop_tic() {
	...
		/* Build a ccwchain for the next segment */
		ret = ccwchain_handle_ccw(tic->cda, cp);
		if (ret)
			return ret;
	...
}

Since there is no enforced limit on the recursion depth or the total number of
chains, a long sequence of non-cyclic TICs could cause uncontrolled mutual
recursion, exhausting the kernel stack and causing a host panic.

[Severity: High]
This is also a pre-existing issue, but does ccwchain_calc_length() have an
out-of-bounds read vulnerability?

When ccwchain_handle_ccw() allocates cp->guest_cp, it copies a maximum of
CCWCHAIN_LEN_MAX (256) elements:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_handle_ccw() {
	...
	ret = vfio_dma_rw(vdev, gcda, cp->guest_cp,
			  CCWCHAIN_LEN_MAX * sizeof(struct ccw1), false);
	...
	/* Count the CCWs in the current chain */
	len = ccwchain_calc_length(gcda, cp);
	...
}

However, in ccwchain_calc_length(), the do-while loop condition allows up to
CCWCHAIN_LEN_MAX + 1 iterations:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_calc_length() {
	...
	do {
		...
		ccw = cp->guest_cp + cnt;
		...
		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
			break;
		...
	} while (cnt < CCWCHAIN_LEN_MAX + 1);
	...
}

If a malicious guest submits a channel program where the first 256 CCWs all
have the command-chaining or data-chaining flags enabled, the loop will not
break early and will execute a 257th iteration.

During this iteration, ccw points to the 257th element (index 256), and
ccw_is_chain(ccw) reads its flags byte. This reads past the 2K allocation
boundary, potentially triggering a host kernel panic or memory safety tooling.

>  
> -	if (ret)
> -		ccwchain_free(chain);
> -
>  	return ret;
>  }

[ ... ]

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

[PATCH v5 02/10] 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
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 6 ++++++
 drivers/s390/cio/vfio_ccw_cp.h | 8 ++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 086d1b54bdb0..1c2890d139c6 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -332,6 +332,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;
 
@@ -441,6 +442,10 @@ static int ccwchain_handle_ccw(dma32_t cda, struct channel_program *cp)
 	if (len < 0)
 		return len;
 
+	/* 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));
 
diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
index fc31eb699807..a9b1d8dbc6f6 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -23,11 +23,18 @@
  */
 #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
  * @orb: orb for the currently processed ssch request
  * @initialized: whether this instance is actually initialized
+ * @guest_cp: copy of guest channel program
+ * @ccwchain_count: number of channel program segments (linked by TIC)
  *
  * @ccwchain_list is the head of a ccwchain list, that contents the
  * translated result of the guest channel program that pointed out by
@@ -38,6 +45,7 @@ struct channel_program {
 	union orb orb;
 	bool initialized;
 	struct ccw1 *guest_cp;
+	unsigned int ccwchain_count;
 };
 
 int cp_init(struct channel_program *cp, union orb *orb);
-- 
2.53.0

Re: [PATCH v5 02/10] s390/vfio_ccw: limit the number of channel program segments

[sashiko-bot]

> 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
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

[PATCH v5 03/10] 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 restructuring the loop to break as soon as 256 CCWs
(thus indexes 0-255) are examined, without looking at memory
outside the range.

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 1c2890d139c6..af632f9d5453 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -377,11 +377,9 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)
 static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
 {
 	struct ccw1 *ccw = cp->guest_cp;
-	int cnt = 0;
-
-	do {
-		cnt++;
+	int cnt;
 
+	for (cnt = 1; cnt <= CCWCHAIN_LEN_MAX; cnt++, ccw++) {
 		/*
 		 * We want to keep counting if the current CCW has the
 		 * command-chaining flag enabled, or if it is a TIC CCW
@@ -391,15 +389,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
 		 * after the TIC, depending on the results of its operation.
 		 */
 		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
-			break;
-
-		ccw++;
-	} while (cnt < CCWCHAIN_LEN_MAX + 1);
-
-	if (cnt == CCWCHAIN_LEN_MAX + 1)
-		cnt = -EINVAL;
+			return cnt;
+	}
 
-	return cnt;
+	return -EINVAL;
 }
 
 static int tic_target_chain_exists(struct ccw1 *tic, struct channel_program *cp)
-- 
2.53.0

Re: [PATCH v5 03/10] s390/vfio_ccw: fix out of bounds check on CCW array

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Hardware Status Modifier skips allow the channel subsystem to fetch and execute CCWs from out-of-bounds host memory because VFIO prematurely truncates the allocated chain.
--

--- Patch [3]: [PATCH v5 03/10] s390/vfio_ccw: fix out of bounds check on CCW array ---
commit 7c8a75a862f0fe5c95ff1ad636c202fa276e66c2
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: fix out of bounds check on CCW array

This commit fixes an out-of-bounds array access in ccwchain_calc_length().
It restructures the loop to exit safely once the maximum number of CCWs
(256) have been processed, preventing the code from evaluating memory
outside the valid range.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c

[ ... ]

> @@ -377,11 +377,9 @@ static void ccwchain_cda_free(struct ccwchain *chain, int idx)
>  static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  {
>  	struct ccw1 *ccw = cp->guest_cp;
> -	int cnt = 0;
> -
> -	do {
> -		cnt++;
> +	int cnt;
>  
> +	for (cnt = 1; cnt <= CCWCHAIN_LEN_MAX; cnt++, ccw++) {
>  		/*
>  		 * We want to keep counting if the current CCW has the
>  		 * command-chaining flag enabled, or if it is a TIC CCW
> @@ -391,15 +389,10 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
>  		 * after the TIC, depending on the results of its operation.
>  		 */
>  		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
> -			break;
> -
> -		ccw++;
> -	} while (cnt < CCWCHAIN_LEN_MAX + 1);
> -
> -	if (cnt == CCWCHAIN_LEN_MAX + 1)
> -		cnt = -EINVAL;
> +			return cnt;

[Severity: Critical]
This is a pre-existing issue, but does this termination condition fully
account for hardware status modifier skips?

If a guest constructs a channel program where a CCW specifies chaining
and executes a command that generates a status modifier, and the
immediately following CCW explicitly clears the chaining flag, will
ccwchain_calc_length() stop counting at the second CCW?

If the calculation stops there, it appears the subsystem would allocate a host
buffer sized for exactly two CCWs. When executed, the hardware would see
the status modifier, skip the second CCW, and fetch a third CCW from
memory immediately following the allocated buffer.

Could this allow the channel subsystem to fetch and execute an untranslated,
out-of-bounds CCW from host memory if the guest places one adjacent to the
allocation?

> +	}
>  
> -	return cnt;
> +	return -EINVAL;
>  }
>  
>  static int tic_target_chain_exists(struct ccw1 *tic, struct channel_program *cp)

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

[PATCH v5 04/10] s390/vfio_ccw: ensure first IDAW remains constant

[Eric Farman]

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.

Fixes: 01aa26c672c0 ("s390/cio: Combine direct and indirect CCW paths")
Cc: stable@vger.kernel.org
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 16 ++++++++++++++++
 drivers/s390/cio/vfio_ccw_cp.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index af632f9d5453..6275794751cb 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);
 	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
 	int idaw_mask = ~(idaw_size - 1);
@@ -539,6 +540,18 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
 			kfree(idaws);
 			return ERR_PTR(ret);
 		}
+
+		idaws_f1 = (dma32_t *)idaws;
+		if (cp->orb.cmd.c64)
+			first_idaw = dma64_to_u64(idaws[0]);
+		else
+			first_idaw = dma32_to_u32(idaws_f1[0]);
+
+		/* Unexpected mismatch from earlier read */
+		if (first_idaw != cp->guest_iova) {
+			kfree(idaws);
+			return ERR_PTR(-EINVAL);
+		}
 	} else {
 		/* Fabricate an IDAL based off CCW data address */
 		if (cp->orb.cmd.c64) {
@@ -604,6 +617,9 @@ static int ccw_count_idaws(struct ccw1 *ccw,
 		iova = dma32_to_u32(ccw->cda);
 	}
 
+	/* Save the read address for later */
+	cp->guest_iova = iova;
+
 	/* Format-1 IDAWs operate on 2K each */
 	if (!cp->orb.cmd.c64)
 		return idal_2k_nr_words((void *)iova, bytes);
diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
index a9b1d8dbc6f6..9af98ff12d67 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -35,6 +35,7 @@
  * @initialized: whether this instance is actually initialized
  * @guest_cp: copy of guest channel program
  * @ccwchain_count: number of channel program segments (linked by TIC)
+ * @guest_iova: first data address of a guest channel program
  *
  * @ccwchain_list is the head of a ccwchain list, that contents the
  * translated result of the guest channel program that pointed out by
@@ -46,6 +47,7 @@ struct channel_program {
 	bool initialized;
 	struct ccw1 *guest_cp;
 	unsigned int ccwchain_count;
+	u64 guest_iova;
 };
 
 int cp_init(struct channel_program *cp, union orb *orb);
-- 
2.53.0

Re: [PATCH v5 04/10] s390/vfio_ccw: ensure first IDAW remains constant

[sashiko-bot]

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

Pre-existing issues:
- [High] The function get_guest_idal over-reads the guest's IDAL buffer when processing 32-bit (Format-1) IDAWs, potentially causing spurious I/O failures if the read crosses into unmapped memory.
--

--- Patch [4]: [PATCH v5 04/10] s390/vfio_ccw: ensure first IDAW remains constant ---
commit 9c17ea86e465307c28b3fca8602d9a6e74a2028b
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: ensure first IDAW remains constant

This commit verifies that the first IDAW read remains unchanged between reads
to ensure a consistent set of IDAWs is used for the channel program.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> --- 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 is a pre-existing issue, but does calculating idal_len using
sizeof(*idaws) (which is 8 bytes for dma64_t) cause an out-of-bounds
read when processing 32-bit Format-1 IDAWs?

Since vfio_dma_rw() is called with this idal_len further down in 
get_guest_idal():

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

Could this over-read cross into unmapped or unpinned memory pages and cause
spurious I/O failures if a guest happens to place its Format-1 IDAL near the
end of a mapped page?

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

[PATCH v5 05/10] s390/vfio_ccw: calculate idal length based on idaw type

[Eric Farman]

Sashiko pointed out that get_guest_idal() unconditionally calculates
the length of the IDAL presuming everything is a Format-2 IDAW.

The output of vfio-ccw is always Format-2, but the input can be either
Format-1 (31-bit addresses) or Format-2 (64-bit addresses). As a result,
the size of the guest IDAL may be incorrect and should be trimmed down.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/r/20260720203400.7328E1F000E9@smtp.kernel.org/
Fixes: 1b676fe3d9d3 ("vfio/ccw: handle a guest Format-1 IDAL")
Cc: stable@vger.kernel.org
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 6275794751cb..5ef082b8289a 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -233,6 +233,7 @@ static void convert_ccw0_to_ccw1(struct ccw1 *source, unsigned long len)
 }
 
 #define idal_is_2k(_cp) (!(_cp)->orb.cmd.c64 || (_cp)->orb.cmd.i2k)
+#define get_idaw_size(_cp) ((_cp)->orb.cmd.c64 ? sizeof(u64) : sizeof(u32))
 
 /*
  * Helpers to operate ccwchain.
@@ -524,7 +525,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
 	dma64_t *idaws;
 	dma32_t *idaws_f1;
 	u64 first_idaw;
-	int idal_len = idaw_nr * sizeof(*idaws);
+	int idal_len = idaw_nr * get_idaw_size(cp);
 	int idaw_size = idal_is_2k(cp) ? PAGE_SIZE / 2 : PAGE_SIZE;
 	int idaw_mask = ~(idaw_size - 1);
 	int i, ret;
@@ -593,7 +594,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 = get_idaw_size(cp);
 	int ret;
 	int bytes = 1;
 
-- 
2.53.0

Re: [PATCH v5 05/10] s390/vfio_ccw: calculate idal length based on idaw type

[sashiko-bot]

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

Pre-existing issues:
- [High] Guest IOVAs are passed untranslated to the host channel subsystem for skipped I/O operations, allowing physical memory probing.
--

--- Patch [5]: [PATCH v5 05/10] s390/vfio_ccw: calculate idal length based on idaw type ---
commit 8013df87ffdce265ecd327b26da1e700483c050d
Author: Eric Farman <farman@linux.ibm.com>
Subject: s390/vfio_ccw: calculate idal length based on idaw type

The patch fixes the length calculation of IDAL in get_guest_idal() so it
doesn't unconditionally assume Format-2 IDAW sizes. Input IDALs can be
Format-1 (31-bit) or Format-2 (64-bit), requiring dynamic size calculation.

Link: https://lore.kernel.org/r/20260720203400.7328E1F000E9@smtp.kernel.org/

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 6275794751cb4..5ef082b8289af 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
[ ... ]
> @@ -524,7 +525,7 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
>  	dma64_t *idaws;
>  	dma32_t *idaws_f1;
>  	u64 first_idaw;
> -	int idal_len = idaw_nr * sizeof(*idaws);
> +	int idal_len = idaw_nr * get_idaw_size(cp);

[Severity: High]
This is a pre-existing issue, but I have a question about how we handle
skipped I/O operations. Is it possible for untranslated guest IOVAs to
be leaked to the host hardware if the CCW has the SKIP flag set or if
count is 0?

When get_guest_idal() populates the idaws array, it copies the IDAL
directly from the guest. Later, if ccw_does_data_transfer() returns
false, ccwchain_fetch_ccw() skips pinning and sets pa->pa_nr = 0:

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_fetch_ccw() {
	...
	if (ccw_does_data_transfer(ccw)) {
		ret = page_array_pin(pa, vdev, idal_is_2k(cp));
		if (ret < 0)
			goto out_unpin;
	} else {
		pa->pa_nr = 0;
	}
	...
}

This then causes page_array_idal_create_words() to perform zero iterations
because its loop is bounded by pa->pa_nr:

drivers/s390/cio/vfio_ccw_cp.c:page_array_idal_create_words() {
	...
	for (i = 0; i < pa->pa_nr; i++) {
		idaws[i] = virt_to_dma64(page_to_virt(pa->pa_page[i]));
	}
	...
}

Would this result in the guest IOVAs remaining in the idaws array,
leaving the untranslated guest addresses exposed to the host hardware?

Since the hardware still validates physical addresses even for skipped
operations, could a malicious guest use this to probe host physical
memory and bypass KASLR?

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

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

[PATCH v5 06/10] 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>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_async.c | 16 ++++++++++++++++
 drivers/s390/cio/vfio_ccw_chp.c   | 15 +++++++++++++++
 drivers/s390/cio/vfio_ccw_ops.c   |  7 +++----
 3 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
index 420d89ba7f83..4aff0b58fa5d 100644
--- a/drivers/s390/cio/vfio_ccw_async.c
+++ b/drivers/s390/cio/vfio_ccw_async.c
@@ -8,6 +8,7 @@
  */
 
 #include <linux/vfio.h>
+#include <linux/nospec.h>
 
 #include "vfio_ccw_private.h"
 
@@ -24,11 +25,20 @@ static ssize_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
 		return -EINVAL;
 
 	mutex_lock(&private->io_mutex);
+
+	if (i >= private->num_regions) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 	if (copy_to_user(buf, (void *)region + pos, count))
 		ret = -EFAULT;
 	else
 		ret = count;
+
+out_unlock:
 	mutex_unlock(&private->io_mutex);
 	return ret;
 }
@@ -48,6 +58,12 @@ static ssize_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
 	if (!mutex_trylock(&private->io_mutex))
 		return -EAGAIN;
 
+	if (i >= private->num_regions) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 	if (copy_from_user((void *)region + pos, buf, count)) {
 		ret = -EFAULT;
diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 38c176cf6295..f3015132d4b5 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/slab.h>
+#include <linux/nospec.h>
 #include <linux/vfio.h>
 #include "vfio_ccw_private.h"
 
@@ -26,6 +27,13 @@ static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
 		return -EINVAL;
 
 	mutex_lock(&private->io_mutex);
+
+	if (i >= private->num_regions) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 
 	if (cio_update_schib(sch)) {
@@ -97,6 +105,12 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 		list_del(&crw->next);
 
 	mutex_lock(&private->io_mutex);
+	if (i >= private->num_regions) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 
 	if (crw)
@@ -109,6 +123,7 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 
 	region->crw = 0;
 
+out:
 	mutex_unlock(&private->io_mutex);
 
 	kfree(crw);
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 45ec722d25ea..032a1cdf4df7 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -243,6 +243,7 @@ static ssize_t vfio_ccw_mdev_read(struct vfio_device *vdev,
 		return vfio_ccw_mdev_read_io_region(private, buf, count, ppos);
 	default:
 		index -= VFIO_CCW_NUM_REGIONS;
+		index = array_index_nospec(index, private->num_regions);
 		return private->region[index].ops->read(private, buf, count,
 							ppos);
 	}
@@ -295,6 +296,7 @@ static ssize_t vfio_ccw_mdev_write(struct vfio_device *vdev,
 		return vfio_ccw_mdev_write_io_region(private, buf, count, ppos);
 	default:
 		index -= VFIO_CCW_NUM_REGIONS;
+		index = array_index_nospec(index, private->num_regions);
 		return private->region[index].ops->write(private, buf, count,
 							 ppos);
 	}
@@ -338,11 +340,8 @@ static int vfio_ccw_mdev_ioctl_get_region_info(struct vfio_device *vdev,
 		    VFIO_CCW_NUM_REGIONS + private->num_regions)
 			return -EINVAL;
 
-		info->index = array_index_nospec(info->index,
-						 VFIO_CCW_NUM_REGIONS +
-						 private->num_regions);
-
 		i = info->index - VFIO_CCW_NUM_REGIONS;
+		i = array_index_nospec(i, private->num_regions);
 
 		info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
 		info->size = private->region[i].size;
-- 
2.53.0

Re: [PATCH v5 06/10] s390/vfio_ccw: ensure index for read/write regions are within range

[sashiko-bot]

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

Pre-existing issues:
- [High] Lockless operations on the `private->crw` linked list can lead to list corruption, Use-After-Free, and double free.
- [High] Missing cancellation of background work items (`crw_work` and `io_work`) during device teardown allows them to execute on freed memory.
--

--- Patch [6]: [PATCH v5 06/10] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit 9645c29c603d4b1935d8c4e94d4ecac3dcd69174
Author: Eric Farman <farman@linux.ibm.com>

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

This commit clamps region indexes for read/write operations to be within
the bounds of private->num_regions. It also uses array_index_nospec() to
prevent speculative out-of-bounds access.

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 38c176cf62957..f3015132d4b5f 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c

[ ... ]

> @@ -97,6 +105,12 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  		list_del(&crw->next);

[Severity: High]
This is a pre-existing issue, but does this locklessly dequeue elements from
private->crw and cause list corruption or use-after-free?

The userspace read syscall path calls vfio_ccw_crw_region_read(), which
dequeues elements from private->crw using list_del():

drivers/s390/cio/vfio_ccw_chp.c:vfio_ccw_crw_region_read() {
	...
	crw = list_first_entry_or_null(&private->crw, struct vfio_ccw_crw, next);
	if (crw)
		list_del(&crw->next);
	...
}

Concurrently, the asynchronous hardware event path locklessly appends elements
to private->crw using list_add_tail():

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_queue_crw() {
	...
	list_add_tail(&crw->next, &private->crw);
	queue_work(vfio_ccw_work_q, &private->crw_work);
	...
}

Could an unprivileged guest with VFIO access trigger this race by executing
a read() syscall on the CRW region concurrently with a hardware asynchronous
channel path event to crash the host kernel?

>  
>  	mutex_lock(&private->io_mutex);
> +	if (i >= private->num_regions) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	i = array_index_nospec(i, private->num_regions);
>  	region = private->region[i].data;

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 45ec722d25ea6..032a1cdf4df70 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c

[ ... ]

> @@ -338,11 +340,8 @@ static int vfio_ccw_mdev_ioctl_get_region_info(struct vfio_device *vdev,
>  		    VFIO_CCW_NUM_REGIONS + private->num_regions)
>  			return -EINVAL;
>  
> -		info->index = array_index_nospec(info->index,
> -						 VFIO_CCW_NUM_REGIONS +
> -						 private->num_regions);
> -
>  		i = info->index - VFIO_CCW_NUM_REGIONS;
> +		i = array_index_nospec(i, private->num_regions);

[Severity: High]
This is a pre-existing issue, but does device teardown safely cancel background
work items before freeing memory?

Asynchronous events can queue crw_work (via vfio_ccw_chp_event) or io_work
(via fsm_irq). When the device is torn down, fsm_close() conditionally flushes
the workqueue only on an error/busy path, leaving pending work active on the
success path:

drivers/s390/cio/vfio_ccw_fsm.c:fsm_close() {
	...
	ret = cio_disable_subchannel(sch);
	if (ret == -EBUSY)
		ret = vfio_ccw_sch_quiesce(sch);
	...
}

Then, vfio_ccw_mdev_release_dev() frees the private structure and destroys the
mutex without invoking cancel_work_sync() for crw_work and io_work:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_release_dev() {
	...
	kfree(private->cp.guest_cp);
	mutex_destroy(&private->io_mutex);
	...
}

Can this lead to a use-after-free on the vfio_ccw_private structure if the
pending work executes on the freed private pointer after it is destroyed by
the VFIO core?

>  
>  		info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
>  		info->size = private->region[i].size;

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

[PATCH v5 07/10] s390/vfio_ccw: cancel existing workqueues

[Eric Farman]

The initialization of the io_work and crw_work workqueues begs the
question of whether they should be un-initialized. Add the corresponding
cleanup tags in _release_dev to ensure work isn't dispatched after
the private struct is free'd.

Suggested-by: Matthew Rosato <mjrosato@linux.ibm.com>
Fixes: e5f84dbaea59 ("vfio: ccw: return I/O results asynchronously")
Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 032a1cdf4df7..9048c1e6e7f0 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -136,6 +136,9 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 		kfree(crw);
 	}
 
+	cancel_work_sync(&private->io_work);
+	cancel_work_sync(&private->crw_work);
+
 	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
 	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
 	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
-- 
2.53.0

Re: [PATCH v5 07/10] s390/vfio_ccw: cancel existing workqueues

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The `private->crw` list is manipulated without locks causing list corruption, and `cancel_work_sync()` is placed after list teardown.
- [Critical] Concurrent execution of CSS event handlers and `vfio_ccw_mdev_remove` leads to Use-After-Free of the `private` struct.
- [High] Eventfd contexts for IRQ triggers are permanently leaked when the device is closed.
- [High] Data race on `private->irb` leading to corrupted I/O status being exposed to userspace.
- [High] Unprotected access to eventfd triggers can lead to Use-After-Free during event signaling.
--

--- Patch [7]: [PATCH v5 07/10] s390/vfio_ccw: cancel existing workqueues ---
commit 5e12c09173779e05794585584185ded44763e3e3
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: cancel existing workqueues

This commit adds cancel_work_sync() calls in vfio_ccw_mdev_release_dev()
to ensure work items are canceled and not dispatched after the private struct
is freed.

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 032a1cdf4df70..9048c1e6e7f0c 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -136,6 +136,9 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  		kfree(crw);
>  	}

[Severity: Critical]
Is it safe to place cancel_work_sync(&private->crw_work) after the teardown
of the private->crw list?

If the list teardown loop runs first, any work item queued by a hardware event
after the loop but before cancellation could be leaked. Additionally, because
cancel_work_sync() blocks until running work finishes, placing it after the
list teardown means crw_work could run concurrently with the teardown and
access the list while it is being freed.

This is a pre-existing issue, but it appears the lack of locking when
manipulating private->crw in vfio_ccw_queue_crw() and
vfio_ccw_crw_region_read() could also lead to list corruption if executed
concurrently.

> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does
vfio_ccw_mdev_release_dev() leak the eventfd contexts when tearing down the
device?

In vfio_ccw_mdev_set_irqs(), the driver takes references to eventfd_ctx
objects via eventfd_ctx_fdget(). However, these references do not appear to
be released via eventfd_ctx_put() during device cleanup here or in
vfio_ccw_mdev_close_device(). Could unprivileged userspace exploit this
resource leak by configuring IRQs and then closing the VFIO device fd?

[Severity: High]
This is a pre-existing issue, but can a data race occur on private->irb?

Hardware interrupts handled by fsm_irq() in vfio_ccw_fsm.c overwrite
private->irb while holding sch->lock. It then queues
vfio_ccw_sch_io_todo(), which reads private->irb locklessly before taking
io_mutex:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo() {
    ...
    irb = &private->irb;
    is_final = !(scsw_actl(&irb->scsw) & ...);
    ...
}

If a subsequent subchannel interrupt arrives asynchronously while
vfio_ccw_sch_io_todo() is reading the structure, might this expose torn or
corrupted I/O status to userspace?

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential
use-after-free when accessing the eventfd triggers?

When userspace disables an IRQ, vfio_ccw_mdev_set_irqs() frees the eventfd
context locklessly:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_set_irqs() {
    ...
    if (*ctx)
        eventfd_ctx_put(*ctx);
    *ctx = NULL;
    ...
}

Concurrently, vfio_ccw_sch_io_todo() reads private->io_trigger locklessly
and passes it to eventfd_signal():

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo() {
    ...
    if (private->io_trigger)
        eventfd_signal(private->io_trigger);
    ...
}

If the context is freed immediately after the workqueue reads the pointer,
will eventfd_signal() dereference freed memory?

[Severity: Critical]
This is a pre-existing issue, but does vfio_ccw_mdev_remove() race with
concurrent CSS event handlers?

vfio_ccw_mdev_remove() clears the drvdata and drops the VFIO device
reference, which frees the private structure:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_remove() {
    ...
    dev_set_drvdata(&parent->dev, NULL);
    vfio_put_device(&private->vdev);
    ...
}

Concurrently, vfio_ccw_chp_event() fetches private locklessly and
dereferences it:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_chp_event() {
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
    ...
    vfio_ccw_queue_crw(private, ...);
    ...
}

If the removal completes before vfio_ccw_chp_event() accesses private,
could this result in a kernel crash due to accessing freed memory?

>  	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
>  	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
>  	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);

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

[PATCH v5 08/10] s390/vfio_ccw: move cp cleanup out of not operational

[Eric Farman]

The fsm_notoper() routine is called when the device has been
lost, and is (by definition) no longer operational. Since this
can happen asynchronously from the normal behavior of the
driver, the cleanup may happen when holding other locks
in the calling sequence (notably, the cio subchannel lock).

Push the cleanup of the private->cp resources to a workqueue,
where it can be done out from under that lock sequence and
(soon) under its own serialization mechanism.

Fixes: 204b394a23ad ("vfio/ccw: Move FSM open/close to MDEV open/close")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 9 +++++++++
 drivers/s390/cio/vfio_ccw_fsm.c     | 3 +--
 drivers/s390/cio/vfio_ccw_ops.c     | 2 ++
 drivers/s390/cio/vfio_ccw_private.h | 3 +++
 4 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 1a095085bc72..c197ad5ab580 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -125,6 +125,15 @@ void vfio_ccw_crw_todo(struct work_struct *work)
 		eventfd_signal(private->crw_trigger);
 }
 
+void vfio_ccw_notoper_todo(struct work_struct *work)
+{
+	struct vfio_ccw_private *private;
+
+	private = container_of(work, struct vfio_ccw_private, notoper_work);
+
+	cp_free(&private->cp);
+}
+
 /*
  * Css driver callbacks
  */
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 4d7988ea47ef..4d47a3c7b9a0 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -170,8 +170,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
 	css_sched_sch_todo(sch, SCH_TODO_UNREG);
 	private->state = VFIO_CCW_STATE_NOT_OPER;
 
-	/* This is usually handled during CLOSE event */
-	cp_free(&private->cp);
+	queue_work(vfio_ccw_work_q, &private->notoper_work);
 }
 
 /*
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 9048c1e6e7f0..26c19f0e5e47 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -54,6 +54,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
 	INIT_LIST_HEAD(&private->crw);
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
 	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
+	INIT_WORK(&private->notoper_work, vfio_ccw_notoper_todo);
 
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
@@ -138,6 +139,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 
 	cancel_work_sync(&private->io_work);
 	cancel_work_sync(&private->crw_work);
+	cancel_work_sync(&private->notoper_work);
 
 	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
 	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 0501d4bbcdbd..e2256402b089 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -102,6 +102,7 @@ struct vfio_ccw_parent {
  * @req_trigger: eventfd ctx for signaling userspace to return device
  * @io_work: work for deferral process of I/O handling
  * @crw_work: work for deferral process of CRW handling
+ * @notoper_work: work for deferred processing in not-operational state
  */
 struct vfio_ccw_private {
 	struct vfio_device vdev;
@@ -125,11 +126,13 @@ struct vfio_ccw_private {
 	struct eventfd_ctx	*req_trigger;
 	struct work_struct	io_work;
 	struct work_struct	crw_work;
+	struct work_struct	notoper_work;
 } __aligned(8);
 
 int vfio_ccw_sch_quiesce(struct subchannel *sch);
 void vfio_ccw_sch_io_todo(struct work_struct *work);
 void vfio_ccw_crw_todo(struct work_struct *work);
+void vfio_ccw_notoper_todo(struct work_struct *work);
 
 extern struct mdev_driver vfio_ccw_mdev_driver;
 
-- 
2.53.0

Re: [PATCH v5 08/10] s390/vfio_ccw: move cp cleanup out of not operational

[sashiko-bot]

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

New issues:
- [High] Canceling `notoper_work` during device release can silently skip the cleanup of channel program resources, causing a memory leak of pinned pages and ccwchains.

Pre-existing issues:
- [High] Asynchronous execution of `cp_free` in `vfio_ccw_notoper_todo` without locks races with other accesses to `private->cp`, leading to data corruption, double-frees, or use-after-free.
--

--- Patch [8]: [PATCH v5 08/10] s390/vfio_ccw: move cp cleanup out of not operational ---
commit a24ecbe23bcd8a2b9448d2b68fd0bbf55921f7a3
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: move cp cleanup out of not operational

The fsm_notoper() routine is called when the device has been
lost, and is (by definition) no longer operational. Since this
can happen asynchronously from the normal behavior of the
driver, the cleanup may happen when holding other locks
in the calling sequence (notably, the cio subchannel lock).

Push the cleanup of the private->cp resources to a workqueue,
where it can be done out from under that lock sequence and
(soon) under its own serialization mechanism.

Fixes: 204b394a23ad ("vfio/ccw: Move FSM open/close to MDEV open/close")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1a095085bc728..c197ad5ab5805 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c

[ ... ]

> @@ -125,6 +125,15 @@ void vfio_ccw_crw_todo(struct work_struct *work)
>  		eventfd_signal(private->crw_trigger);
>  }
>  
> +void vfio_ccw_notoper_todo(struct work_struct *work)
> +{
> +	struct vfio_ccw_private *private;
> +
> +	private = container_of(work, struct vfio_ccw_private, notoper_work);
> +
> +	cp_free(&private->cp);

[Severity: High]
This is a pre-existing issue, but moving the cleanup to an asynchronous
workqueue exposes a race condition.

Does calling cp_free() here asynchronously without holding locks race with
other FSM callbacks?

For example, if fsm_close() is triggered concurrently, it calls cp_free()
synchronously outside of sch->lock. Since the cp->initialized check in
cp_free() is non-atomic:

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

Could multiple threads bypass this check concurrently, leading to double-frees
of the ccwchain_list and use-after-free conditions?

> +}
> +
>  /*
>   * Css driver callbacks
>   */

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 9048c1e6e7f0c..26c19f0e5e474 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c

[ ... ]

> @@ -138,6 +139,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);
> +	cancel_work_sync(&private->notoper_work);

[Severity: High]
If the device is closed or unbound before the workqueue has a chance to
execute, does canceling notoper_work here silently skip the cleanup of
the channel program resources?

Since fsm_notoper() changes the state to VFIO_CCW_STATE_NOT_OPER when
queuing this work, standard close handlers like fsm_close() bypass
cp_free().

Without a fallback call to cp_free() to reclaim resources if the work
execution was skipped by cancel_work_sync(), will this result in a
permanent memory leak of pinned guest pages and ccwchain structures?

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

[PATCH v5 09/10] s390/vfio_ccw: implement a channel program mutex

[Eric Farman]

The channel_program struct is manipulated without a serialization
mechanism to ensure consistent behavior. Take a broad stroke of
putting the entire structure behind a mutex, and ensure everything
that needs private->cp holds this mutex.

There are a couple where the cio layer's subchannel->lock performs
this role in this code, which isn't correct (it should only be used
when touching the actual subchannel, like cio_enable_subchannel()),
so adjust the locations where that spinlock is acquired/released
to correctly coexist with this new mutex.

Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces")
Cc: stable@vger.kernel.org
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c      | 30 +++++++++++++++++++++++++----
 drivers/s390/cio/vfio_ccw_drv.c     |  6 ++++++
 drivers/s390/cio/vfio_ccw_fsm.c     | 20 ++++++++++++-------
 drivers/s390/cio/vfio_ccw_ops.c     | 10 +++++++++-
 drivers/s390/cio/vfio_ccw_private.h |  3 +++
 5 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 5ef082b8289a..ab66caff9894 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -738,12 +738,15 @@ static int ccwchain_fetch_one(struct ccw1 *ccw,
  */
 int cp_init(struct channel_program *cp, union orb *orb)
 {
-	struct vfio_device *vdev =
-		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
+	struct vfio_ccw_private *private =
+		container_of(cp, struct vfio_ccw_private, cp);
+	struct vfio_device *vdev = &private->vdev;
 	/* custom ratelimit used to avoid flood during guest IPL */
 	static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 1);
 	int ret;
 
+	lockdep_assert_held(&private->cp_mutex);
+
 	/* this is an error in the caller */
 	if (cp->initialized)
 		return -EBUSY;
@@ -784,11 +787,14 @@ int cp_init(struct channel_program *cp, union orb *orb)
  */
 void cp_free(struct channel_program *cp)
 {
-	struct vfio_device *vdev =
-		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
+	struct vfio_ccw_private *private =
+		container_of(cp, struct vfio_ccw_private, cp);
+	struct vfio_device *vdev = &private->vdev;
 	struct ccwchain *chain, *temp;
 	int i;
 
+	lockdep_assert_held(&private->cp_mutex);
+
 	if (!cp->initialized)
 		return;
 
@@ -841,11 +847,15 @@ void cp_free(struct channel_program *cp)
  */
 int cp_prefetch(struct channel_program *cp)
 {
+	struct vfio_ccw_private *private =
+		container_of(cp, struct vfio_ccw_private, cp);
 	struct ccwchain *chain;
 	struct ccw1 *ccw;
 	struct page_array *pa;
 	int len, idx, ret;
 
+	lockdep_assert_held(&private->cp_mutex);
+
 	/* this is an error in the caller */
 	if (!cp->initialized)
 		return -EINVAL;
@@ -883,10 +893,14 @@ int cp_prefetch(struct channel_program *cp)
  */
 union orb *cp_get_orb(struct channel_program *cp, struct subchannel *sch)
 {
+	struct vfio_ccw_private *private =
+		container_of(cp, struct vfio_ccw_private, cp);
 	union orb *orb;
 	struct ccwchain *chain;
 	struct ccw1 *cpa;
 
+	lockdep_assert_held(&private->cp_mutex);
+
 	/* this is an error in the caller */
 	if (!cp->initialized)
 		return NULL;
@@ -931,10 +945,14 @@ union orb *cp_get_orb(struct channel_program *cp, struct subchannel *sch)
  */
 void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
 {
+	struct vfio_ccw_private *private =
+		container_of(cp, struct vfio_ccw_private, cp);
 	struct ccwchain *chain;
 	dma32_t cpa = scsw->cmd.cpa;
 	u32 ccw_head;
 
+	lockdep_assert_held(&private->cp_mutex);
+
 	if (!cp->initialized)
 		return;
 
@@ -977,9 +995,13 @@ void cp_update_scsw(struct channel_program *cp, union scsw *scsw)
  */
 bool cp_iova_pinned(struct channel_program *cp, u64 iova, u64 length)
 {
+	struct vfio_ccw_private *private =
+		container_of(cp, struct vfio_ccw_private, cp);
 	struct ccwchain *chain;
 	int i;
 
+	lockdep_assert_held(&private->cp_mutex);
+
 	if (!cp->initialized)
 		return false;
 
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index c197ad5ab580..4830f0dd9c3a 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -91,6 +91,8 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 
 	is_final = !(scsw_actl(&irb->scsw) &
 		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
+
+	mutex_lock(&private->cp_mutex);
 	if (scsw_is_solicited(&irb->scsw)) {
 		cp_update_scsw(&private->cp, &irb->scsw);
 		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
@@ -98,6 +100,8 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 			cp_is_finished = true;
 		}
 	}
+	mutex_unlock(&private->cp_mutex);
+
 	mutex_lock(&private->io_mutex);
 	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
 	mutex_unlock(&private->io_mutex);
@@ -131,7 +135,9 @@ void vfio_ccw_notoper_todo(struct work_struct *work)
 
 	private = container_of(work, struct vfio_ccw_private, notoper_work);
 
+	mutex_lock(&private->cp_mutex);
 	cp_free(&private->cp);
+	mutex_unlock(&private->cp_mutex);
 }
 
 /*
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 4d47a3c7b9a0..cefdfcb0cad7 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -25,17 +25,15 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
 	unsigned long flags;
 	int ret;
 
-	spin_lock_irqsave(&sch->lock, flags);
-
 	orb = cp_get_orb(&private->cp, sch);
-	if (!orb) {
-		ret = -EIO;
-		goto out;
-	}
+	if (!orb)
+		return -EIO;
 
 	VFIO_CCW_TRACE_EVENT(5, "stIO");
 	VFIO_CCW_TRACE_EVENT(5, dev_name(&sch->dev));
 
+	spin_lock_irqsave(&sch->lock, flags);
+
 	/* Issue "Start Subchannel" */
 	ccode = ssch(sch->schid, orb);
 
@@ -71,7 +69,6 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
 	default:
 		ret = ccode;
 	}
-out:
 	spin_unlock_irqrestore(&sch->lock, flags);
 	return ret;
 }
@@ -251,6 +248,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 	private->state = VFIO_CCW_STATE_CP_PROCESSING;
 	memcpy(scsw, io_region->scsw_area, sizeof(*scsw));
 
+	mutex_lock(&private->cp_mutex);
+
 	if (scsw->cmd.fctl & SCSW_FCTL_START_FUNC) {
 		orb = (union orb *)io_region->orb_area;
 
@@ -299,6 +298,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 			cp_free(&private->cp);
 			goto err_out;
 		}
+
+		mutex_unlock(&private->cp_mutex);
 		return;
 	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
 		VFIO_CCW_MSG_EVENT(2,
@@ -319,6 +320,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 	}
 
 err_out:
+	mutex_unlock(&private->cp_mutex);
 	private->state = VFIO_CCW_STATE_IDLE;
 	trace_vfio_ccw_fsm_io_request(scsw->cmd.fctl, schid,
 				      io_region->ret_code, errstr);
@@ -409,7 +411,11 @@ static void fsm_close(struct vfio_ccw_private *private,
 
 	private->state = VFIO_CCW_STATE_STANDBY;
 	spin_unlock_irq(&sch->lock);
+
+	mutex_lock(&private->cp_mutex);
 	cp_free(&private->cp);
+	mutex_unlock(&private->cp_mutex);
+
 	return;
 
 err_unlock:
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 26c19f0e5e47..b8e702f36106 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -38,8 +38,13 @@ static void vfio_ccw_dma_unmap(struct vfio_device *vdev, u64 iova, u64 length)
 		container_of(vdev, struct vfio_ccw_private, vdev);
 
 	/* Drivers MUST unpin pages in response to an invalidation. */
-	if (!cp_iova_pinned(&private->cp, iova, length))
+	mutex_lock(&private->cp_mutex);
+	if (!cp_iova_pinned(&private->cp, iova, length)) {
+		mutex_unlock(&private->cp_mutex);
 		return;
+	}
+
+	mutex_unlock(&private->cp_mutex);
 
 	vfio_ccw_mdev_reset(private);
 }
@@ -50,6 +55,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
 		container_of(vdev, struct vfio_ccw_private, vdev);
 
 	mutex_init(&private->io_mutex);
+	mutex_init(&private->cp_mutex);
 	private->state = VFIO_CCW_STATE_STANDBY;
 	INIT_LIST_HEAD(&private->crw);
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
@@ -91,6 +97,7 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
 out_free_cp:
 	kfree(private->cp.guest_cp);
 out_free_private:
+	mutex_destroy(&private->cp_mutex);
 	mutex_destroy(&private->io_mutex);
 	return -ENOMEM;
 }
@@ -146,6 +153,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);
+	mutex_destroy(&private->cp_mutex);
 	mutex_destroy(&private->io_mutex);
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index e2256402b089..b595fd81f370 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -94,6 +94,7 @@ struct vfio_ccw_parent {
  * @schib_region: MMIO region for SCHIB information
  * @crw_region: MMIO region for getting channel report words
  * @num_regions: number of additional regions
+ * @cp_mutex: protect against concurrent update of CP resources
  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
  * @scsw: scsw info
@@ -116,7 +117,9 @@ struct vfio_ccw_private {
 	struct ccw_crw_region	*crw_region;
 	int num_regions;
 
+	struct mutex		cp_mutex;
 	struct channel_program	cp;
+
 	struct irb		irb;
 	union scsw		scsw;
 	struct list_head	crw;
-- 
2.53.0

Re: [PATCH v5 09/10] s390/vfio_ccw: implement a channel program mutex

[sashiko-bot]

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

New issues:
- [High] Channel program resources are leaked if `notoper_work` is cancelled during device release.

Pre-existing issues:
- [High] Unprotected access to `private->irb` could lead to data corruption on concurrent interrupts.
--

--- Patch [9]: [PATCH v5 09/10] s390/vfio_ccw: implement a channel program mutex ---
commit cd6c396d349e5308a7574cecefee3c092057158e
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a channel program mutex

This commit introduces a mutex to serialize the manipulation of the channel
program structure (private->cp). It also adjusts where the subchannel
spinlock is acquired and released so it coexists properly with the new
mutex.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index c197ad5ab5805..4830f0dd9c3a3 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -91,6 +91,8 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  
>  	is_final = !(scsw_actl(&irb->scsw) &
>  		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));

[Severity: High]
This is a pre-existing issue, but does this code locklessly access the
shared irb structure?

In fsm_irq(), private->irb can be overwritten locklessly from hardirq
context:

fsm_irq()
    memcpy(&private->irb, irb, sizeof(*irb));

Concurrently, vfio_ccw_sch_io_todo() reads private->irb here and evaluates
its contents without acquiring a lock like sch->lock.

If a spurious or unsolicited hardware interrupt arrives while the workqueue
is processing a previous interrupt, could a torn or partially updated read
result in incorrect I/O completion status being passed to the guest?

> +
> +	mutex_lock(&private->cp_mutex);
>  	if (scsw_is_solicited(&irb->scsw)) {
>  		cp_update_scsw(&private->cp, &irb->scsw);

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 26c19f0e5e474..b8e702f36106c 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -146,6 +153,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);
> +	mutex_destroy(&private->cp_mutex);

[Severity: High]
Can this leak channel program resources?

If fsm_close() fails, it triggers a VFIO_CCW_EVENT_NOT_OPER event, which
queues notoper_work to free the channel program.

If the device is subsequently released before the workqueue executes,
vfio_ccw_mdev_release_dev() cancels the work:

vfio_ccw_mdev_release_dev() {
    ...
    cancel_work_sync(&private->notoper_work);
    ...
}

Since cancel_work_sync() aborts the cleanup without a fallback, and
vfio_ccw_mdev_release_dev() does not explicitly call cp_free(), do the
channel program structures (ccwchain) and pinned guest pages remain
leaked indefinitely?

>  	mutex_destroy(&private->io_mutex);
>  }

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

[PATCH v5 10/10] s390/vfio_ccw: implement a crw lock

[Eric Farman]

Unlike the channel_program struct, which covers synchronous I/O
submissions and asynchronous interrupts, the CRW region relies
exclusively on asynchronous events coming from hardware.

Implement a lock to manage the list of those payloads, to ensure
they are read cohesively.

Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
Cc: stable@vger.kernel.org
Cc: Farhan Ali <alifm@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_chp.c     | 26 ++++++++++++++++----------
 drivers/s390/cio/vfio_ccw_drv.c     |  6 ++++++
 drivers/s390/cio/vfio_ccw_ops.c     |  4 ++++
 drivers/s390/cio/vfio_ccw_private.h |  4 ++++
 4 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index f3015132d4b5..7708eb4d6de0 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -93,17 +93,12 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
 	struct ccw_crw_region *region;
 	struct vfio_ccw_crw *crw;
+	unsigned long flags;
 	int ret;
 
 	if (pos + count > sizeof(*region))
 		return -EINVAL;
 
-	crw = list_first_entry_or_null(&private->crw,
-				       struct vfio_ccw_crw, next);
-
-	if (crw)
-		list_del(&crw->next);
-
 	mutex_lock(&private->io_mutex);
 	if (i >= private->num_regions) {
 		ret = -EINVAL;
@@ -113,6 +108,16 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 
+	spin_lock_irqsave(&private->crw_lock, flags);
+	crw = list_first_entry_or_null(&private->crw,
+				       struct vfio_ccw_crw, next);
+
+	if (crw)
+		list_del(&crw->next);
+
+	/* Drop CRW lock while copying to userspace */
+	spin_unlock_irqrestore(&private->crw_lock, flags);
+
 	if (crw)
 		memcpy(&region->crw, &crw->crw, sizeof(region->crw));
 
@@ -122,15 +127,16 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 		ret = count;
 
 	region->crw = 0;
-
-out:
-	mutex_unlock(&private->io_mutex);
-
 	kfree(crw);
 
 	/* Notify the guest if more CRWs are on our queue */
+	spin_lock_irqsave(&private->crw_lock, flags);
 	if (!list_empty(&private->crw) && private->crw_trigger)
 		eventfd_signal(private->crw_trigger);
+	spin_unlock_irqrestore(&private->crw_lock, flags);
+
+out:
+	mutex_unlock(&private->io_mutex);
 
 	return ret;
 }
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 4830f0dd9c3a..437d7709b4a5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -122,11 +122,14 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 void vfio_ccw_crw_todo(struct work_struct *work)
 {
 	struct vfio_ccw_private *private;
+	unsigned long flags;
 
 	private = container_of(work, struct vfio_ccw_private, crw_work);
 
+	spin_lock_irqsave(&private->crw_lock, flags);
 	if (!list_empty(&private->crw) && private->crw_trigger)
 		eventfd_signal(private->crw_trigger);
+	spin_unlock_irqrestore(&private->crw_lock, flags);
 }
 
 void vfio_ccw_notoper_todo(struct work_struct *work)
@@ -290,6 +293,7 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
 			       unsigned int rsid)
 {
 	struct vfio_ccw_crw *crw;
+	unsigned long flags;
 
 	/*
 	 * If unable to allocate a CRW, just drop the event and
@@ -307,7 +311,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
 	crw->crw.erc = erc;
 	crw->crw.rsid = rsid;
 
+	spin_lock_irqsave(&private->crw_lock, flags);
 	list_add_tail(&crw->next, &private->crw);
+	spin_unlock_irqrestore(&private->crw_lock, flags);
 	queue_work(vfio_ccw_work_q, &private->crw_work);
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index b8e702f36106..2e2b05b78073 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -61,6 +61,7 @@ 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);
 	INIT_WORK(&private->notoper_work, vfio_ccw_notoper_todo);
+	spin_lock_init(&private->crw_lock);
 
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
@@ -138,11 +139,14 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 	struct vfio_ccw_private *private =
 		container_of(vdev, struct vfio_ccw_private, vdev);
 	struct vfio_ccw_crw *crw, *temp;
+	unsigned long flags;
 
+	spin_lock_irqsave(&private->crw_lock, flags);
 	list_for_each_entry_safe(crw, temp, &private->crw, next) {
 		list_del(&crw->next);
 		kfree(crw);
 	}
+	spin_unlock_irqrestore(&private->crw_lock, flags);
 
 	cancel_work_sync(&private->io_work);
 	cancel_work_sync(&private->crw_work);
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index b595fd81f370..dd8efa9ee64e 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -98,6 +98,8 @@ struct vfio_ccw_parent {
  * @cp: channel program for the current I/O operation
  * @irb: irb info received from interrupt
  * @scsw: scsw info
+ * @crw_lock: serialization of CRW list information
+ * @crw: list of Channel Report Word elements
  * @io_trigger: eventfd ctx for signaling userspace I/O results
  * @crw_trigger: eventfd ctx for signaling userspace CRW information
  * @req_trigger: eventfd ctx for signaling userspace to return device
@@ -122,6 +124,8 @@ struct vfio_ccw_private {
 
 	struct irb		irb;
 	union scsw		scsw;
+
+	spinlock_t		crw_lock;
 	struct list_head	crw;
 
 	struct eventfd_ctx	*io_trigger;
-- 
2.53.0

Re: [PATCH v5 10/10] s390/vfio_ccw: implement a crw lock

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing write-side lock in `vfio_ccw_mdev_set_irqs` renders `crw_lock` ineffective, leading to Use-After-Free of `eventfd_ctx`.
- [High] TOCTOU race between `vfio_ccw_mdev_remove` and asynchronous channel path events leads to Use-After-Free of `vfio_ccw_private`.
--

--- Patch [10]: [PATCH v5 10/10] s390/vfio_ccw: implement a crw lock ---
commit b1ac07dbd7bc9e331720d0ba13ad54e4a3ae455a
Author: Eric Farman <farman@linux.ibm.com>
s390/vfio_ccw: implement a crw lock

This commit introduces a new crw_lock to serialize access to the Channel
Report Word (CRW) list elements in the vfio_ccw driver. It replaces implicit
synchronization with explicit spinlocks when queueing, reading, and freeing
CRW structures, as well as when notifying userspace via eventfd.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 4830f0dd9c3a3..437d7709b4a56 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -122,11 +122,14 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
>  void vfio_ccw_crw_todo(struct work_struct *work)
>  {
>  	struct vfio_ccw_private *private;
> +	unsigned long flags;
>  
>  	private = container_of(work, struct vfio_ccw_private, crw_work);
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);
>  	if (!list_empty(&private->crw) && private->crw_trigger)
>  		eventfd_signal(private->crw_trigger);
> +	spin_unlock_irqrestore(&private->crw_lock, flags);

[Severity: High]
This isn't a bug introduced by this patch, but does crw_lock fully protect
crw_trigger from concurrent modification?

In vfio_ccw_mdev_set_irqs(), the eventfd context can be updated and the old
context freed without acquiring crw_lock:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_set_irqs() {
    ...
    if (*ctx)
        eventfd_ctx_put(*ctx);
    *ctx = efdctx;
    ...
}

Because the write side in vfio_ccw_mdev_set_irqs() doesn't take the lock,
could a concurrent VFIO_DEVICE_SET_IRQS ioctl free the context while
eventfd_signal() is executing here, leading to a Use-After-Free?

[ ... ]
> @@ -290,6 +293,7 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
>  			       unsigned int rsid)
>  {
>  	struct vfio_ccw_crw *crw;
> +	unsigned long flags;
>  
>  	/*
>  	 * If unable to allocate a CRW, just drop the event and
> @@ -307,7 +311,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
>  	crw->crw.erc = erc;
>  	crw->crw.rsid = rsid;
>  
> +	spin_lock_irqsave(&private->crw_lock, flags);

[Severity: High]
This is a pre-existing issue, but could this lock access trigger a
Use-After-Free if the mdev is being concurrently removed?

The CSS core calls vfio_ccw_chp_event() asynchronously. It retrieves the
private pointer locklessly and then calls this function:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_chp_event() {
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
    ...
    if (!private || !mask)
        return 0;
    ...
    vfio_ccw_queue_crw(private, ...);
}

If vfio_ccw_mdev_remove() runs concurrently, it clears the driver data and
drops the final reference:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_remove() {
    ...
    dev_set_drvdata(&parent->dev, NULL);
    vfio_put_device(&private->vdev);
    ...
}

If the channel path event handler passes the NULL check and gets preempted
before mdev removal finishes, won't taking private->crw_lock here access
freed memory?

>  	list_add_tail(&crw->next, &private->crw);
> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  	queue_work(vfio_ccw_work_q, &private->crw_work);
>  }

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