[PATCH v9 00/10] s390/vfio_ccw fixes

[PATCH v9 00/10] s390/vfio_ccw fixes

[Eric Farman]

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

Patches 1-6 are identical to v6/v7/v8
Patches 7-10 have minor changes compared to v7/v8

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/
v6: https://lore.kernel.org/r/20260726172153.1818056-1-farman@linux.ibm.com/
v7: https://lore.kernel.org/r/20260727192230.2715207-1-farman@linux.ibm.com/
v8: https://lore.kernel.org/r/20260728013509.1551753-1-farman@linux.ibm.com/
v8->v9:
 - [MR] Added r-b (thank you!)
 - [MR] Commit/comment message improvements
 - [MR/sashiko] Re-add workqueue cleanup to vfio_ccw_mdev_release_dev, in the
   form of a flush_work() call

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: selectively expand io_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      | 74 +++++++++++++++++++++--------
 drivers/s390/cio/vfio_ccw_cp.h      | 10 ++++
 drivers/s390/cio/vfio_ccw_drv.c     | 21 +++++++-
 drivers/s390/cio/vfio_ccw_fsm.c     |  8 +++-
 drivers/s390/cio/vfio_ccw_ops.c     | 39 +++++++++++++--
 drivers/s390/cio/vfio_ccw_private.h | 10 +++-
 8 files changed, 176 insertions(+), 33 deletions(-)

-- 
2.53.0

[PATCH v9 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

[PATCH v9 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

[PATCH v9 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

[PATCH v9 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

[PATCH v9 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

[PATCH v9 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

[PATCH v9 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
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 032a1cdf4df7..d361d1fde3a0 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -131,6 +131,13 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 		container_of(vdev, struct vfio_ccw_private, vdev);
 	struct vfio_ccw_crw *crw, *temp;
 
+	/*
+	 * Ensure these work items are fully drained, so none can
+	 * fire after being released.
+	 */
+	cancel_work_sync(&private->io_work);
+	cancel_work_sync(&private->crw_work);
+
 	list_for_each_entry_safe(crw, temp, &private->crw, next) {
 		list_del(&crw->next);
 		kfree(crw);
@@ -202,6 +209,14 @@ 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);
+
+	/*
+	 * Ensure these work items are drained, in the event the
+	 * device is re-opened instead of released.
+	 */
+	cancel_work_sync(&private->io_work);
+	cancel_work_sync(&private->crw_work);
+
 	vfio_ccw_unregister_dev_regions(private);
 }
 
-- 
2.53.0

[PATCH v9 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
a future patch can safely manage the locking requirements.

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     | 13 +++++++++++++
 drivers/s390/cio/vfio_ccw_private.h |  3 +++
 4 files changed, 26 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 d361d1fde3a0..1df6d649565b 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)
@@ -134,9 +135,16 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 	/*
 	 * Ensure these work items are fully drained, so none can
 	 * fire after being released.
+	 *
+	 * notoper_work should have nothing to do here, because only
+	 * open devices could have channel_program resources in use
+	 * and those would be released during close. Nevertheless,
+	 * call flush here as well to be certain anything that was
+	 * allocated is freed.
 	 */
 	cancel_work_sync(&private->io_work);
 	cancel_work_sync(&private->crw_work);
+	flush_work(&private->notoper_work);
 
 	list_for_each_entry_safe(crw, temp, &private->crw, next) {
 		list_del(&crw->next);
@@ -213,9 +221,14 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
 	/*
 	 * Ensure these work items are drained, in the event the
 	 * device is re-opened instead of released.
+	 *
+	 * notoper_work needs to be given a chance to run if it
+	 * is queued, so any memory associated with the channel
+	 * program can be returned.
 	 */
 	cancel_work_sync(&private->io_work);
 	cancel_work_sync(&private->crw_work);
+	flush_work(&private->notoper_work);
 
 	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..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

[PATCH v9 09/10] s390/vfio_ccw: selectively expand io_mutex

[Eric Farman]

The io_mutex was defined to serialize the io_regions, but then has
also sort of been associated with the I/O themselves because of
the close relationship they share.

With the handful of races that are possible, the choices are either to:
 A) expand the scope of io_mutex to close these remaining windows, or
 B) reduce the scope of io_mutex to just io_region, and introduce a new
    lock mechanism for the remaining I/O resources

This patch implements A, since B brings with it a lot more interactions
that would need to be tracked and kept in a correct hierarchy. It also
takes advantage of the workqueue element for cp_free() that now gets
called out of fsm_notoper(), which could be invoked out of an interrupt
context and thus cannot acquire a mutex itself.

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

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index f3015132d4b5..9269b54f5cfd 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -98,13 +98,13 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 	if (pos + count > sizeof(*region))
 		return -EINVAL;
 
+	mutex_lock(&private->io_mutex);
 	crw = list_first_entry_or_null(&private->crw,
 				       struct vfio_ccw_crw, next);
 
 	if (crw)
 		list_del(&crw->next);
 
-	mutex_lock(&private->io_mutex);
 	if (i >= private->num_regions) {
 		ret = -EINVAL;
 		goto out;
diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 5ef082b8289a..58722c4baa25 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -977,17 +977,23 @@ 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;
 
 	if (!cp->initialized)
 		return false;
 
+	mutex_lock(&private->io_mutex);
 	list_for_each_entry(chain, &cp->ccwchain_list, next) {
 		for (i = 0; i < chain->ch_len; i++)
-			if (page_array_iova_pinned(&chain->ch_pa[i], iova, length))
+			if (page_array_iova_pinned(&chain->ch_pa[i], iova, length)) {
+				mutex_unlock(&private->io_mutex);
 				return true;
+			}
 	}
+	mutex_unlock(&private->io_mutex);
 
 	return false;
 }
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index c197ad5ab580..757ff5b2556e 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -91,6 +91,7 @@ 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->io_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,9 +99,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 			cp_is_finished = true;
 		}
 	}
-	mutex_lock(&private->io_mutex);
 	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
-	mutex_unlock(&private->io_mutex);
 
 	/*
 	 * Reset to IDLE only if processing of a channel program
@@ -110,6 +109,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 	 */
 	if (cp_is_finished)
 		private->state = VFIO_CCW_STATE_IDLE;
+	mutex_unlock(&private->io_mutex);
 
 	if (private->io_trigger)
 		eventfd_signal(private->io_trigger);
@@ -131,7 +131,9 @@ void vfio_ccw_notoper_todo(struct work_struct *work)
 
 	private = container_of(work, struct vfio_ccw_private, notoper_work);
 
+	mutex_lock(&private->io_mutex);
 	cp_free(&private->cp);
+	mutex_unlock(&private->io_mutex);
 }
 
 /*
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 4d47a3c7b9a0..5fd94e9d5c61 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -170,6 +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 routine could be called from IRQ context, so defer */
 	queue_work(vfio_ccw_work_q, &private->notoper_work);
 }
 
@@ -409,7 +410,11 @@ static void fsm_close(struct vfio_ccw_private *private,
 
 	private->state = VFIO_CCW_STATE_STANDBY;
 	spin_unlock_irq(&sch->lock);
+
+	mutex_lock(&private->io_mutex);
 	cp_free(&private->cp);
+	mutex_unlock(&private->io_mutex);
+
 	return;
 
 err_unlock:
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index e2256402b089..739121116ab6 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -88,7 +88,8 @@ struct vfio_ccw_parent {
  * @state: internal state of the device
  * @completion: synchronization helper of the I/O completion
  * @io_region: MMIO region to input/output I/O arguments/results
- * @io_mutex: protect against concurrent update of I/O regions
+ * @io_mutex: protect against concurrent update of I/O resources
+ *            and @cp lifecycle
  * @region: additional regions for other subchannel operations
  * @cmd_region: MMIO region for asynchronous I/O commands other than START
  * @schib_region: MMIO region for SCHIB information
-- 
2.53.0

[PATCH v9 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
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Reviewed-by: Farhan Ali<alifm@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 9269b54f5cfd..7708eb4d6de0 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -93,18 +93,13 @@ 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;
 
 	mutex_lock(&private->io_mutex);
-	crw = list_first_entry_or_null(&private->crw,
-				       struct vfio_ccw_crw, next);
-
-	if (crw)
-		list_del(&crw->next);
-
 	if (i >= private->num_regions) {
 		ret = -EINVAL;
 		goto out;
@@ -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 757ff5b2556e..ab6b518cc353 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -118,11 +118,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)
@@ -286,6 +289,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
@@ -303,7 +307,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 1df6d649565b..5ce91285c7d5 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -55,6 +55,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)
@@ -131,6 +132,7 @@ 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;
 
 	/*
 	 * Ensure these work items are fully drained, so none can
@@ -146,10 +148,12 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 	cancel_work_sync(&private->crw_work);
 	flush_work(&private->notoper_work);
 
+	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);
 
 	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 739121116ab6..3bd0171d38d0 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
@@ -120,6 +122,8 @@ struct vfio_ccw_private {
 	struct channel_program	cp;
 	struct irb		irb;
 	union scsw		scsw;
+
+	spinlock_t		crw_lock;
 	struct list_head	crw;
 
 	struct eventfd_ctx	*io_trigger;
-- 
2.53.0

Re: [PATCH v9 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/20260728033022.2658232-1-farman@linux.ibm.com?part=2

Re: [PATCH v9 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] A preexisting logic flaw allows a guest to cause the host's Channel Subsystem to execute out-of-bounds memory as a Channel Command Word (CCW), potentially leading to arbitrary host memory corruption via DMA.
--

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

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

This commit modifies the loop in ccwchain_calc_length() to fix an
out-of-bounds array access that could examine the 257th CCW before failing.
It changes a do..while loop to a for loop to strictly bound the CCW array
examination.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 1c2890d139c6b..af632f9d54535 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;

[Severity: Critical]
This is a pre-existing issue, but does this calculation adequately account for
the hardware's behavior of skipping a CCW when a Status Modifier is presented?

If a guest submits a channel program where a CCW that can generate a Status
Modifier (like a Search CCW) has the command-chaining (CC) flag enabled and is
followed by a TIC CCW that lacks the CC flag and jumps to a different chain,
the loop will stop counting at the TIC because both !ccw_is_chain(ccw) and
!is_tic_within_range(...) are true.

In this scenario, ccwchain_calc_length() returns a length of exactly 2. The
host would then allocate 16 bytes for chain->ch_ccw from the DMA heap. When the
hardware executes the first CCW and encounters a Status Modifier, it skips the
TIC and fetches the next CCW from 16 bytes into the allocation, which is
out-of-bounds of the host's DMA heap allocation.

By grooming the host heap, could a guest place controlled data adjacent to this
allocation, which the hardware would then execute as an untranslated CCW,
potentially leading to arbitrary host memory corruption via DMA?

> +	}
>  
> -	return cnt;
> +	return -EINVAL;
>  }

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

Re: [PATCH v9 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] When processing Format-1 IDALs (32-bit IDAWs), get_guest_idal() computes idal_len using the size of a 64-bit host IDAW structure (idaw_nr * 8) rather than the actual 32-bit size of the guest IDAWs (idaw_nr * 4), causing an over-read of guest memory.
--

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

s390/vfio_ccw: ensure first IDAW remains constant

The first IDAW in a list is read separately to calculate the needed
buffer size. This patch verifies that the address found in the first
IDAW remains unchanged between reads to ensure a consistent set of IDAWs.

Cc: stable@vger.kernel.org

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

[Severity: High]
This is a pre-existing issue, but does this calculate an oversized length
for 32-bit Format-1 guest IDAWs?

When processing Format-1 IDALs, idaws is a dma64_t pointer (8 bytes).
Using sizeof(*idaws) means idal_len evaluates to idaw_nr * 8. Later in
get_guest_idal(), vfio_dma_rw() uses this oversized length:

	if (ccw_is_idal(ccw)) {
		/* Copy IDAL from guest */
		ret = vfio_dma_rw(vdev, dma32_to_u32(ccw->cda), idaws,
				  idal_len, false);

Could this cause valid I/O operations to fail with an error if the 32-bit
IDAL is located near the end of a mapped guest page, resulting in an
over-read into an unmapped DMA region?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Unprotected state modification of `private->state` in the workqueue handler causing a data race.
- [Medium] Untranslated guest IOVAs are passed to the hardware for skipped data transfers.
--

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

s390/vfio_ccw: calculate idal length based on idaw type

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.

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.

[ ... ]

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

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential address translation
issue later in this file. In ccwchain_fetch_ccw(), when a guest submits a
channel program with a CCW that skips data transfer (e.g., READ with
CCW_FLAG_SKIP):

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

Does this bypass translation and pass untranslated guest IOVAs to the
hardware? If ccw_does_data_transfer() returns 0, the idaws buffer
might be filled with raw guest physical addresses, which the hardware will
interpret as host physical addresses and throw an addressing exception.


[Severity: Medium]
This is also a pre-existing issue, but is it safe to modify private->state
without holding the io_mutex or sch->lock?

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo() {
	...
	if (cp_is_finished)
		private->state = VFIO_CCW_STATE_IDLE;
	...
}

If vfio_ccw_sch_io_todo() executes concurrently with an asynchronous event
like vfio_ccw_sch_event(), could this locklessly overwrite the state to
VFIO_CCW_STATE_IDLE after the event handler has already set it to
VFIO_CCW_STATE_NOT_OPER under the sch->lock?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The `vfio_ccw_notoper_todo` work function calls `cp_free()` without acquiring `private->io_mutex`, leading to a severe data race with userspace I/O request processing.
- [High] Asynchronous subchannel events can queue `notoper_work` after `flush_work()` has completed during device release, leading to a use-after-free of the `private` structure.
- [High] Concurrent execution of `cp_free()` from `fsm_close()` and the new `notoper_work` leads to a double free of channel program resources.
--

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

s390/vfio_ccw: move cp cleanup out of not operational

This commit moves the cleanup of the channel program out of the
fsm_notoper() routine and defers it to a workqueue. This avoids
performing the cleanup while holding the cio subchannel lock.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> --- 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: Critical]
This isn't a bug introduced by this patch, but does moving the cleanup
to a workqueue create a data race with userspace I/O requests?

If vfio_ccw_notoper_todo() runs concurrently with a guest operating system
issuing an I/O request via vfio_ccw_mdev_write_io_region() (which acquires
private->io_mutex), might cp_free() execute locklessly?

If notoper_todo executes during fsm_io_request() processing cp_prefetch(),
it could tear down the channel program and free ccwchain pointers while
they are actively being iterated and dereferenced.

The commit message notes a future patch manages locking, but should this
be protected by io_mutex here to avoid memory leaks and use-after-free
conditions?

[Severity: High]
This is also a pre-existing issue, but could this lockless execution lead
to a double free if the device is closed concurrently?

When the device is closed, fsm_close() drops sch->lock and calls cp_free().
If a notoper event occurs simultaneously after the lock is dropped,
vfio_ccw_sch_event() queues notoper_work.

Since cp_free() is lockless and cp->initialized is not atomically updated,
could both the fsm_close() thread and the new workqueue thread read
cp->initialized as true and double-free the channel program allocations?

> 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

[ ... ]

> @@ -134,9 +135,16 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  	/*
>  	 * Ensure these work items are fully drained, so none can
>  	 * fire after being released.
> +	 *
> +	 * notoper_work should have nothing to do here, because only
> +	 * open devices could have channel_program resources in use
> +	 * and those would be released during close. Nevertheless,
> +	 * call flush here as well to be certain anything that was
> +	 * allocated is freed.
>  	 */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);
> +	flush_work(&private->notoper_work);

[Severity: High]
This is a pre-existing issue, but does flushing the work here guarantee
that new work cannot be queued afterwards?

If a user removes the mdev via sysfs concurrently with a subchannel event,
vfio_ccw_sch_event() fetches the private pointer from drvdata outside
the subchannel lock:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_event() {
	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
	...
	if (cio_update_schib(sch)) {
		if (private)
			vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
	}
}

If the event thread is preempted, vfio_ccw_mdev_remove() can complete
unregistering the device and invoke vfio_ccw_mdev_release_dev(). Once
release finishes, flush_work() returns and the private structure is freed.

When the preempted vfio_ccw_sch_event() thread resumes, won't it use the
freed private pointer to call fsm_notoper() and execute queue_work() on
the freed private->notoper_work memory?

Calling queue_work() after the workqueue shutdown sequence completes
might corrupt the workqueue linked list.

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

Re: [PATCH v9 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:
- [Critical] Race condition causing linked list corruption on `private->crw`.
- [Critical] Missing background work cancellation leading to Use-After-Free.
--

--- Patch [6]: [PATCH v9 06/10] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit a96682323321d1f6bd6354e74ae1c8c0bbb835a1
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.

> 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: Critical]
This is a pre-existing issue, but is it safe to dequeue from private->crw
locklessly here?

In drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_queue_crw(), the hardware event
path appends to this list without locks:

    list_add_tail(&crw->next, &private->crw);

Could a concurrent userspace read on the CRW region race with a hardware
event and corrupt the linked list pointers?

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


[Severity: Critical]
This is another pre-existing issue, but does vfio_ccw_mdev_release_dev()
need to cancel the background work queues during teardown?

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_release_dev() {
    struct vfio_ccw_private *private =
        container_of(vdev, struct vfio_ccw_private, vdev);
    ...
}

During device release, if fsm_close() disables the subchannel immediately
and skips vfio_ccw_sch_quiesce(), could private->crw_work and
private->io_work still be queued?

Since vfio_ccw_mdev_release_dev() lacks cancel_work_sync() calls, would any
pending work items execute after the private struct has been freed by the
VFIO core, leading to a use-after-free?

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Placing `cancel_work_sync(&private->io_work)` after the FSM CLOSE event in `vfio_ccw_mdev_close_device` fails to prevent `io_work` from executing concurrently with `cp_free`, leading to a use-after-free on `ccwchain` objects.
- [High] Missing synchronization against mdev removal in asynchronous CSS callbacks (`vfio_ccw_chp_event`, `vfio_ccw_sch_irq`) allows them to queue work on the `private` structure after it has been cancelled and freed.
- [High] The `private->crw` linked list is modified and accessed concurrently without any locking mechanism, leading to list corruption.
- [High] Lockless updates to eventfd contexts (`io_trigger`, `crw_trigger`) can lead to Use-After-Free.
--

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

This commit adds cleanup tags in _release_dev and _close_device to cancel
the io_work and crw_work workqueues. This ensures work items are not
dispatched after the private struct is freed or when the device is closed.

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 032a1cdf4df70..d361d1fde3a0d 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -131,6 +131,13 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  	struct vfio_ccw_crw *crw, *temp;
>  
> +	/*
> +	 * Ensure these work items are fully drained, so none can
> +	 * fire after being released.
> +	 */
> +	cancel_work_sync(&private->io_work);
> +	cancel_work_sync(&private->crw_work);
> +

[Severity: High]
This is a pre-existing issue, but missing synchronization against mdev removal
in asynchronous CSS callbacks allows them to queue work on the private
structure after it has been freed.

vfio_ccw_mdev_remove() clears the device pointer without holding sch->lock:

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

Concurrently, an incoming asynchronous channel path event retrieves the
private structure without synchronization:

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

If the interrupt handler thread is preempted, the removal process can
complete, freeing the private struct. When the handler resumes, it blindly
accesses private, resulting in a use-after-free.

Is there a way to serialize mdev removal with the hardware callbacks?

>  	list_for_each_entry_safe(crw, temp, &private->crw, next) {

[Severity: High]
This is a pre-existing issue, but the private->crw linked list is modified
and accessed concurrently without any locking mechanism, which can lead to
list corruption.

vfio_ccw_queue_crw() appends elements to the list from interrupt context:

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

Concurrently, vfio_ccw_crw_region_read() retrieves and deletes elements
from the list in process context without acquiring any locks:

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

Could this concurrent access cause list corruption or double frees when an
unprivileged user reads the CRW region while hardware events occur?

>  		list_del(&crw->next);
>  		kfree(crw);
>  	}

[ ... ]

> @@ -202,6 +209,14 @@ 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);
> +
> +	/*
> +	 * Ensure these work items are drained, in the event the
> +	 * device is re-opened instead of released.
> +	 */
> +	cancel_work_sync(&private->io_work);

[Severity: High]
This isn't a bug introduced by this patch, but placing the cancellation
after the FSM CLOSE event leaves a race window open.

When a device is closed, vfio_ccw_mdev_close_device() triggers
vfio_ccw_fsm_event() which synchronously executes fsm_close(). This disables
the subchannel, drops the lock, and calls cp_free():

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

If a hardware interrupt queued io_work right before the subchannel was
disabled, the work item vfio_ccw_sch_io_todo() remains queued and will
execute concurrently. It calls cp_update_scsw(), which accesses the freed
cp structures:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_io_todo() {
    ...
    if (scsw_is_solicited(&irb->scsw)) {
        cp_update_scsw(&private->cp, &irb->scsw);
    ...
}

Could the cancellation be moved before the FSM event to ensure work items
are drained before the channel program is freed?

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

[Severity: High]
This is a pre-existing issue, but lockless updates to eventfd contexts can
lead to a use-after-free.

The io_trigger pointer is read locklessly in workqueue functions to signal
events:

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

Concurrently, the VFIO_DEVICE_SET_IRQS ioctl can free the context before
setting the pointer to NULL:

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

If the workqueue thread reads the pointer just before the ioctl drops the
reference, it will call eventfd_signal() on a freed context. Should these
eventfd updates be protected by a lock or RCU?

>  	vfio_ccw_unregister_dev_regions(private);
>  }

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

Re: [PATCH v9 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:
- [Critical] Unbounded mutual recursion in `ccwchain_handle_ccw` and `ccwchain_loop_tic` allows a malicious guest to trigger a host kernel stack overflow or kernel memory exhaustion via a long linear chain of TICs.
--

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

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

The routine cp_free() cannot be used to free memory if cp_init() fails
before initialization completes. This patch moves cleanup outside the
recursive loop to properly free all segments created by a Transfer in
Channel (TIC) 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: Critical]
This is a pre-existing issue, but does this mutually recursive call chain
between ccwchain_handle_ccw() and ccwchain_loop_tic() allow a malicious
guest to trigger a host kernel stack overflow?

If a guest constructs a channel program with a long linear sequence of
Transfer In Channel (TIC) commands, each pointing to a distinct guest
physical address, the host processes it by recursively entering:

ccwchain_handle_ccw()
  ccwchain_loop_tic()
    ccwchain_handle_ccw()

Could this unbounded recursion exhaust the host kernel stack and cause a
denial of service?

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

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

Re: [PATCH v9 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:
- [Critical] Concurrent lockless modification of eventfd triggers in `vfio_ccw_mdev_set_irqs` causes a Use-After-Free of the `eventfd_ctx`.
- [High] Concurrent execution of asynchronous CSS events and mdev removal causes a Use-After-Free of the `private` structure.
--

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

s390/vfio_ccw: implement a crw lock

This commit introduces a spinlock to serialize access to the CRW region,
which relies exclusively on asynchronous events from hardware. It ensures
cohesive reading of those payloads and manages the list of those payloads.

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 757ff5b2556e3..ab6b518cc353f 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -118,11 +118,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);

[Severity: Critical]
This is a pre-existing issue, but can this concurrent lockless access to
private->crw_trigger lead to a use-after-free of the eventfd_ctx?

The ioctl writer in vfio_ccw_mdev_set_irqs() modifies and releases the eventfd
pointers locklessly:

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

Concurrently, this worker thread reads the trigger pointer here without any
synchronization against the ioctl writer (no RCU, no matching locks).

Could the ioctl free the context immediately after the worker validates the
pointer but before it signals it? (This same pattern also appears in
vfio_ccw_sch_io_todo() with private->io_trigger).

> +	spin_unlock_irqrestore(&private->crw_lock, flags);
>  }

[ ... ]

> @@ -286,6 +289,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
> @@ -303,7 +307,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 isn't a bug introduced by this patch, but can the private pointer be freed
before this lock is acquired, leading to a use-after-free?

Concurrent mdev removal clears the driver data and drops the device reference
without synchronizing against the CSS subsystem:

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

Concurrently, an asynchronous CSS event triggers vfio_ccw_chp_event(), which
fetches the private pointer locklessly just before it is cleared:

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

This path uses the potentially freed pointer to lock memory here and push a
freed work_struct onto the kernel workqueue.

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

Re: [PATCH v9 09/10] s390/vfio_ccw: selectively expand io_mutex

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete locking of `private->crw` list mutations, leading to list corruption.
- [High] Race between `vfio_ccw_sch_event()` and device removal leads to Use-After-Free of `private`.
--

--- Patch [9]: [PATCH v9 09/10] s390/vfio_ccw: selectively expand io_mutex ---
commit d6adad0fd2f3370cc987cc74322451109b224ff2
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: selectively expand io_mutex
    
The io_mutex was originally defined to serialize io_regions, but has become
associated with I/O resources due to their close relationship. This patch
expands the scope of io_mutex to close remaining race windows. It also takes
advantage of workqueue element deferral for cp_free().

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index f3015132d4b5f..9269b54f5cfd4 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -98,13 +98,13 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  	if (pos + count > sizeof(*region))
>  		return -EINVAL;
>  
> +	mutex_lock(&private->io_mutex);
>  	crw = list_first_entry_or_null(&private->crw,
>  				       struct vfio_ccw_crw, next);
>  
>  	if (crw)
>  		list_del(&crw->next);
>  
> -	mutex_lock(&private->io_mutex);

[Severity: High]
This isn't a bug introduced by this patch, but does expanding the io_mutex
to cover this list manipulation still leave a data race?

While the reader side now holds io_mutex, the writer side in
vfio_ccw_queue_crw() adds items to the list without acquiring this lock:

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

Since vfio_ccw_queue_crw() executes from asynchronous CSS driver callbacks,
can it run concurrently with the reader and lead to linked list corruption?

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index 4d47a3c7b9a06..5fd94e9d5c618 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -170,6 +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 routine could be called from IRQ context, so defer */
>  	queue_work(vfio_ccw_work_q, &private->notoper_work);
>  }

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free and
subsequent workqueue list corruption if the device is being concurrently
removed?

In vfio_ccw_sch_event(), the private pointer is fetched locklessly before
acquiring sch->lock:

drivers/s390/cio/vfio_ccw_drv.c:vfio_ccw_sch_event() {
    ...
    struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
    ...
    spin_lock_irqsave(&sch->lock, flags);
    ...
    if (private)
        vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
    ...
}

If the device is unbound via sysfs concurrently, vfio_ccw_sch_remove() can
clear the drvdata and free the private pointer before vfio_ccw_sch_event()
acquires the lock.

When fsm_notoper() is then called, it will queue the embedded notoper_work
struct on the freed private pointer. Could this corrupt the kernel workqueue
lists?

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

Re: [PATCH v9 00/10] s390/vfio_ccw fixes

[Christian Borntraeger]

Am 28.07.26 um 05:30 schrieb Eric Farman:
> This series addresses some pre-existing issues found in the
> s390 vfio_ccw (DASD passthrough) driver.
> 
> Patches 1-6 are identical to v6/v7/v8
> Patches 7-10 have minor changes compared to v7/v8
> 
> 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/
> v6: https://lore.kernel.org/r/20260726172153.1818056-1-farman@linux.ibm.com/
> v7: https://lore.kernel.org/r/20260727192230.2715207-1-farman@linux.ibm.com/
> v8: https://lore.kernel.org/r/20260728013509.1551753-1-farman@linux.ibm.com/
> v8->v9:
>   - [MR] Added r-b (thank you!)
>   - [MR] Commit/comment message improvements
>   - [MR/sashiko] Re-add workqueue cleanup to vfio_ccw_mdev_release_dev, in the
>     form of a flush_work() call
> 
> 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: selectively expand io_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      | 74 +++++++++++++++++++++--------
>   drivers/s390/cio/vfio_ccw_cp.h      | 10 ++++
>   drivers/s390/cio/vfio_ccw_drv.c     | 21 +++++++-
>   drivers/s390/cio/vfio_ccw_fsm.c     |  8 +++-
>   drivers/s390/cio/vfio_ccw_ops.c     | 39 +++++++++++++--
>   drivers/s390/cio/vfio_ccw_private.h | 10 +++-
>   8 files changed, 176 insertions(+), 33 deletions(-

All applied. Thank you. 

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

[Matthew Rosato]

On 7/27/26 11:30 PM, Eric Farman wrote:
> 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
> a future patch can safely manage the locking requirements.
> 
> 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_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index d361d1fde3a0..1df6d649565b 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)
> @@ -134,9 +135,16 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
>  	/*
>  	 * Ensure these work items are fully drained, so none can
>  	 * fire after being released.
> +	 *
> +	 * notoper_work should have nothing to do here, because only
> +	 * open devices could have channel_program resources in use
> +	 * and those would be released during close. Nevertheless,
> +	 * call flush here as well to be certain anything that was
> +	 * allocated is freed.
>  	 */
>  	cancel_work_sync(&private->io_work);
>  	cancel_work_sync(&private->crw_work);
> +	flush_work(&private->notoper_work);

Sashiko still seems grumpy about this, but it largely seems fine to me.

We know that notoper_work will have either 0 or 1 task queued to it
(never more than 1) and that for the life a device a task would only be
queued to it exactly once (on entering NOT_OPER the first time, which it
a terminal state e.g. the FSM does not allow transferring to any other
state).  It is impossible to "re-enter" NOT_OPER and queue a second
notoper_work.  Therefore once not_oper has been flushed once (either
here or in close_dev()), nothing will add another.

The only scneario I'm trying to think about is whether it is still
theoretically possible to go into the NOT_OPER state after this
flush_work().  Which wouldn't be solved by a cancel_work_sync here
either btw.

This window would be very small and highly unlikely + this otherwise
closes the more obvious issues so with that:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Re: [PATCH v9 09/10] s390/vfio_ccw: selectively expand io_mutex

[Matthew Rosato]

On 7/27/26 11:30 PM, Eric Farman wrote:
> The io_mutex was defined to serialize the io_regions, but then has
> also sort of been associated with the I/O themselves because of
> the close relationship they share.
> 
> With the handful of races that are possible, the choices are either to:
>  A) expand the scope of io_mutex to close these remaining windows, or
>  B) reduce the scope of io_mutex to just io_region, and introduce a new
>     lock mechanism for the remaining I/O resources
> 
> This patch implements A, since B brings with it a lot more interactions
> that would need to be tracked and kept in a correct hierarchy. It also
> takes advantage of the workqueue element for cp_free() that now gets
> called out of fsm_notoper(), which could be invoked out of an interrupt
> context and thus cannot acquire a mutex itself.
> 
> Fixes: 4f76617378ee ("vfio-ccw: protect the I/O region")
> Cc: stable@vger.kernel.org
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Re: [PATCH v9 00/10] s390/vfio_ccw fixes

[Matthew Rosato]

On 7/27/26 11:30 PM, Eric Farman wrote:
> This series addresses some pre-existing issues found in the
> s390 vfio_ccw (DASD passthrough) driver.
> 
> Patches 1-6 are identical to v6/v7/v8
> Patches 7-10 have minor changes compared to v7/v8
> 
> 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/
> v6: https://lore.kernel.org/r/20260726172153.1818056-1-farman@linux.ibm.com/
> v7: https://lore.kernel.org/r/20260727192230.2715207-1-farman@linux.ibm.com/
> v8: https://lore.kernel.org/r/20260728013509.1551753-1-farman@linux.ibm.com/
> v8->v9:
>  - [MR] Added r-b (thank you!)
>  - [MR] Commit/comment message improvements
>  - [MR/sashiko] Re-add workqueue cleanup to vfio_ccw_mdev_release_dev, in the
>    form of a flush_work() call

Thanks Eric.

I think at this point the big issues are resolved, and this already is a
significant improvement overall.

We've also identified a few things (here and off-list) that would be
reasonable tackled as follow-ons, such as a well-defined lock hierarchy
and identifying reasonable places to add lockdep assertions.

Thanks,
Matt

> 
> 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: selectively expand io_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      | 74 +++++++++++++++++++++--------
>  drivers/s390/cio/vfio_ccw_cp.h      | 10 ++++
>  drivers/s390/cio/vfio_ccw_drv.c     | 21 +++++++-
>  drivers/s390/cio/vfio_ccw_fsm.c     |  8 +++-
>  drivers/s390/cio/vfio_ccw_ops.c     | 39 +++++++++++++--
>  drivers/s390/cio/vfio_ccw_private.h | 10 +++-
>  8 files changed, 176 insertions(+), 33 deletions(-)
>