[PATCH v3 0/8] s390/vfio_ccw fixes

[PATCH v3 0/8] 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/
v2->v3:
 (Patches 1-4 are unchanged except for these first two items)
 - [FA] Added r-b to patch 1 (thank you!)
 - [sashiko] Fixed typo on error return from ccwchain_handle_ccw()
 - [NEW][sashiko] Fixed the idal size calculated by get_guest_idal()
 - [sashiko] Expanded clamps across region types
 - [sashiko] Replace cp_lock with cp_mutex, and rework intersections with
   the existing sch->lock and private->io_mutex

Eric Farman (8):
  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: implement a channel program mutex
  s390/vfio_ccw: implement a crw lock

 drivers/s390/cio/vfio_ccw_async.c   |  4 ++
 drivers/s390/cio/vfio_ccw_chp.c     | 19 +++++++---
 drivers/s390/cio/vfio_ccw_cp.c      | 59 ++++++++++++++++++++++++-----
 drivers/s390/cio/vfio_ccw_cp.h      |  7 ++++
 drivers/s390/cio/vfio_ccw_drv.c     | 14 ++++++-
 drivers/s390/cio/vfio_ccw_fsm.c     | 22 +++++++----
 drivers/s390/cio/vfio_ccw_ops.c     | 20 +++++++---
 drivers/s390/cio/vfio_ccw_private.h |  5 +++
 8 files changed, 122 insertions(+), 28 deletions(-)

-- 
2.53.0

[PATCH v3 1/8] 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>
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 v3 1/8] s390/vfio_ccw: free all memory if cp_init() fails

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Unbounded mutual recursion in channel program parsing leads to host kernel stack overflow.
- [High] Out-of-bounds read in `ccwchain_calc_length` due to an off-by-one error.
--

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

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

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. This patch performs the cleanup outside of
the recursive ccwchain_handle_ccw()/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: Critical]
This is a pre-existing issue, but does this mutual recursion between
ccwchain_handle_ccw() and ccwchain_loop_tic() have a depth limit?

If a guest VM constructs a channel program with a long sequence of
transfer in channel commands that each jump to a distinct guest physical
address, tic_target_chain_exists() will return false.

This causes ccwchain_loop_tic() to recursively call ccwchain_handle_ccw():

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_loop_tic() {
        ...
        if (tic_target_chain_exists(tic, cp))
                continue;

        ret = ccwchain_handle_ccw(tic->cda, cp);
        ...
}

Since there doesn't appear to be a limit on the recursion depth here,
could this exhaust the host kernel stack?

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

[ ... ]

[Severity: High]
This is also a pre-existing issue, but is there a potential out-of-bounds
read in ccwchain_calc_length() due to an off-by-one error?

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

Since cp->guest_cp is allocated with exactly CCWCHAIN_LEN_MAX elements,
if a guest provides a channel program where all 256 items have the
command-chaining flag enabled, the loop executes 256 times.

On the 257th iteration, cnt is 257 and ccw points to cp->guest_cp[256],
which is past the end of the allocation. The loop condition evaluates
to true (256 < 257), allowing ccw_is_chain(ccw) to dereference the
out-of-bounds pointer.

Could this read hit an unmapped guard page and crash the host?

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

[PATCH v3 2/8] s390/vfio_ccw: limit the number of channel program segments

[Eric Farman]

The processing of channel programs, and the CCWs within them, is done
recursively. As such, there is an arbitrary (but not architectural)
limit to the number of CCWs that can exist in a single channel program.

The vfio-ccw logic breaks these channel programs into segments whenever
it encounters a Transfer-In-Channel (TIC) CCW, and the combined number
of segments count towards the global limit. Impose an equivalent limit
to the number of segments until such logic can be made non-recursive.

Fixes: 0a19e61e6d4c ("vfio: ccw: introduce channel program interfaces")
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 6 ++++++
 drivers/s390/cio/vfio_ccw_cp.h | 6 ++++++
 2 files changed, 12 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..dc91a317ef19 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -23,6 +23,11 @@
  */
 #define CCWCHAIN_LEN_MAX	256
 
+/*
+ * Maximum number of chains
+ */
+#define CCWCHAIN_COUNT_MAX	16
+
 /**
  * struct channel_program - manage information for channel program
  * @ccwchain_list: list head of ccwchains
@@ -38,6 +43,7 @@ struct channel_program {
 	union orb orb;
 	bool initialized;
 	struct ccw1 *guest_cp;
+	int ccwchain_count;
 };
 
 int cp_init(struct channel_program *cp, union orb *orb);
-- 
2.53.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Off-by-one error in ccwchain_calc_length allows a guest-triggered out-of-bounds read.
--

--- Patch [2]: [PATCH v3 2/8] s390/vfio_ccw: limit the number of channel program segments ---
commit b0f514e864a35fcb11c0505d666fd9d7fbad12c0
Author: Eric Farman <farman@linux.ibm.com>
Subject: s390/vfio_ccw: limit the number of channel program segments

This commit limits the number of channel program segments that can exist
in a single channel program. It breaks channel programs into segments
when it encounters a Transfer-In-Channel (TIC) CCW, and adds a check to
ensure the segment count does not exceed a global limit. This prevents
hitting an arbitrary limit due to recursive processing.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 086d1b54bdb05..1c2890d139c6b 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
[ ... ]
> @@ -441,6 +442,10 @@ static int ccwchain_handle_ccw(dma32_t cda, struct channel_program *cp)
>  	if (len < 0)
>  		return len;
>  

[Severity: High]
This isn't a bug introduced by this patch, but does ccwchain_calc_length()
contain an off-by-one error that allows a guest-triggered out-of-bounds read?

When a guest VM submits a channel program containing a sequence of 256 CCWs
with the command-chaining or data-chaining flag set, the driver handles
this request and calls ccwchain_calc_length() to parse the chain.

The array cp->guest_cp is allocated to hold exactly CCWCHAIN_LEN_MAX (256)
elements. However, the loop in ccwchain_calc_length() uses the condition
while (cnt < CCWCHAIN_LEN_MAX + 1):

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_calc_length() {
	do {
		cnt++;
		...
		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
			break;

		ccw++;
	} while (cnt < CCWCHAIN_LEN_MAX + 1);
}

Does this allow the loop to iterate up to 257 times? During the 257th
iteration (cnt = 257), does the pointer ccw advance past the end of the
cp->guest_cp array, causing the ccw_is_chain(ccw) macro to dereference
an out-of-bounds pointer to read ccw->flags?

Could this lead to a kernel heap out-of-bounds read and subsequent kernel
panic if KASAN is enabled, allowing an unprivileged guest to impact host
stability?

> +	/* 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);
[ ... ]

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

[PATCH v3 3/8] 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
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 1c2890d139c6..258a99930c6a 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -393,11 +393,14 @@ static int ccwchain_calc_length(u64 iova, struct channel_program *cp)
 		if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
 			break;
 
-		ccw++;
-	} while (cnt < CCWCHAIN_LEN_MAX + 1);
+		/* Exit the loop when we reach the maximum */
+		if (cnt >= CCWCHAIN_LEN_MAX) {
+			cnt = -EINVAL;
+			break;
+		}
 
-	if (cnt == CCWCHAIN_LEN_MAX + 1)
-		cnt = -EINVAL;
+		ccw++;
+	} while (1);
 
 	return cnt;
 }
-- 
2.53.0

Re: [PATCH v3 3/8] 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
> 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/20260723174751.1180334-1-farman@linux.ibm.com?part=3

[PATCH v3 4/8] 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
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c | 15 +++++++++++++++
 drivers/s390/cio/vfio_ccw_cp.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 258a99930c6a..06b6bc6142a0 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -533,6 +533,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);
@@ -549,6 +550,17 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
 			kfree(idaws);
 			return ERR_PTR(ret);
 		}
+
+		if (cp->orb.cmd.c64)
+			first_idaw = dma64_to_u64(idaws[0]);
+		else
+			first_idaw = dma32_to_u32(idaws[0] >> 32);
+
+		/* 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) {
@@ -614,6 +626,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 dc91a317ef19..d547dbc969cc 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -43,6 +43,7 @@ struct channel_program {
 	union orb orb;
 	bool initialized;
 	struct ccw1 *guest_cp;
+	u64 guest_iova;
 	int ccwchain_count;
 };
 
-- 
2.53.0

Re: [PATCH v3 4/8] s390/vfio_ccw: ensure first IDAW remains constant

[sashiko-bot]

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

New issues:
- [Low] Applying a bitwise shift directly to a restricted `__bitwise` type (`dma64_t`) and implicitly casting it strips type annotations, triggering sparse warnings.

Pre-existing issues:
- [High] `get_guest_idal` reads too many bytes for Format-1 IDALs, potentially causing spurious failures for valid channel programs.
--

--- Patch [4]: [PATCH v3 4/8] s390/vfio_ccw: ensure first IDAW remains constant ---
commit e8a4291c688f39a3fdc915242b5bb6676e357ac5
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: ensure first IDAW remains constant

This commit ensures the first IDAW in a list is read separately to accurately
calculate the buffer size needed. It verifies that the address found in the
first IDAW is unchanged between reads to maintain consistency.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 258a99930c6a0..06b6bc6142a0f 100644
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -533,6 +533,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 get_guest_idal() read too many bytes
for Format-1 IDALs here?

Because idal_len is calculated using sizeof(*idaws) which evaluates to 8
bytes, instead of the actual 4 byte IDAW size for Format-1 IDALs,
vfio_dma_rw() will attempt to read idaw_nr * 8 bytes.

If a guest VM submits an I/O request with a Format-1 IDAL that fits exactly
at the end of a mapped page, this could cross the page boundary into an
unmapped page and cause the DMA read to fail, spuriously rejecting a valid
request.

[ ... ]

> @@ -549,6 +550,17 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
>  			kfree(idaws);
>  			return ERR_PTR(ret);
>  		}
> +
> +		if (cp->orb.cmd.c64)
> +			first_idaw = dma64_to_u64(idaws[0]);
> +		else
> +			first_idaw = dma32_to_u32(idaws[0] >> 32);

[Severity: Low]
Does applying a bitwise shift directly to a restricted __bitwise type like
dma64_t and passing it to dma32_to_u32() trigger sparse warnings?

Since idaws[0] is of type dma64_t (__bitwise u64) and dma32_to_u32() expects
a dma32_t (__bitwise u32), this implicitly strips type annotations.

Should this be explicitly cast using (__force u64) before shifting to avoid
violating sparse type rules?

> +
> +		/* 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) {

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

Re: [PATCH v3 4/8] s390/vfio_ccw: ensure first IDAW remains constant

[Eric Farman]

On 7/23/26 2:29 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Low] Applying a bitwise shift directly to a restricted `__bitwise` type (`dma64_t`) and implicitly casting it strips type annotations, triggering sparse warnings.
> 
> Pre-existing issues:
> - [High] `get_guest_idal` reads too many bytes for Format-1 IDALs, potentially causing spurious failures for valid channel programs.
> --
> 
> --- Patch [4]: [PATCH v3 4/8] s390/vfio_ccw: ensure first IDAW remains constant ---
> commit e8a4291c688f39a3fdc915242b5bb6676e357ac5
> Author: Eric Farman <farman@linux.ibm.com>
> 
> s390/vfio_ccw: ensure first IDAW remains constant
> 
> This commit ensures the first IDAW in a list is read separately to accurately
> calculate the buffer size needed. It verifies that the address found in the
> first IDAW is unchanged between reads to maintain consistency.
> 
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>> index 258a99930c6a0..06b6bc6142a0f 100644
>> --- a/drivers/s390/cio/vfio_ccw_cp.c
>> +++ b/drivers/s390/cio/vfio_ccw_cp.c
>> @@ -533,6 +533,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 get_guest_idal() read too many bytes
> for Format-1 IDALs here?
> 
> Because idal_len is calculated using sizeof(*idaws) which evaluates to 8
> bytes, instead of the actual 4 byte IDAW size for Format-1 IDALs,
> vfio_dma_rw() will attempt to read idaw_nr * 8 bytes.
> 
> If a guest VM submits an I/O request with a Format-1 IDAL that fits exactly
> at the end of a mapped page, this could cross the page boundary into an
> unmapped page and cause the DMA read to fail, spuriously rejecting a valid
> request.
> 
> [ ... ]
> 
>> @@ -549,6 +550,17 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, struct channel_program *cp, int
>>   			kfree(idaws);
>>   			return ERR_PTR(ret);
>>   		}
>> +
>> +		if (cp->orb.cmd.c64)
>> +			first_idaw = dma64_to_u64(idaws[0]);
>> +		else
>> +			first_idaw = dma32_to_u32(idaws[0] >> 32);
> 
> [Severity: Low]
> Does applying a bitwise shift directly to a restricted __bitwise type like
> dma64_t and passing it to dma32_to_u32() trigger sparse warnings?
> 
> Since idaws[0] is of type dma64_t (__bitwise u64) and dma32_to_u32() expects
> a dma32_t (__bitwise u32), this implicitly strips type annotations.
> 
> Should this be explicitly cast using (__force u64) before shifting to avoid
> violating sparse type rules?

Probably a better solution to this is:

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 06b6bc6142a0..08be6d5a99f3 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -551,10 +551,11 @@ static dma64_t *get_guest_idal(struct ccw1 *ccw, 
struct channel_program *cp, int
                         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[0] >> 32);
+                       first_idaw = dma32_to_u32(idaws_f1[0]);

                 /* Unexpected mismatch from earlier read */
                 if (first_idaw != cp->guest_iova) {


> 
>> +
>> +		/* 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) {
> 

[PATCH v3 5/8] 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
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 06b6bc6142a0..b4a041d35bba 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.
@@ -534,7 +535,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;
@@ -602,7 +603,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 v3 5/8] s390/vfio_ccw: calculate idal length based on idaw type

[sashiko-bot]

> 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
> 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/20260723174751.1180334-1-farman@linux.ibm.com?part=5

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

[Eric Farman]

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

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

diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
index 420d89ba7f83..f213f62f6641 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,6 +25,8 @@ static ssize_t vfio_ccw_async_region_read(struct vfio_ccw_private *private,
 		return -EINVAL;
 
 	mutex_lock(&private->io_mutex);
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 	if (copy_to_user(buf, (void *)region + pos, count))
 		ret = -EFAULT;
@@ -48,6 +51,7 @@ static ssize_t vfio_ccw_async_region_write(struct vfio_ccw_private *private,
 	if (!mutex_trylock(&private->io_mutex))
 		return -EAGAIN;
 
+	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..bf678270c457 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,8 @@ static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
 		return -EINVAL;
 
 	mutex_lock(&private->io_mutex);
+
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 
 	if (cio_update_schib(sch)) {
@@ -97,6 +100,7 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 		list_del(&crw->next);
 
 	mutex_lock(&private->io_mutex);
+	i = array_index_nospec(i, private->num_regions);
 	region = private->region[i].data;
 
 	if (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 v3 6/8] s390/vfio_ccw: ensure index for read/write regions are within range

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Concurrent manipulation of the `private->crw` linked list without locking.
--

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

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

This commit ensures that region indexes for existing read/write regions
are properly clamped to the range of the capabilities chain using
array_index_nospec().

> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 38c176cf62957..bf678270c4572 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
[ ... ]
> @@ -97,6 +100,7 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
>  		list_del(&crw->next);
>  
>  	mutex_lock(&private->io_mutex);
> +	i = array_index_nospec(i, private->num_regions);

[Severity: Critical]
This is a pre-existing issue, but is it possible for concurrent reads to
corrupt the private->crw linked list here?

Looking at vfio_ccw_crw_region_read(), it appears the crw list element is
retrieved and removed from the list before acquiring any locks:

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

Could two concurrent pread() calls (e.g., from an untrusted QEMU guest)
enter this function simultaneously, fetch the exact same crw element, and
both execute list_del() and kfree()?

Additionally, could asynchronous hardware events from the CSS driver add
elements to the list simultaneously?

For example, vfio_ccw_queue_crw() calls:
    list_add_tail(&crw->next, &private->crw);

Could this unsynchronized addition run simultaneously with the VFS read path
list removals and lead to slab corruption?

>  	region = private->region[i].data;
>  
>  	if (crw)

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

[PATCH v3 7/8] 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
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 10 +++++++++-
 drivers/s390/cio/vfio_ccw_fsm.c     | 22 +++++++++++++++-------
 drivers/s390/cio/vfio_ccw_ops.c     | 10 +++++++++-
 drivers/s390/cio/vfio_ccw_private.h |  3 +++
 4 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 1a095085bc72..1d8c2ed9da50 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);
@@ -259,12 +263,16 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 	rc = 0;
 
 	if (cio_update_schib(sch)) {
-		if (private)
+		if (private) {
+			spin_unlock_irqrestore(&sch->lock, flags);
 			vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
+			goto out;
+		}
 	}
 
 out_unlock:
 	spin_unlock_irqrestore(&sch->lock, flags);
+out:
 
 	return rc;
 }
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 4d7988ea47ef..9fbe97bd23f9 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;
 }
@@ -171,7 +168,9 @@ static void fsm_notoper(struct vfio_ccw_private *private,
 	private->state = VFIO_CCW_STATE_NOT_OPER;
 
 	/* This is usually handled during CLOSE event */
+	mutex_lock(&private->cp_mutex);
 	cp_free(&private->cp);
+	mutex_unlock(&private->cp_mutex);
 }
 
 /*
@@ -252,6 +251,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;
 
@@ -300,6 +301,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,
@@ -320,6 +323,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);
@@ -410,7 +414,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 032a1cdf4df7..04800cfa779b 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;
 }
@@ -141,6 +148,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
 	kmem_cache_free(vfio_ccw_io_region, private->io_region);
 	kfree(private->cp.guest_cp);
+	mutex_destroy(&private->cp_mutex);
 	mutex_destroy(&private->io_mutex);
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 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 v3 7/8] s390/vfio_ccw: implement a channel program mutex

[sashiko-bot]

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

New issues:
- [High] Dropping `sch->lock` in `vfio_ccw_sch_event` before dispatching the FSM event causes `css_sched_sch_todo` to be called locklessly, leading to lost unregister events.

Pre-existing issues:
- [High] Missing locking in `vfio_ccw_mdev_reset` allows concurrent `VFIO_DEVICE_RESET` ioctls to trigger physical subchannel unregistration.
--

--- Patch [7]: [PATCH v3 7/8] s390/vfio_ccw: implement a channel program mutex ---
commit 7b4b6e7c580cf97b697d04eaf47e890ff6d29bb0
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a channel program mutex
    
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.
    
Cc: stable@vger.kernel.org
Signed-off-by: Eric Farman <farman@linux.ibm.com>

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -259,12 +263,16 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>  	rc = 0;
>  
>  	if (cio_update_schib(sch)) {
> -		if (private)
> +		if (private) {
> +			spin_unlock_irqrestore(&sch->lock, flags);
>  			vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> +			goto out;
> +		}
>  	}

[Severity: High]
Since sch->lock is dropped here, does this allow css_sched_sch_todo() to be
called locklessly?

If vfio_ccw_fsm_event() routes to fsm_notoper(), we have:

vfio_ccw_sch_event()
  spin_unlock_irqrestore(&sch->lock, flags);
  vfio_ccw_fsm_event() -> fsm_notoper()
    css_sched_sch_todo(sch, SCH_TODO_UNREG);

Could this lead to a race on sch->todo, allowing other events to overwrite
SCH_TODO_UNREG and leaving the kernel state out of sync with hardware?

> 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);
> +	if (!cp_iova_pinned(&private->cp, iova, length)) {
> +		mutex_unlock(&private->cp_mutex);
>  		return;
> +	}
> +
> +	mutex_unlock(&private->cp_mutex);
>  
>  	vfio_ccw_mdev_reset(private);
>  }

[Severity: High]
This is a pre-existing issue, but does vfio_ccw_mdev_reset() lack locking
protection against concurrent invocations?

If two threads concurrently trigger vfio_ccw_mdev_reset() (e.g. via the
VFIO_DEVICE_RESET ioctl), they could interleave:

Thread A issues VFIO_CCW_EVENT_CLOSE, transitioning to STANDBY.
Before Thread A issues OPEN, Thread B issues VFIO_CCW_EVENT_CLOSE.

The FSM receives CLOSE while in STANDBY state, which routes to fsm_notoper()
and schedules SCH_TODO_UNREG. Could this cause the physical subchannel to be
permanently unregistered, acting as a host denial of service?

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

[PATCH v3 8/8] 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>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_chp.c     | 17 +++++++++++------
 drivers/s390/cio/vfio_ccw_drv.c     |  4 ++++
 drivers/s390/cio/vfio_ccw_ops.c     |  3 +++
 drivers/s390/cio/vfio_ccw_private.h |  2 ++
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index bf678270c457..3d05aaf790fa 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -93,15 +93,19 @@ 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);
+	i = array_index_nospec(i, private->num_regions);
+	region = private->region[i].data;
+
+	spin_lock(&private->crw_lock);
 	crw = list_first_entry_or_null(&private->crw,
 				       struct vfio_ccw_crw, next);
 
 	if (crw)
 		list_del(&crw->next);
 
-	mutex_lock(&private->io_mutex);
-	i = array_index_nospec(i, private->num_regions);
-	region = private->region[i].data;
+	/* Drop CRW lock while copying to userspace */
+	spin_unlock(&private->crw_lock);
 
 	if (crw)
 		memcpy(&region->crw, &crw->crw, sizeof(region->crw));
@@ -112,14 +116,15 @@ static ssize_t vfio_ccw_crw_region_read(struct vfio_ccw_private *private,
 		ret = count;
 
 	region->crw = 0;
-
-	mutex_unlock(&private->io_mutex);
-
 	kfree(crw);
 
 	/* Notify the guest if more CRWs are on our queue */
+	spin_lock(&private->crw_lock);
 	if (!list_empty(&private->crw) && private->crw_trigger)
 		eventfd_signal(private->crw_trigger);
+	spin_unlock(&private->crw_lock);
+
+	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 1d8c2ed9da50..2c0a27851342 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -125,8 +125,10 @@ void vfio_ccw_crw_todo(struct work_struct *work)
 
 	private = container_of(work, struct vfio_ccw_private, crw_work);
 
+	spin_lock(&private->crw_lock);
 	if (!list_empty(&private->crw) && private->crw_trigger)
 		eventfd_signal(private->crw_trigger);
+	spin_unlock(&private->crw_lock);
 }
 
 /*
@@ -300,7 +302,9 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
 	crw->crw.erc = erc;
 	crw->crw.rsid = rsid;
 
+	spin_lock(&private->crw_lock);
 	list_add_tail(&crw->next, &private->crw);
+	spin_unlock(&private->crw_lock);
 	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 04800cfa779b..277138f14b25 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)
@@ -138,10 +139,12 @@ 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;
 
+	spin_lock(&private->crw_lock);
 	list_for_each_entry_safe(crw, temp, &private->crw, next) {
 		list_del(&crw->next);
 		kfree(crw);
 	}
+	spin_unlock(&private->crw_lock);
 
 	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 ac5aaa78a74b..2562679b5ef5 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -121,6 +121,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 v3 8/8] s390/vfio_ccw: implement a crw lock

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing cancellation of work items (`crw_work` and `io_work`) during device release leads to a Use-After-Free vulnerability.
- [High] Concurrent updates to eventfd contexts (`crw_trigger`, `io_trigger`) in `vfio_ccw_mdev_set_irqs()` lack synchronization, leading to a Use-After-Free of the `eventfd_ctx`.
--

--- Patch [8]: [PATCH v3 8/8] s390/vfio_ccw: implement a crw lock ---
commit 9d63fadad88983453bdf16e8b5f5b2ddd01df2c8
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.

Fixes: 3f02cb2fd9d2 ("vfio-ccw: Wire up the CRW irq and CRW region")
Cc: stable@vger.kernel.org
Cc: Farhan Ali <alifm@linux.ibm.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 1d8c2ed9da509..2c0a27851342c 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -125,8 +125,10 @@ void vfio_ccw_crw_todo(struct work_struct *work)
>  
>  	private = container_of(work, struct vfio_ccw_private, crw_work);
>  
> +	spin_lock(&private->crw_lock);
>  	if (!list_empty(&private->crw) && private->crw_trigger)
>  		eventfd_signal(private->crw_trigger);
> +	spin_unlock(&private->crw_lock);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for this worker
thread to race with the VFIO_DEVICE_SET_IRQS ioctl handler?

Looking at vfio_ccw_mdev_set_irqs(), it updates the eventfd pointers and
releases old contexts via eventfd_ctx_put() without holding crw_lock.

Could a worker thread read a non-null crw_trigger here, but before
eventfd_signal() executes, the ioctl frees the eventfd context, resulting in a
use-after-free of the eventfd_ctx?

>  }
>  

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 04800cfa779b6..277138f14b251 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c

[ ... ]

> @@ -138,10 +139,12 @@ 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;
>  
> +	spin_lock(&private->crw_lock);
>  	list_for_each_entry_safe(crw, temp, &private->crw, next) {
>  		list_del(&crw->next);
>  		kfree(crw);
>  	}
> +	spin_unlock(&private->crw_lock);

[Severity: High]
This is a pre-existing issue, but does this function need to call
cancel_work_sync() for private->crw_work and private->io_work before the
private structure is torn down and freed?

If a work item was asynchronously queued (e.g., via vfio_ccw_chp_event()),
could it execute after the device is released and dereference the freed
memory?

The flush_workqueue() in vfio_ccw_sch_quiesce() doesn't appear to be
guaranteed to run on all paths, and might not prevent new works from being
queued before the structure is freed.

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

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