[PATCH v2 0/7] s390/vfio_ccw fixes

[PATCH v2 0/7] 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/
v1->v2:
 - [sashiko] Reorder patches to put the cp_init and channel program segment
   pieces at the head of the series
 - [sashiko] Fix the out of bounds array check to break the loop correctly
 - [sashiko] Move the nospec clamp to the default case, rather than the
   entirety of the switch statement (to keep it in the default leg of the
   switch where it's used, rather than outside the entire switch)
 - [EF] Return -EINVAL if ccwchain count is exhausted, instead of -ENOMEM
 - [EF] Drop calc_max_idal_len() logic in place of a comparison against
   idaw[0] before/after the pair of vfio_dma_rw() calls 
 - [EF] Implement two new spin locks, one for struct channel_program in
   struct vfio_ccw_private, and one for the list of CRWs in struct
   vfio_ccw_private.

Eric Farman (7):
  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: ensure index for read/write regions are within range
  s390/vfio_ccw: implement a channel program lock
  s390/vfio_ccw: implement a crw lock

 drivers/s390/cio/vfio_ccw_chp.c     | 15 ++++---
 drivers/s390/cio/vfio_ccw_cp.c      | 67 +++++++++++++++++++++++++----
 drivers/s390/cio/vfio_ccw_cp.h      |  8 ++++
 drivers/s390/cio/vfio_ccw_drv.c     | 10 ++++-
 drivers/s390/cio/vfio_ccw_ops.c     | 13 +++++-
 drivers/s390/cio/vfio_ccw_private.h |  4 ++
 6 files changed, 101 insertions(+), 16 deletions(-)

-- 
2.53.0

[PATCH v2 1/7] s390/vfio_ccw: free all memory if cp_init() fails

[Eric Farman]

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

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

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

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

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index 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 v2 1/7] s390/vfio_ccw: free all memory if cp_init() fails

[sashiko-bot]

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

Pre-existing issues:
- [High] An untrusted guest can cause kernel stack exhaustion or memory exhaustion (OOM) by providing a channel program with a deeply nested, linear chain of Transfer in Channel (TIC) CCWs.
- [High] An off-by-one loop condition in `ccwchain_calc_length()` causes an out-of-bounds heap read.
--

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

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

This commit updates cp_init() to address a memory leak when initialization
fails. It performs the cleanup outside of the recursive handling logic to
avoid leaking segments created by Transfer in Channel (TIC) commands.

> 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);
>  
> -	if (ret)
> -		ccwchain_free(chain);
> -

[Severity: High]
This is a pre-existing issue, but can this recursive call to
ccwchain_handle_ccw() cause kernel stack exhaustion?

Looking at ccwchain_loop_tic() in drivers/s390/cio/vfio_ccw_cp.c, it
recursively calls ccwchain_handle_ccw() for each forward-jumping TIC:

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

Since this happens without any depth tracking or overall chain count limits,
could an untrusted guest cause a panic or OOM by providing a channel program
with a deeply nested, linear chain of Transfer in Channel (TIC) CCWs?

[Severity: High]
This is a pre-existing issue, but does this off-by-one loop condition
in ccwchain_calc_length() cause an out-of-bounds heap read?

drivers/s390/cio/vfio_ccw_cp.c:ccwchain_calc_length() {
        ...
        } while (cnt < CCWCHAIN_LEN_MAX + 1);
        ...
}

If a guest VM supplies exactly 256 CCWs that all have the command-chaining
flag enabled, will the loop evaluate !ccw_is_chain(ccw) on the 257th
iteration?

This appears to dereference ccw one element past the allocated array on
cp->guest_cp, which could trigger memory debug splats or crashes.

>  	return ret;
>  }

[ ... ]

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

Re: [PATCH v2 1/7] s390/vfio_ccw: free all memory if cp_init() fails

[Farhan Ali]

On 7/20/2026 1:19 PM, Eric Farman wrote:
> The routine cp_free() is called to unpin/free any memory once an I/O
> is completed successfully, or if cp_prefetch() fails. But if cp_init()
> fails, and cp->initialized is not enabled, the same routine cannot be
> used to free all the memory.
>
> An attempt to address this exists in ccwchain_handle_ccw(), where a
> single call to ccwchain_free() is made for the currently-processed
> CCW segment. But this will leak other segments (created as a result
> of a Transfer in Channel) that had been allocated as part of the same
> channel program.
>
> Address this by performing the cleanup outside of the recursive
> ccwchain_handle_ccw()/ccwchain_loop_tic() logic.
>
> Fixes: 8b515be512a2 ("vfio-ccw: Fix memory leak and don't call cp_free in cp_init")
> Cc: stable@vger.kernel.org
> Cc: Farhan Ali <alifm@linux.ibm.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_cp.c | 22 ++++++++++++++++++----
>   1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 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;

The fix looks correct to me and I don't think Sashiko found any issues 
with this patch (issues identified as pre-existing and i think something 
this series is trying to fix).

Reviewed-by: Farhan Ali<alifm@linux.ibm.com>

Thanks

Farhan

Re: [PATCH v2 1/7] s390/vfio_ccw: free all memory if cp_init() fails

[Eric Farman]

On 7/20/26 5:48 PM, Farhan Ali wrote:
> 
> On 7/20/2026 1:19 PM, Eric Farman wrote:
>> The routine cp_free() is called to unpin/free any memory once an I/O
>> is completed successfully, or if cp_prefetch() fails. But if cp_init()
>> fails, and cp->initialized is not enabled, the same routine cannot be
>> used to free all the memory.
>>
>> An attempt to address this exists in ccwchain_handle_ccw(), where a
>> single call to ccwchain_free() is made for the currently-processed
>> CCW segment. But this will leak other segments (created as a result
>> of a Transfer in Channel) that had been allocated as part of the same
>> channel program.
>>
>> Address this by performing the cleanup outside of the recursive
>> ccwchain_handle_ccw()/ccwchain_loop_tic() logic.
>>
>> Fixes: 8b515be512a2 ("vfio-ccw: Fix memory leak and don't call cp_free 
>> in cp_init")
>> Cc: stable@vger.kernel.org
>> Cc: Farhan Ali <alifm@linux.ibm.com>
>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>> ---
>>   drivers/s390/cio/vfio_ccw_cp.c | 22 ++++++++++++++++++----
>>   1 file changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/ 
>> vfio_ccw_cp.c
>> index 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;
> 
> The fix looks correct to me and I don't think Sashiko found any issues 
> with this patch (issues identified as pre-existing and i think something 
> this series is trying to fix).

Correct, the two pre-existing findings reported against this patch 
should be addressed by patches 2 and 3, respectively, in this series.

> 
> Reviewed-by: Farhan Ali<alifm@linux.ibm.com>
> 

Thank you!

> Thanks
> 
> Farhan
> 
> 
> 

[PATCH v2 2/7] 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..7abdebd8f7c3 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 ERR_PTR(-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 v2 2/7] s390/vfio_ccw: limit the number of channel program segments

[sashiko-bot]

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

New issues:
- [Low] Returning a pointer (`ERR_PTR`) from a function that returns an `int`.

Pre-existing issues:
- [High] Concurrent unprotected access to channel program structures via ioctl reset and write paths.
- [High] Out-of-bounds read on `cp->guest_cp` when calculating the CCW chain length.
--

--- Patch [2]: [PATCH v2 2/7] s390/vfio_ccw: limit the number of channel program segments ---
commit 55497ee3c2db8e265add0b6679cc5ddd751ba7ce
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: limit the number of channel program segments

This commit imposes a limit on the number of channel program segments to
prevent excessive recursive processing. It tracks the count in the
channel_program structure and fails if it exceeds CCWCHAIN_COUNT_MAX.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 086d1b54bdb05..7abdebd8f7c3d 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;
>  
> +	/* Limit number of chains in a single channel program */
> +	if (cp->ccwchain_count >= CCWCHAIN_COUNT_MAX)
> +		return ERR_PTR(-EINVAL);

[Severity: Low]
Does this return a pointer from a function that returns an int?
ccwchain_handle_ccw() returns an int, but ERR_PTR(-EINVAL) returns a void
pointer, which could cause a type mismatch or compilation warning.

[ ... ]

[Severity: High]
This is a pre-existing issue, but can concurrent unprotected access to
channel program structures occur via the ioctl reset and write paths?

If a user-space process concurrently invokes ioctl(VFIO_DEVICE_RESET) and
write() on the VFIO device file descriptor:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_reset() {
    ...
    vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
    ...
}

This calls into the state machine to close and free the channel program
without acquiring private->io_mutex:

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

Concurrently, vfio_ccw_mdev_write_io_region() acquires io_mutex and processes
IO, allocating and using private->cp. Could this data race lead to a
use-after-free?

[Severity: High]
This is a pre-existing issue, but is there an out-of-bounds read on
cp->guest_cp when calculating the CCW chain length?

If an unprivileged user provides a channel program filled with exactly 256
command-chained CCWs:

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

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

The loop executes a 257th iteration when cnt == 257, evaluating
!ccw_is_chain(ccw) on guest_cp[256], which points past the 256-element
guest_cp array.

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

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

[Eric Farman]

On 7/20/26 4:30 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [Low] Returning a pointer (`ERR_PTR`) from a function that returns an `int`.
> 
> Pre-existing issues:
> - [High] Concurrent unprotected access to channel program structures via ioctl reset and write paths.
> - [High] Out-of-bounds read on `cp->guest_cp` when calculating the CCW chain length.
> --
> 
> --- Patch [2]: [PATCH v2 2/7] s390/vfio_ccw: limit the number of channel program segments ---
> commit 55497ee3c2db8e265add0b6679cc5ddd751ba7ce
> Author: Eric Farman <farman@linux.ibm.com>
> 
> s390/vfio_ccw: limit the number of channel program segments
> 
> This commit imposes a limit on the number of channel program segments to
> prevent excessive recursive processing. It tracks the count in the
> channel_program structure and fails if it exceeds CCWCHAIN_COUNT_MAX.
> 
>> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
>> index 086d1b54bdb05..7abdebd8f7c3d 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;
>>   
>> +	/* Limit number of chains in a single channel program */
>> +	if (cp->ccwchain_count >= CCWCHAIN_COUNT_MAX)
>> +		return ERR_PTR(-EINVAL);
> 
> [Severity: Low]
> Does this return a pointer from a function that returns an int?

Bah, I'd pulled this hunk out of another branch where I changed what the 
function returned, and neglected to update this return. This should 
simply be "return -EINVAL" and not the ERR_PTR() wrapper.

> ccwchain_handle_ccw() returns an int, but ERR_PTR(-EINVAL) returns a void
> pointer, which could cause a type mismatch or compilation warning.
> 
> [ ... ]
> 
> [Severity: High]
> This is a pre-existing issue, but can concurrent unprotected access to
> channel program structures occur via the ioctl reset and write paths?
> 
> If a user-space process concurrently invokes ioctl(VFIO_DEVICE_RESET) and
> write() on the VFIO device file descriptor:
> 
> drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_reset() {
>      ...
>      vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
>      ...
> }
> 
> This calls into the state machine to close and free the channel program
> without acquiring private->io_mutex:
> 
> drivers/s390/cio/vfio_ccw_fsm.c:fsm_close() {
>      ...
>      cp_free(&private->cp);
>      ...
> }
> 
> Concurrently, vfio_ccw_mdev_write_io_region() acquires io_mutex and processes
> IO, allocating and using private->cp. Could this data race lead to a
> use-after-free?
> 
> [Severity: High]
> This is a pre-existing issue, but is there an out-of-bounds read on
> cp->guest_cp when calculating the CCW chain length?
> 
> If an unprivileged user provides a channel program filled with exactly 256
> command-chained CCWs:
> 
> drivers/s390/cio/vfio_ccw_cp.c:ccwchain_calc_length() {
>      ...
>      if (!ccw_is_chain(ccw) && !is_tic_within_range(ccw, iova, cnt))
>          break;
> 
>      ccw++;
> } while (cnt < CCWCHAIN_LEN_MAX + 1);
> 
> The loop executes a 257th iteration when cnt == 257, evaluating
> !ccw_is_chain(ccw) on guest_cp[256], which points past the 256-element
> guest_cp array.
> 

[PATCH v2 3/7] 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 7abdebd8f7c3..ce61858c82eb 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 v2 3/7] 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:
- [High] The driver does not explicitly cancel asynchronous background works (io_work and crw_work) during device removal or release, which can lead to a Use-After-Free if a work executes after the private device structure is freed.
--

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

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

This commit fixes an out-of-bounds check in ccwchain_calc_length() when
counting CCWs. It restructures the loop to break as soon as the maximum of
256 CCWs are examined, preventing memory reads outside the array range.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index 7abdebd8f7c3d..ce61858c82eb6 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;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
during device teardown in drivers/s390/cio/vfio_ccw_ops.c.

Looking at vfio_ccw_mdev_remove() and vfio_ccw_mdev_release_dev(), the
driver tears down the device and the VFIO core frees the private
structure without explicitly cancelling asynchronous background works
(io_work and crw_work).

While flush_workqueue() is called in vfio_ccw_sch_quiesce(), this only
flushes currently queued work and does not prevent concurrent queuing.

For instance, if vfio_ccw_chp_event() reads the private structure and
queues private->crw_work right before or during teardown, the work item
can remain on vfio_ccw_work_q and execute after private has been freed.

Does this teardown path need a cancel_work_sync() for these work items
to prevent them from accessing freed memory?

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

[PATCH v2 4/7] 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 ce61858c82eb..c6f0264fb5d3 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 v2 4/7] 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:

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but concurrent execution of a device reset or DMA unmap with an active I/O request leads to a critical use-after-free and data corruption of the channel program structures.
- [High] This problem wasn't introduced by this patch, but the IDAL read length calculation in `get_guest_idal()` for Format-1 IDAWs incorrectly calculates the length as if they were 64-bit IDAWs, leading to an out-of-bounds read from guest memory.
--

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

s390/vfio_ccw: ensure first IDAW remains constant

This patch ensures the first IDAW in a list remains constant by verifying
its address between reads, and returning an error if a mismatch is found.
This helps maintain a consistent set of IDAWs for I/O operations.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> index ce61858c82eb6..c6f0264fb5d33 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 the IDAL read length calculation in
get_guest_idal() for Format-1 IDAWs incorrectly calculates the length as if
they were 64-bit IDAWs, leading to an out-of-bounds read from guest memory.

Because Format-1 IDAWs are actually 32 bits (4 bytes) long, this instructs
vfio_dma_rw() to read exactly twice the valid IDAL size.

If the IDAL resides near the boundary of the guest's mapped IOMMU DMA region,
this over-read spans into an unmapped area, causing vfio_dma_rw() to reject
the read and fail the legitimate channel program.

Does this code need to calculate the length based on the actual IDAW size
instead of the host's 8-byte target struct dma64_t?

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

[ ... ]

Additionally, there is another regression present in the state machine:

[Severity: Critical]
This is a pre-existing issue, but concurrent execution of a device reset or
DMA unmap with an active I/O request leads to a use-after-free and data
corruption of the channel program structures.

If an unprivileged guest triggers an I/O operation, invoking
vfio_ccw_mdev_write_io_region() in the host, this function takes
private->io_mutex and pushes VFIO_CCW_EVENT_IO_REQ to the state machine,
invoking fsm_io_request().

Here, private->state is set to VFIO_CCW_STATE_CP_PROCESSING, and
cp_init()/cp_prefetch() are called without holding any locks:

drivers/s390/cio/vfio_ccw_fsm.c:fsm_io_request()
    private->state = VFIO_CCW_STATE_CP_PROCESSING;
    //...
    io_region->ret_code = cp_init(&private->cp, orb);
    //...
    io_region->ret_code = cp_prefetch(&private->cp);

Concurrently, if the guest alters its IOMMU mappings, it triggers
vfio_ccw_dma_unmap() on the host. This callback checks cp_iova_pinned() without
holding any locks and then executes vfio_ccw_mdev_reset(), which pushes
VFIO_CCW_EVENT_CLOSE to the state machine without acquiring private->io_mutex:

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_reset()
    vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);

Since the state is currently CP_PROCESSING, this routes to fsm_close(), which
unconditionally frees the structures concurrently with the active initialization
routine:

drivers/s390/cio/vfio_ccw_fsm.c:fsm_close()
    cp_free(&private->cp);

Can this sequence actually occur and cause cp_free() to free the ccwchain
memory while cp_prefetch() is actively modifying it? Should these events be
serialized to prevent this?

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

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

[Eric Farman]

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

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

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 45ec722d25ea..e695cd985369 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);
 	}
-- 
2.53.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Incomplete Spectre v1 Mitigation / Speculative Out-of-Bounds Access
- [High] Broken Spectre v1 Mitigation / Speculative Out-of-Bounds Read in ioctl handler
--

--- Patch [5]: [PATCH v2 5/7] s390/vfio_ccw: ensure index for read/write regions are within range ---
commit 9523937d39e77d5858c1e23dd917c8844bcf3876
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_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 45ec722d25ea6..e695cd985369d 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -243,6 +243,7 @@ static ssize_t vfio_ccw_mdev_read(struct vfio_device *vdev,
>  		return vfio_ccw_mdev_read_io_region(private, buf, count, ppos);
>  	default:
>  		index -= VFIO_CCW_NUM_REGIONS;
> +		index = array_index_nospec(index, private->num_regions);
>  		return private->region[index].ops->read(private, buf, count,
>  							ppos);

[Severity: High]
This isn't a bug introduced by this patch, but does this actually fix the
speculative out-of-bounds access?

We clamp the local index here, but we pass the unmodified ppos pointer
down to the read handlers. The handlers then recompute the index from ppos
without array_index_nospec():

drivers/s390/cio/vfio_ccw_async.c:vfio_ccw_async_region_read() {
    unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
    ...
    region = private->region[i].data;
    if (copy_to_user(buf, (void *)region + pos, count))
}

Can a malicious guest exploit this by passing an out-of-bounds offset,
causing the read handler to speculatively recompute the out-of-bounds index
i and perform a speculative out-of-bounds read via private->region[i].data?

The same pattern exists in vfio_ccw_async_region_write(),
vfio_ccw_schib_region_read(), and vfio_ccw_crw_region_read().

>  	}

[ ... ]

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

[Severity: High]
This isn't a bug introduced by this patch, but looking further down in
vfio_ccw_ops.c, is there a broken speculative mitigation in
vfio_ccw_mdev_ioctl_get_region_info()?

drivers/s390/cio/vfio_ccw_ops.c:vfio_ccw_mdev_ioctl_get_region_info() {
    ...
    info->index = array_index_nospec(info->index,
                                     VFIO_CCW_NUM_REGIONS +
                                     private->num_regions);
    i = info->index - VFIO_CCW_NUM_REGIONS;
    info->offset = VFIO_CCW_INDEX_TO_OFFSET(info->index);
    info->size = private->region[i].size;
    ...
}

If info->index is maliciously out of bounds, array_index_nospec() will
speculatively force it to 0. Since VFIO_CCW_NUM_REGIONS is 1, i becomes -1.

Does this result in a deterministic speculative out-of-bounds read at
index -1 when evaluating private->region[i].size?

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

[PATCH v2 6/7] s390/vfio_ccw: implement a channel program lock

[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 spin lock, which can be
acquired with or without the mutex guarding the I/O regions that
feed into the population of the channel program.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_cp.c      | 13 ++++++++++++-
 drivers/s390/cio/vfio_ccw_cp.h      |  1 +
 drivers/s390/cio/vfio_ccw_drv.c     |  6 +++++-
 drivers/s390/cio/vfio_ccw_ops.c     |  8 +++++++-
 drivers/s390/cio/vfio_ccw_private.h |  2 ++
 5 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
index c6f0264fb5d3..7bfb3a952817 100644
--- a/drivers/s390/cio/vfio_ccw_cp.c
+++ b/drivers/s390/cio/vfio_ccw_cp.c
@@ -790,7 +790,7 @@ int cp_init(struct channel_program *cp, union orb *orb)
  * @cp, which must have been returned by a previous call to cp_init().
  * Otherwise, undefined behavior occurs.
  */
-void cp_free(struct channel_program *cp)
+void cp_free_locked(struct channel_program *cp)
 {
 	struct vfio_device *vdev =
 		&container_of(cp, struct vfio_ccw_private, cp)->vdev;
@@ -810,6 +810,17 @@ void cp_free(struct channel_program *cp)
 	}
 }
 
+void cp_free(struct channel_program *cp)
+{
+	struct vfio_ccw_private *private =
+		container_of(cp, struct vfio_ccw_private, cp);
+	unsigned long flags;
+
+	spin_lock_irqsave(&private->cp_lock, flags);
+	cp_free_locked(cp);
+	spin_unlock_irqrestore(&private->cp_lock, flags);
+}
+
 /**
  * cp_prefetch() - translate a guest physical address channel program to
  *                 a real-device runnable channel program.
diff --git a/drivers/s390/cio/vfio_ccw_cp.h b/drivers/s390/cio/vfio_ccw_cp.h
index d547dbc969cc..edfb47a292ca 100644
--- a/drivers/s390/cio/vfio_ccw_cp.h
+++ b/drivers/s390/cio/vfio_ccw_cp.h
@@ -48,6 +48,7 @@ struct channel_program {
 };
 
 int cp_init(struct channel_program *cp, union orb *orb);
+void cp_free_locked(struct channel_program *cp);
 void cp_free(struct channel_program *cp);
 int cp_prefetch(struct channel_program *cp);
 union orb *cp_get_orb(struct channel_program *cp, struct subchannel *sch);
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 1a095085bc72..bfb68de6e52c 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -83,6 +83,7 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 {
 	struct vfio_ccw_private *private;
 	struct irb *irb;
+	unsigned long flags;
 	bool is_final;
 	bool cp_is_finished = false;
 
@@ -91,13 +92,16 @@ void vfio_ccw_sch_io_todo(struct work_struct *work)
 
 	is_final = !(scsw_actl(&irb->scsw) &
 		     (SCSW_ACTL_DEVACT | SCSW_ACTL_SCHACT));
+	spin_lock_irqsave(&private->cp_lock, flags);
 	if (scsw_is_solicited(&irb->scsw)) {
 		cp_update_scsw(&private->cp, &irb->scsw);
 		if (is_final && private->state == VFIO_CCW_STATE_CP_PENDING) {
-			cp_free(&private->cp);
+			cp_free_locked(&private->cp);
 			cp_is_finished = true;
 		}
 	}
+	spin_unlock_irqrestore(&private->cp_lock, flags);
+
 	mutex_lock(&private->io_mutex);
 	memcpy(private->io_region->irb_area, irb, sizeof(*irb));
 	mutex_unlock(&private->io_mutex);
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index e695cd985369..d2475b3158cf 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -36,10 +36,15 @@ static void vfio_ccw_dma_unmap(struct vfio_device *vdev, u64 iova, u64 length)
 {
 	struct vfio_ccw_private *private =
 		container_of(vdev, struct vfio_ccw_private, vdev);
+	unsigned long flags;
 
 	/* Drivers MUST unpin pages in response to an invalidation. */
-	if (!cp_iova_pinned(&private->cp, iova, length))
+	spin_lock_irqsave(&private->cp_lock, flags);
+	if (!cp_iova_pinned(&private->cp, iova, length)) {
+		spin_unlock_irqrestore(&private->cp_lock, flags);
 		return;
+	}
+	spin_unlock_irqrestore(&private->cp_lock, flags);
 
 	vfio_ccw_mdev_reset(private);
 }
@@ -54,6 +59,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->cp_lock);
 
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 0501d4bbcdbd..061b594fde36 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -115,7 +115,9 @@ struct vfio_ccw_private {
 	struct ccw_crw_region	*crw_region;
 	int num_regions;
 
+	spinlock_t		cp_lock;
 	struct channel_program	cp;
+
 	struct irb		irb;
 	union scsw		scsw;
 	struct list_head	crw;
-- 
2.53.0

Re: [PATCH v2 6/7] s390/vfio_ccw: implement a channel program lock

[sashiko-bot]

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

New issues:
- [High] The newly introduced `cp_lock` spinlock is held while executing operations that acquire sleeping locks, leading to a guaranteed 'scheduling while atomic' kernel BUG.

Pre-existing issues:
- [High] Incomplete locking allows a data race between channel program initialization and `vfio_ccw_dma_unmap()`, leading to a NULL pointer dereference.
- [High] The background work items `crw_work` and `io_work` are not explicitly cancelled before the driver frees the device's private memory.
--

--- Patch [6]: [PATCH v2 6/7] s390/vfio_ccw: implement a channel program lock ---
commit fcac9cc035321189b03cc15ccc7af0d3409a1018
Author: Eric Farman <farman@linux.ibm.com>

s390/vfio_ccw: implement a channel program lock

The channel_program struct is manipulated without a serialization
mechanism to ensure consistent behavior. This patch introduces a spin
lock to guard the entire structure, which can be acquired with or
without the mutex guarding the I/O regions.

> diff --git a/drivers/s390/cio/vfio_ccw_cp.c b/drivers/s390/cio/vfio_ccw_cp.c
> --- a/drivers/s390/cio/vfio_ccw_cp.c
> +++ b/drivers/s390/cio/vfio_ccw_cp.c
> @@ -810,6 +810,17 @@ void cp_free(struct channel_program *cp)
>  	}
>  }
>  
> +void cp_free(struct channel_program *cp)
> +{
> +	struct vfio_ccw_private *private =
> +		container_of(cp, struct vfio_ccw_private, cp);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&private->cp_lock, flags);
> +	cp_free_locked(cp);

[Severity: High]
Does holding the cp_lock spinlock here lead to a scheduling while atomic bug?

When cp_free_locked() cascades into the unpinning logic:

cp_free_locked()
  page_array_unpin_free()
    vfio_unpin_pages()
      vfio_iommu_type1_unpin_pages()
        mutex_lock(&iommu->lock);

The IOMMU backend unconditionally acquires a sleeping lock while the
thread is in atomic context from the spin_lock_irqsave(). Can this crash
the kernel when I/O request cleanup executes?

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

[ ... ]

> 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
> @@ -36,10 +36,15 @@ static void vfio_ccw_dma_unmap(struct vfio_device *vdev, u64 iova, u64 length)
>  {
>  	struct vfio_ccw_private *private =
>  		container_of(vdev, struct vfio_ccw_private, vdev);
> +	unsigned long flags;
>  
>  	/* Drivers MUST unpin pages in response to an invalidation. */
> -	if (!cp_iova_pinned(&private->cp, iova, length))
> +	spin_lock_irqsave(&private->cp_lock, flags);
> +	if (!cp_iova_pinned(&private->cp, iova, length)) {

[Severity: High]
This is a pre-existing issue, but does adding the lock here expose a NULL
pointer dereference due to incomplete locking elsewhere?

In fsm_io_request(), cp_init() and cp_prefetch() are called without
holding the new cp_lock. cp_init() sets cp->initialized = true. Then
cp_prefetch() calls page_array_alloc(), which does:

page_array_alloc()
  pa->pa_nr = len;
  pa->pa_iova = kzalloc_objs(*pa->pa_iova, len);

If a concurrent unmap enters vfio_ccw_dma_unmap(), acquires cp_lock, and
iterates the channel program because initialized is true, can it
dereference the still-NULL pa->pa_iova array since the initializing
thread hasn't allocated it yet?

> +		spin_unlock_irqrestore(&private->cp_lock, flags);
>  		return;
> +	}
> +	spin_unlock_irqrestore(&private->cp_lock, flags);
>  
>  	vfio_ccw_mdev_reset(private);
>  }

[ ... ]

> @@ -54,6 +59,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->cp_lock);

[Severity: High]
This isn't a bug introduced by this patch, but do these work items leak
and cause a use-after-free on device teardown?

According to the subsystem guidelines, once queue_work() is called,
flush_work() and other workqueue shutdown methods prevent us from leaking
the work struct on shutdown.

If an asynchronous subchannel event fires and queues crw_work, and the
user concurrently removes the mdev device, vfio_ccw_mdev_release_dev()
frees the private structure:

vfio_ccw_mdev_release_dev()
  kfree(private->cp.guest_cp);
  mutex_destroy(&private->io_mutex);

Will the workqueue scheduler eventually execute the pending crw_work and
access the already-freed private structure, since the removal function
drops references without explicitly cancelling background work?

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

[PATCH v2 7/7] 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.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_chp.c     | 15 ++++++++++-----
 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, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 38c176cf6295..bbc445efd67f 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -90,14 +90,18 @@ 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);
+	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);
-	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));
@@ -108,14 +112,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 bfb68de6e52c..12edccb62cbf 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);
 }
 
 /*
@@ -296,7 +298,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 d2475b3158cf..b3699ffd2f1d 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_WORK(&private->io_work, vfio_ccw_sch_io_todo);
 	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
 	spin_lock_init(&private->cp_lock);
+	spin_lock_init(&private->crw_lock);
 
 	private->cp.guest_cp = kzalloc_objs(struct ccw1, CCWCHAIN_LEN_MAX);
 	if (!private->cp.guest_cp)
@@ -137,10 +138,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 061b594fde36..4a43609d6422 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -120,6 +120,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 v2 7/7] 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 workqueue cancellation during device teardown leading to Use-After-Free.
- [High] Data race and Use-After-Free on `eventfd_ctx` via lockless modification of `crw_trigger`.
--

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

s390/vfio_ccw: implement a crw lock

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

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

> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index bfb68de6e52ca..12edccb62cbf5 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 is a pre-existing issue, but could this access to private->crw_trigger
cause a use-after-free of the eventfd context?

Looking at vfio_ccw_mdev_set_irqs() in drivers/s390/cio/vfio_ccw_ops.c, it
replaces private->crw_trigger and frees the old context with
eventfd_ctx_put() without holding crw_lock or any other synchronizing lock:

vfio_ccw_mdev_set_irqs()
            if (*ctx)
                eventfd_ctx_put(*ctx);
            *ctx = efdctx;

If userspace calls the VFIO_DEVICE_SET_IRQS ioctl concurrently with
vfio_ccw_crw_todo(), could the eventfd context be freed before
eventfd_signal() is executed here?

[ ... ]

> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index d2475b3158cf8..b3699ffd2f1d7 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c

[ ... ]

> @@ -137,10 +138,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);

[Severity: High]
This isn't a bug introduced by this patch, but does this function need to
call cancel_work_sync() for crw_work and io_work before the device is
released?

Since private->crw_work is dynamically queued onto vfio_ccw_work_q during
asynchronous hardware events, it might be pending or executing when the
device is removed.

If vfio_ccw_mdev_release_dev() runs and the private structure is subsequently
freed by the VFIO core without explicitly canceling the work, could it
result in a use-after-free when vfio_ccw_crw_todo() dereferences the freed
private pointer?

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