[PATCH v6 00/10] s390/vfio_ccw fixes

[PATCH v6 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/
v5: https://lore.kernel.org/r/20260726040129.2946151-1-farman@linux.ibm.com/
v5->v6:
 - [sashiko] Add cancel_work_sync() to vfio_ccw_mdev_close_dev() as the "usual"
   cleanup, and keep them in vfio_ccw_mdev_release_dev() to cover corner cases
 - [MR; offlist] Remove the proposed notoper_work queue, and move the cp_free
   from the FSM to vfio_ccw_mdev_close_dev() to ensure we cover the case where
   a device becomes not operational (due to sch_event) while I/O is in flight. 

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     | 10 +++
 drivers/s390/cio/vfio_ccw_fsm.c     | 23 ++++---
 drivers/s390/cio/vfio_ccw_ops.c     | 40 ++++++++++--
 drivers/s390/cio/vfio_ccw_private.h |  7 +++
 8 files changed, 191 insertions(+), 42 deletions(-)

-- 
2.53.0

[PATCH v6 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 v6 01/10] 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:
- [High] Unbounded mutual recursion in ccwchain_handle_ccw() and ccwchain_loop_tic() allows guest-triggered host kernel stack overflow.
--

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

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

This commit fixes a memory leak in the channel program initialization by 
moving the cleanup of allocated CCW segments outside of the recursive 
ccwchain_handle_ccw() and ccwchain_loop_tic() logic.

> 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 does this mutual recursion between
ccwchain_handle_ccw() and ccwchain_loop_tic() in 
drivers/s390/cio/vfio_ccw_cp.c have a bound to prevent a host kernel 
stack overflow?

If a guest constructs a channel program with a long sequence of
Transfer-in-Channel (TIC) instructions, each pointing to a unique guest
memory page, it appears these functions will continuously call each other:

ccwchain_handle_ccw()
  ccwchain_loop_tic()
    ccwchain_handle_ccw()

Could an unprivileged guest exploit this unbounded recursion to exhaust the
host kernel stack?

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

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

[PATCH v6 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 v6 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/20260726172153.1818056-1-farman@linux.ibm.com?part=2

[PATCH v6 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 v6 03/10] s390/vfio_ccw: fix out of bounds check on CCW array

[sashiko-bot]

> 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>

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

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

[PATCH v6 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 v6 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] Out of Bounds Memory Read in get_guest_idal for Format-1 IDALs
--

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

s390/vfio_ccw: ensure first IDAW remains constant

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

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

[Severity: High]
This is a pre-existing issue, but does this unconditionally calculate the DMA 
read length assuming 8-byte IDAWs?

Since sizeof(*idaws) evaluates to 8 for the dma64_t type, but Format-1 IDAWs 
are only 4 bytes (dma32_t), idal_len appears to be double the required size 
for Format-1.

When this over-calculated size is later passed to vfio_dma_rw() in this same
function:

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

Could this cause valid I/O requests to fail if the over-calculated length 
happens to cross into an unmapped adjacent guest memory page?

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

[PATCH v6 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 v6 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] When processing a CCW that does not transfer data (e.g., a read command with the SKIP flag set), vfio-ccw fails to translate the IDAL, passing raw guest IOVAs directly to the host channel subsystem, which causes host IOMMU faults.
--

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

s390/vfio_ccw: calculate idal length based on idaw type

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.

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
[ ... ]
> @@ -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.
[ ... ]

While reviewing this, I noticed a separate issue in this file.

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;
	}

	ccw->cda = virt_to_dma32(idaws);
	ccw->flags |= CCW_FLAG_IDA;

	/* Populate the IDAL with pinned/translated addresses from page */
	page_array_idal_create_words(pa, idaws);

[Severity: High]
This isn't a bug introduced by this patch, but when processing a CCW that does
not transfer data, such as a read command with the SKIP flag set, does vfio-ccw
fail to translate the IDAL?

Because ccw_does_data_transfer(ccw) returns false, pa->pa_nr is set to 0. This
prevents page_array_idal_create_words() from overwriting the raw guest IOVAs
with translated host addresses.

Could a guest VM submit a channel program with a CCW where ccw_is_skip() and
ccw_is_read() are true, containing an arbitrary guest physical address?

Since the untranslated guest IOVAs remain in the idaws array, passing them
directly to the host channel subsystem might trigger a host IOMMU fault or
Channel Program Check, potentially leading to a host denial of service.

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

[PATCH v6 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 v6 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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Data Race / List Corruption in private->crw
- [High] Use-After-Free due to missing cancel_work_sync in release handler
- [Low] Event Loss on Failed copy_to_user
--

--- Patch [6]: [PATCH v6 06/10] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit f91d3ca5d2a5910d7f313cb125ab777b26c1f34e
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 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. This commit
ensures read/write region indexes are within range and clamped via
array_index_nospec.

> 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);
>  
>  	mutex_lock(&private->io_mutex);

[Severity: High]
This is a pre-existing issue, but can this list modification race with
concurrent additions?

Looking at vfio_ccw_crw_region_read() just above this diff, the crw list is
popped and modified without holding any locks:

	crw = list_first_entry_or_null(&private->crw, struct vfio_ccw_crw, next);
	if (crw)
		list_del(&crw->next);

Could this race with vfio_ccw_queue_crw(), which adds to the list using
list_add_tail(&crw->next, &private->crw)? If a hardware event asynchronously
occurs while a guest reads the CRW region, could this result in list
corruption?

> +	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);

[Severity: Low]
This is a pre-existing issue, but does this unconditionally free the CRW
event even if the copy to userspace failed?

Looking at the end of vfio_ccw_crw_region_read():

	if (copy_to_user(buf, (void *)region + pos, count))
		ret = -EFAULT;
	else
		ret = count;

	region->crw = 0;

out:
	mutex_unlock(&private->io_mutex);

	kfree(crw);

If userspace provides an invalid buffer, copy_to_user() will fail and return
-EFAULT, but the event is freed rather than requeued. Can this lead to a
permanently lost Channel Report Word event?

> 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
[ ... ]
> @@ -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);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but looking at the lifetime
management in this file, is there a potential use-after-free during
device removal?

In vfio_ccw_mdev_release_dev(), the private struct and its embedded work items
are freed without calling cancel_work_sync():

	kmem_cache_free(vfio_ccw_io_region, private->io_region);
	kfree(private->cp.guest_cp);
	mutex_destroy(&private->io_mutex);

If an mdev is created but never opened by userspace, channel path events can
still trigger vfio_ccw_chp_event(), queuing private->crw_work. If the mdev is
subsequently removed without being opened, the flush_workqueue() in the close
handler is bypassed. Could a background work item then execute using the
freed vfio_ccw_private struct?

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

[PATCH v6 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 032a1cdf4df7..bd488e40e153 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -136,6 +136,10 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 		kfree(crw);
 	}
 
+	/* Should be empty, but just in case */
+	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);
@@ -202,6 +206,10 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
 		container_of(vdev, struct vfio_ccw_private, vdev);
 
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
+
+	cancel_work_sync(&private->io_work);
+	cancel_work_sync(&private->crw_work);
+
 	vfio_ccw_unregister_dev_regions(private);
 }
 
-- 
2.53.0

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

[sashiko-bot]

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

New issues:
- [Medium] The patch places `cancel_work_sync(&private->crw_work)` after the list cleanup loop, introducing a memory leak race condition.

Pre-existing issues:
- [High] Concurrent execution of `cp_free(&private->cp)` can lead to a double free of `cp->ccwchain` list elements.
- [High] The `private->crw` list is manipulated and read concurrently without any locking, leading to severe list corruption.
- [High] The `crw_work` can be queued on a freed `private` structure due to a TOCTOU race between asynchronous hardware events and device removal.
- [High] Eventfd contexts (io_trigger, crw_trigger, req_trigger) are leaked if userspace closes the device without explicitly un-setting the IRQs.
- [High] Eventfd contexts can be accessed after being freed due to a lack of synchronization when disabling IRQs.
--

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

s390/vfio_ccw: cancel existing workqueues

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 and _close_device to ensure work isn't
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..bd488e40e1531 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -136,6 +136,10 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  		kfree(crw);
>  	}
>  
> +	/* Should be empty, but just in case */
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);
> +

[Severity: Medium]
Could this introduce a race condition that leaks vfio_ccw_crw elements?

If a concurrent channel path event executes vfio_ccw_queue_crw() right after
the list cleanup loop finishes but before cancel_work_sync() is called, the
newly queued work is cancelled. This would leave the allocated crw stranded
on the list to be leaked when the overarching private structure is freed.

[Severity: High]
This is a pre-existing issue, but is the private->crw list safe from
corruption?

While the cleanup loop above uses spin_lock_irqsave(&private->crw_lock,
flags), other paths manipulate the list locklessly:

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);
    ...
}

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);
    ...
}

Can an unprivileged guest performing CRW reads concurrently with channel path
hardware events cause doubly-linked list corruption?

[Severity: High]
This isn't a bug introduced by this patch, but could there be a use-after-free
if asynchronous hardware events race with device removal?

If a hardware event triggers:

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;

And then a concurrent device removal clears drvdata and frees the private
structure:

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

When vfio_ccw_chp_event() resumes, it calls vfio_ccw_queue_crw() utilizing
the freed private pointer, which then submits a freed work_struct.
Can this be prevented by synchronizing device removal against active channel
path events?

>  	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);

[Severity: High]
This is a pre-existing issue, but does this function leak eventfd contexts?

Neither vfio_ccw_mdev_close_device() nor vfio_ccw_mdev_release_dev() calls
eventfd_ctx_put() to release the eventfd contexts (io_trigger, crw_trigger,
req_trigger). If a user application configures interrupts via
VFIO_DEVICE_SET_IRQS and then exits abruptly or closes the device without
explicitly un-setting the IRQs, are these kernel objects permanently leaked?

[ ... ]

> @@ -202,6 +206,10 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  
>  	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
> +
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);
> +
>  	vfio_ccw_unregister_dev_regions(private);
>  }

[Severity: High]
This is a pre-existing issue, but is it possible to double-free
cp->ccwchain list elements here?

vfio_ccw_mdev_close_device() triggers fsm_close(), which drops sch->lock
and explicitly calls cp_free(&private->cp):

drivers/s390/cio/vfio_ccw_fsm.c:fsm_close() {
    ...
    private->state = VFIO_CCW_STATE_STANDBY;
    spin_unlock_irq(&sch->lock);
    cp_free(&private->cp);
    ...
}

Concurrently, if a previously queued asynchronous I/O interrupt worker executes:

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);
        cp_is_finished = true;
    }
    ...
}

Since cp_free() checks and unsets cp->initialized without atomics or
locks:

drivers/s390/cio/vfio_ccw_cp.c:cp_free() {
    if (!cp->initialized)
        return;
    cp->initialized = false;
    list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
    ...
}

Can both threads execute the list teardown simultaneously, leading to memory
corruption? Also, the newly added cancel_work_sync() runs after this
event, so it doesn't prevent this race.

[Severity: High]
This isn't a bug introduced by this patch, but can eventfd contexts be accessed
after being freed due to a lack of synchronization when disabling IRQs?

Userspace invoking VFIO_DEVICE_SET_IRQS can disable the interrupt:

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

Concurrently, a worker thread might read private->io_trigger locklessly:

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

Since eventfd_ctx_put(*ctx) can drop the final reference and free the context
before the structure pointer is set to NULL, and there is no RCU protection
or locking, can the worker thread dereference the freed eventfd context?

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

[PATCH v6 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 for this
scenario out to the closure of the entire MDEV, where it
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_fsm.c | 3 ---
 drivers/s390/cio/vfio_ccw_ops.c | 9 +++++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 4d7988ea47ef..248adf1e7707 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -169,9 +169,6 @@ 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);
 }
 
 /*
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index bd488e40e153..a717174675bd 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -210,6 +210,15 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
 	cancel_work_sync(&private->io_work);
 	cancel_work_sync(&private->crw_work);
 
+	/*
+	 * Ensure that a not-operational device has indeed
+	 * freed any resources. Normally the interrupt handler
+	 * or FSM close should do this, but there exists the
+	 * possibility that a device removed from the config
+	 * would have left some of this about.
+	 */
+	cp_free(&private->cp);
+
 	vfio_ccw_unregister_dev_regions(private);
 }
 
-- 
2.53.0

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

[sashiko-bot]

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

New issues:
- [High] Memory leak / API violation due to pinned pages remaining indefinitely when `dma_unmap` is called on a not-operational device.

Pre-existing issues:
- [High] Use-After-Free or Double Free of channel program resources due to concurrent execution of `fsm_close()` and asynchronous workqueues.
- [High] Data Race leading to Use-After-Free between `vfio_ccw_dma_unmap()` and `cp_free()`.
- [High] Data Race causing List Corruption in the `private->crw` list.
--

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

s390/vfio_ccw: move cp cleanup out of not operational

This patch defers the cleanup of the private channel program resources
from the fsm_notoper routine to the closure of the MDEV device. This aims to
prevent lock contention issues when the device asynchronously becomes not
operational.

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d7988ea47ef0..248adf1e77073 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -169,9 +169,6 @@ 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);

[Severity: High]
Does removing this cleanup leave pages pinned indefinitely if the device enters
the NOT_OPER state?

If a guest unmaps DMA memory after the device enters VFIO_CCW_STATE_NOT_OPER,
vfio_ccw_dma_unmap() calls vfio_ccw_mdev_reset().

However, in the NOT_OPER state, the FSM maps VFIO_CCW_EVENT_CLOSE to fsm_nop.
This causes the reset to return -EINVAL without freeing the pinned pages.

Could this leak the memory and violate the VFIO API contract?

>  }
>  
>  /*

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index bd488e40e1531..a717174675bd6 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -210,6 +210,15 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
>  	cancel_work_sync(&private->io_work);

[Severity: High]
This is a pre-existing issue, but can vfio_ccw_mdev_close_device() trigger a
use-after-free by dispatching the close event before canceling the I/O
workqueues?

In vfio_ccw_mdev_close_device(), vfio_ccw_fsm_event() emits the close event,
which invokes fsm_close().

fsm_close() unconditionally calls cp_free() before cancel_work_sync()
completes.

Concurrently, the asynchronous workqueue vfio_ccw_sch_io_todo() can execute
and also conditionally call cp_free() locklessly.

Does this cause a double-free or use-after-free of the channel program
resources?

>  	cancel_work_sync(&private->crw_work);

[Severity: High]
This isn't a bug introduced by this patch, but does the CRW list suffer from
data races that could corrupt the list?

Looking at the CRW region handling, vfio_ccw_queue_crw() adds items to
private->crw using list_add_tail().

Conversely, vfio_ccw_crw_region_read() removes items using list_del().

Since both operate locklessly without a spinlock or mutex, could concurrent
execution corrupt the kernel linked list pointers?

> +	/*
> +	 * Ensure that a not-operational device has indeed
> +	 * freed any resources. Normally the interrupt handler
> +	 * or FSM close should do this, but there exists the
> +	 * possibility that a device removed from the config
> +	 * would have left some of this about.
> +	 */
> +	cp_free(&private->cp);

[Severity: High]
This is a pre-existing issue, but does calling cp_free() concurrently with
DMA unmap operations lead to a use-after-free?

In cp_free(), the channel program list is traversed and its elements are freed
locklessly:

drivers/s390/cio/vfio_ccw_cp.c:cp_free() {
    ...
    list_for_each_entry_safe(chain, temp, &cp->ccwchain_list, next) {
        ...
        ccwchain_free(chain);
    }
}

Meanwhile, vfio_ccw_dma_unmap() invokes cp_iova_pinned(), which traverses the
exact same ccwchain_list without holding any locks.

If cp_free() unlinks and frees the chains while cp_iova_pinned() is
traversing them, could this result in a use-after-free?

> +
>  	vfio_ccw_unregister_dev_regions(private);
>  }

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

[PATCH v6 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     |  4 ++++
 drivers/s390/cio/vfio_ccw_fsm.c     | 20 ++++++++++++-------
 drivers/s390/cio/vfio_ccw_ops.c     | 12 +++++++++++-
 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 1a095085bc72..faf2de074a10 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);
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 248adf1e7707..beaed753d5da 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;
 }
@@ -249,6 +246,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;
 
@@ -297,6 +296,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,
@@ -317,6 +318,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);
@@ -407,7 +409,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 a717174675bd..ddfcd2b4296f 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);
@@ -90,6 +96,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;
 }
@@ -145,6 +152,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);
 }
 
@@ -217,7 +225,9 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
 	 * possibility that a device removed from the config
 	 * would have left some of this about.
 	 */
+	mutex_lock(&private->cp_mutex);
 	cp_free(&private->cp);
+	mutex_unlock(&private->cp_mutex);
 
 	vfio_ccw_unregister_dev_regions(private);
 }
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 0501d4bbcdbd..ac5aaa78a74b 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
@@ -115,7 +116,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 v6 09/10] s390/vfio_ccw: implement a channel program mutex

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A 3-thread AB-BC-CA deadlock is introduced involving `cp_mutex`, `iommu->lock`, and `iommu->device_list_lock`.
--

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

s390/vfio_ccw: implement a channel program mutex

This patch introduces a mutex to protect the channel program struct in the
vfio_ccw driver. The lock is held across operations that require private->cp
to ensure consistent behavior and replaces some incorrect subchannel lock uses.

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -249,6 +246,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;

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> --- 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);

[Severity: High]
Could this introduce a 3-way deadlock with the VFIO IOMMU subsystem?

When an I/O request is submitted, fsm_io_request() takes cp_mutex and
eventually calls cp_prefetch(), which attempts to acquire iommu->lock via
vfio_pin_pages().

At the same time, if a VFIO DMA unmap occurs, the VFIO core calls this
vfio_ccw_dma_unmap() callback while already holding device_list_lock.
This function then attempts to acquire cp_mutex.

Meanwhile, a concurrent VFIO DMA unmap could acquire iommu->lock in the
VFIO core (which drops it temporarily) and attempt to acquire
device_list_lock.

This seems to create a circular dependency:

Thread 1 (I/O submission):
fsm_io_request()
  mutex_lock(&private->cp_mutex);
  cp_prefetch()
    ...
      mutex_lock(&iommu->lock);

Thread 2 (VFIO DMA unmap):
vfio_notify_dma_unmap()
  mutex_lock(&iommu->device_list_lock);
  vfio_ccw_dma_unmap()
    mutex_lock(&private->cp_mutex);

Thread 3 (Concurrent VFIO DMA unmap):
vfio_notify_dma_unmap()
  mutex_lock(&iommu->lock);
  mutex_lock(&iommu->device_list_lock);

Is it possible for this sequence to freeze the CCW device operations and
the VFIO IOMMU subsystem?

> +	if (!cp_iova_pinned(&private->cp, iova, length)) {
> +		mutex_unlock(&private->cp_mutex);
>  		return;
> +	}
> +
> +	mutex_unlock(&private->cp_mutex);
>  
>  	vfio_ccw_mdev_reset(private);
>  }

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

[PATCH v6 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 faf2de074a10..d7e93b9d491c 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);
 }
 
 /*
@@ -279,6 +282,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
@@ -296,7 +300,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 ddfcd2b4296f..2f28846fdf72 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -60,6 +60,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);
+	spin_lock_init(&private->crw_lock);
 
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
@@ -137,11 +138,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);
 
 	/* Should be empty, but just in case */
 	cancel_work_sync(&private->io_work);
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index ac5aaa78a74b..c23430e1a78e 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
@@ -121,6 +123,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 v6 10/10] s390/vfio_ccw: implement a crw lock

[sashiko-bot]

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

Pre-existing issues:
- [High] A race condition between asynchronous channel path events (vfio_ccw_chp_event) and device removal (vfio_ccw_mdev_remove) can cause a Use-After-Free of the private structure.
- [High] Unbounded growth of the CRW queue leads to kernel memory leaks and potential OOM.
- [High] Unsynchronized replacement and freeing of eventfd triggers in vfio_ccw_mdev_set_irqs races with their usage, leading to a Use-After-Free.
- [High] Incorrect teardown sequence in vfio_ccw_mdev_release_dev frees list elements before cancelling asynchronous work, causing a memory leak or UAF.
- [Medium] A 0-byte read operation on the CRW region inappropriately consumes and drops a CRW event.
--

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

s390/vfio_ccw: implement a crw lock

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.

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index f3015132d4b5f..7708eb4d6de0b 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
[ ... ]
> @@ -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);
> +

[Severity: Medium]
This is a pre-existing issue, but does a 0-byte read operation on the CRW
region inappropriately consume and drop a CRW event?

If userspace performs a read operation on the CRW region with count == 0, the
driver proceeds to dequeue an event from private->crw here, performs a 0-byte
copy_to_user(), and then frees the CRW object. The event would be lost forever
without being delivered.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index faf2de074a10f..d7e93b9d491ca 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 is a pre-existing issue, but is there an unsynchronized replacement and
freeing of eventfd triggers in vfio_ccw_mdev_set_irqs() that races with their
usage here?

Userspace can call the VFIO_DEVICE_SET_IRQS ioctl to change or clear eventfds.
vfio_ccw_mdev_set_irqs() handles this and calls eventfd_ctx_put() without
holding any locks to serialize it against asynchronous readers like
vfio_ccw_crw_todo().

If a reader holds a pointer to private->crw_trigger while the ioctl drops the
reference, could the reader call eventfd_signal() on a freed object?

> @@ -296,7 +300,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);
>  }

[Severity: High]
This is a pre-existing issue, but could unbounded growth of the CRW queue
lead to kernel memory leaks and potential OOM?

vfio_ccw_queue_crw() unconditionally allocates new struct vfio_ccw_crw objects
using kzalloc_obj(GFP_ATOMIC) and appends them to private->crw. Without bounds
checking or a limit on the number of elements, a stream of events could cause
the queue to grow infinitely, consuming un-reclaimable kernel memory.

[Severity: High]
This is a pre-existing issue, but is there a race condition between
asynchronous channel path events and device removal?

In vfio_ccw_chp_event(), the private structure is fetched using
dev_get_drvdata() without acquiring a reference count:

vfio_ccw_chp_event()
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);

Concurrently, if a user unbinds the device, vfio_ccw_mdev_remove() executes,
clearing the drvdata and dropping the final reference via vfio_put_device(),
which frees the private structure.

Could vfio_ccw_chp_event() then call vfio_ccw_queue_crw(), dereferencing the
freed pointer and causing a Use-After-Free?

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index ddfcd2b4296f1..2f28846fdf724 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
[ ... ]
> @@ -137,11 +138,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);
>  
>  	/* Should be empty, but just in case */
>  	cancel_work_sync(&private->io_work);

[Severity: High]
This is a pre-existing issue, but does this teardown sequence free list
elements before cancelling asynchronous work?

By iterating through private->crw and freeing all elements before calling
cancel_work_sync(&private->crw_work), the lifecycle ordering violates the
principle of stopping asynchronous work before freeing the resources it
operates on.

If an event concurrently calls vfio_ccw_queue_crw() and inserts an element
after the list is emptied but before cancel_work_sync(), could the element
be leaked?

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