[PATCH 0/3] s390/cio: Harden pmcw/schib handling for dnv=0

[PATCH 0/3] s390/cio: Harden pmcw/schib handling for dnv=0

[Vineeth Vijayan]

For I/O subchannels, pmcw.dnv must be validated before relying on any
other PMCW or SCHIB fields.
Two issues exist today:
 - Some I/O entry points check pmcw.ena without first verifying that
   pmcw.dnv is set.
 - cio_update_schib() updates the cached SCHIB before validating
   pmcw.dnv, allowing an stsch result with dnv=0 to leave stale or
   undefined data in sch->schib.
 
This can trigger spurious, non-fatal error messages in the guest kernel
log when a virtio device is being detached.
 
Fix this by validating pmcw.dnv before updating the cached SCHIB. Also
clear the cached SCHIB when stsch succeeds but returns dnv=0, ensuring
that stale state is not retained.
 
Additionally, add pmcw.dnv checks before pmcw.ena checks in I/O entry
points and return -ENODEV when no device is present. Guard remaining
direct accesses to cached PMCW fields, such as chpid[] and pam, to
ensure they are only evaluated when the SCHIB contents are valid.
 
This was reported and discussed at:
 
Link: https://lore.kernel.org/linux-s390/20260612155407.199218-1-wbezenah@linux.ibm.com/ 

Vineeth Vijayan (3):
  s390/cio: Fix cio_update_schib() to not cache invalid schib
  s390/cio: Check pmcw.dnv before pmcw.ena in I/O entry points
  s390/cio: Guard PMCW field accesses with dnv check

 drivers/s390/cio/chp.c          |  3 +++
 drivers/s390/cio/cio.c          | 11 +++++++----
 drivers/s390/cio/device.c       |  9 +++++----
 drivers/s390/cio/device_fsm.c   |  3 +++
 drivers/s390/cio/device_ops.c   | 21 +++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_fsm.c |  2 +-
 6 files changed, 40 insertions(+), 9 deletions(-)

-- 
2.53.0

[PATCH 1/3] s390/cio: Fix cio_update_schib() to not cache invalid schib

[Vineeth Vijayan]

When pmcw.dnv is 0, the contents of all SCHIB fields are unpredictable.
Zero sch->schib in that case to prevent subsequent code from making
decisions based on unpredictable data.

Reported-by: William Bezenah <wbezenah@linux.ibm.com>
Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Fixes: 8c58a229688c ("s390/cio: Do not unregister the subchannel based on DNV")
---
 drivers/s390/cio/cio.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/cio.c b/drivers/s390/cio/cio.c
index 70dc8cc76594..e1c62eb60cca 100644
--- a/drivers/s390/cio/cio.c
+++ b/drivers/s390/cio/cio.c
@@ -453,7 +453,8 @@ EXPORT_SYMBOL_GPL(cio_commit_config);
 /**
  * cio_update_schib - Perform stsch and update schib if subchannel is valid.
  * @sch: subchannel on which to perform stsch
- * Return zero on success, -ENODEV otherwise.
+ * Return zero on success, -ENODEV if the subchannel is not operational,
+ * -EACCES if the subchannel has no valid device.
  */
 int cio_update_schib(struct subchannel *sch)
 {
@@ -462,10 +463,12 @@ int cio_update_schib(struct subchannel *sch)
 	if (stsch(sch->schid, &schib))
 		return -ENODEV;
 
-	memcpy(&sch->schib, &schib, sizeof(schib));
-
-	if (!css_sch_is_valid(&schib))
+	if (!css_sch_is_valid(&schib)) {
+		memset(&sch->schib, 0, sizeof(sch->schib));
 		return -EACCES;
+	}
+
+	memcpy(&sch->schib, &schib, sizeof(schib));
 
 	return 0;
 }
-- 
2.53.0

[PATCH 2/3] s390/cio: Check pmcw.dnv before pmcw.ena in I/O entry points

[Vineeth Vijayan]

The device number valid (dnv) bit in the PMCW must be checked before
acting on any other PMCW fields for IO-type subchannels. A subchannel
with dnv=0 has no valid device number associated, making it meaningless
to evaluate the enabled (ena) state or issue any I/O instruction against
it.

Reported-by: William Bezenah <wbezenah@linux.ibm.com>
Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Fixes: 8c58a229688c ("s390/cio: Do not unregister the subchannel based on DNV")
---
 drivers/s390/cio/device_ops.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c
index 61c07b4a0fe8..c1ba4a19368f 100644
--- a/drivers/s390/cio/device_ops.c
+++ b/drivers/s390/cio/device_ops.c
@@ -142,6 +142,8 @@ int ccw_device_clear(struct ccw_device *cdev, unsigned long intparm)
 	if (!cdev || !cdev->dev.parent)
 		return -ENODEV;
 	sch = to_subchannel(cdev->dev.parent);
+	if (!sch->schib.pmcw.dnv)
+		return -ENODEV;
 	if (!sch->schib.pmcw.ena)
 		return -EINVAL;
 	if (cdev->private->state == DEV_STATE_NOT_OPER)
@@ -198,6 +200,8 @@ int ccw_device_start_timeout_key(struct ccw_device *cdev, struct ccw1 *cpa,
 	if (!cdev || !cdev->dev.parent)
 		return -ENODEV;
 	sch = to_subchannel(cdev->dev.parent);
+	if (!sch->schib.pmcw.dnv)
+		return -ENODEV;
 	if (!sch->schib.pmcw.ena)
 		return -EINVAL;
 	if (cdev->private->state == DEV_STATE_NOT_OPER)
@@ -379,6 +383,8 @@ int ccw_device_halt(struct ccw_device *cdev, unsigned long intparm)
 	if (!cdev || !cdev->dev.parent)
 		return -ENODEV;
 	sch = to_subchannel(cdev->dev.parent);
+	if (!sch->schib.pmcw.dnv)
+		return -ENODEV;
 	if (!sch->schib.pmcw.ena)
 		return -EINVAL;
 	if (cdev->private->state == DEV_STATE_NOT_OPER)
@@ -413,6 +419,8 @@ int ccw_device_resume(struct ccw_device *cdev)
 	if (!cdev || !cdev->dev.parent)
 		return -ENODEV;
 	sch = to_subchannel(cdev->dev.parent);
+	if (!sch->schib.pmcw.dnv)
+		return -ENODEV;
 	if (!sch->schib.pmcw.ena)
 		return -EINVAL;
 	if (cdev->private->state == DEV_STATE_NOT_OPER)
@@ -548,6 +556,8 @@ int ccw_device_tm_start_timeout_key(struct ccw_device *cdev, struct tcw *tcw,
 	int rc;
 
 	sch = to_subchannel(cdev->dev.parent);
+	if (!sch->schib.pmcw.dnv)
+		return -ENODEV;
 	if (!sch->schib.pmcw.ena)
 		return -EINVAL;
 	if (cdev->private->state == DEV_STATE_VERIFY) {
@@ -694,6 +704,8 @@ int ccw_device_tm_intrg(struct ccw_device *cdev)
 {
 	struct subchannel *sch = to_subchannel(cdev->dev.parent);
 
+	if (!sch->schib.pmcw.dnv)
+		return -ENODEV;
 	if (!sch->schib.pmcw.ena)
 		return -EINVAL;
 	if (cdev->private->state != DEV_STATE_ONLINE)
-- 
2.53.0

[PATCH 3/3] s390/cio: Guard PMCW field accesses with dnv check

[Vineeth Vijayan]

When PMCW.DNV is 0, no I/O device is associated with the subchannel.
However, several code paths access PMCW fields directly from the cached
sch->schib without first invoking the update helper. Add explicit DNV
validation before accessing PMCW fields from the cached SCHIB to avoid
using invalid data.

Reported-by: William Bezenah <wbezenah@linux.ibm.com>
Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>
Reviewed-by: Peter Oberparleiter <oberpar@linux.ibm.com>
Fixes: 8c58a229688c ("s390/cio: Do not unregister the subchannel based on DNV")
---
 drivers/s390/cio/chp.c          | 3 +++
 drivers/s390/cio/device.c       | 9 +++++----
 drivers/s390/cio/device_fsm.c   | 3 +++
 drivers/s390/cio/device_ops.c   | 9 +++++++++
 drivers/s390/cio/vfio_ccw_fsm.c | 2 +-
 5 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
index c890f21a82ce..eaf0527bff6c 100644
--- a/drivers/s390/cio/chp.c
+++ b/drivers/s390/cio/chp.c
@@ -78,6 +78,9 @@ u8 chp_get_sch_opm(struct subchannel *sch)
 	int opm;
 	int i;
 
+	if (!sch->schib.pmcw.dnv)
+		return 0;
+
 	opm = 0;
 	chp_id_init(&chpid);
 	for (i = 0; i < 8; i++) {
diff --git a/drivers/s390/cio/device.c b/drivers/s390/cio/device.c
index fb591118ecb2..68dd4a62975d 100644
--- a/drivers/s390/cio/device.c
+++ b/drivers/s390/cio/device.c
@@ -922,7 +922,7 @@ static int ccw_device_move_to_sch(struct ccw_device *cdev,
 
 	if (!sch_is_pseudo_sch(old_sch)) {
 		spin_lock_irq(&old_sch->lock);
-		old_enabled = old_sch->schib.pmcw.ena;
+		old_enabled = old_sch->schib.pmcw.dnv && old_sch->schib.pmcw.ena;
 		rc = 0;
 		if (old_enabled)
 			rc = cio_disable_subchannel(old_sch);
@@ -941,7 +941,7 @@ static int ccw_device_move_to_sch(struct ccw_device *cdev,
 		CIO_MSG_EVENT(0, "device_move(0.%x.%04x,0.%x.%04x)=%d\n",
 			      cdev->private->dev_id.ssid,
 			      cdev->private->dev_id.devno, sch->schid.ssid,
-			      sch->schib.pmcw.dev, rc);
+			      sch->schid.sch_no, rc);
 		if (old_enabled) {
 			/* Try to re-enable the old subchannel. */
 			spin_lock_irq(&old_sch->lock);
@@ -1207,7 +1207,7 @@ static void io_subchannel_quiesce(struct subchannel *sch)
 	cdev = sch_get_cdev(sch);
 	if (cio_is_console(sch->schid))
 		goto out_unlock;
-	if (!sch->schib.pmcw.ena)
+	if (!sch->schib.pmcw.dnv || !sch->schib.pmcw.ena)
 		goto out_unlock;
 	ret = cio_disable_subchannel(sch);
 	if (ret != -EBUSY)
@@ -1254,7 +1254,8 @@ static int recovery_check(struct device *dev, void *data)
 	switch (cdev->private->state) {
 	case DEV_STATE_ONLINE:
 		sch = to_subchannel(cdev->dev.parent);
-		if ((sch->schib.pmcw.pam & sch->opm) == sch->vpm)
+		if (sch->schib.pmcw.dnv &&
+		    (sch->schib.pmcw.pam & sch->opm) == sch->vpm)
 			break;
 		fallthrough;
 	case DEV_STATE_DISCONNECTED:
diff --git a/drivers/s390/cio/device_fsm.c b/drivers/s390/cio/device_fsm.c
index ab419d40a8a7..b5686c25c83c 100644
--- a/drivers/s390/cio/device_fsm.c
+++ b/drivers/s390/cio/device_fsm.c
@@ -170,6 +170,9 @@ __recover_lost_chpids(struct subchannel *sch, int old_lpm)
 	int mask, i;
 	struct chp_id chpid;
 
+	if (!sch->schib.pmcw.dnv)
+		return;
+
 	chp_id_init(&chpid);
 	for (i = 0; i<8; i++) {
 		mask = 0x80 >> i;
diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c
index c1ba4a19368f..f2f7f8cba410 100644
--- a/drivers/s390/cio/device_ops.c
+++ b/drivers/s390/cio/device_ops.c
@@ -490,6 +490,8 @@ struct channel_path_desc_fmt0 *ccw_device_get_chp_desc(struct ccw_device *cdev,
 	struct chp_id chpid;
 
 	sch = to_subchannel(cdev->dev.parent);
+	if (!sch->schib.pmcw.dnv)
+		return NULL;
 	chp_id_init(&chpid);
 	chpid.id = sch->schib.pmcw.chpid[chp_idx];
 	return chp_get_chp_desc(chpid);
@@ -510,6 +512,8 @@ u8 *ccw_device_get_util_str(struct ccw_device *cdev, int chp_idx)
 	struct chp_id chpid;
 	u8 *util_str;
 
+	if (!sch->schib.pmcw.dnv)
+		return NULL;
 	chp_id_init(&chpid);
 	chpid.id = sch->schib.pmcw.chpid[chp_idx];
 	chp = chpid_to_chp(chpid);
@@ -662,6 +666,9 @@ int ccw_device_get_mdc(struct ccw_device *cdev, u8 mask)
 	struct chp_id chpid;
 	int mdc = 0, i;
 
+	if (!sch->schib.pmcw.dnv)
+		return 0;
+
 	/* Adjust requested path mask to excluded varied off paths. */
 	if (mask)
 		mask &= sch->lpm;
@@ -798,6 +805,8 @@ int ccw_device_get_chpid(struct ccw_device *cdev, int chp_idx, u8 *chpid)
 
 	if ((chp_idx < 0) || (chp_idx > 7))
 		return -EINVAL;
+	if (!sch->schib.pmcw.dnv)
+		return -ENODEV;
 	mask = 0x80 >> chp_idx;
 	if (!(sch->schib.pmcw.pim & mask))
 		return -ENODEV;
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 5fd94e9d5c61..9a000b0231d6 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -399,7 +399,7 @@ static void fsm_close(struct vfio_ccw_private *private,
 
 	spin_lock_irq(&sch->lock);
 
-	if (!sch->schib.pmcw.ena)
+	if (!sch->schib.pmcw.dnv || !sch->schib.pmcw.ena)
 		goto err_unlock;
 
 	ret = cio_disable_subchannel(sch);
-- 
2.53.0

Re: [PATCH 1/3] s390/cio: Fix cio_update_schib() to not cache invalid schib

[sashiko-bot]

> When pmcw.dnv is 0, the contents of all SCHIB fields are unpredictable.
> Zero sch->schib in that case to prevent subsequent code from making
> decisions based on unpredictable data.
> 
> Reported-by: William Bezenah <wbezenah@linux.ibm.com>
> Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>

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

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

Re: [PATCH 2/3] s390/cio: Check pmcw.dnv before pmcw.ena in I/O entry points

[sashiko-bot]

> The device number valid (dnv) bit in the PMCW must be checked before
> acting on any other PMCW fields for IO-type subchannels. A subchannel
> with dnv=0 has no valid device number associated, making it meaningless
> to evaluate the enabled (ena) state or issue any I/O instruction against
> it.
> 
> Reported-by: William Bezenah <wbezenah@linux.ibm.com>
> Signed-off-by: Vineeth Vijayan <vneethv@linux.ibm.com>

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

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

Re: [PATCH 3/3] s390/cio: Guard PMCW field accesses with dnv check

[sashiko-bot]

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

Pre-existing issues:
- [High] Lockless access and TOCTOU data race when validating sch->schib.pmcw.dnv and reading PMCW fields.
--

commit 2558ea997b16f6b5c46e330784e1d48f82b99802
Author: Vineeth Vijayan <vneethv@linux.ibm.com>

s390/cio: Guard PMCW field accesses with dnv check

This commit adds explicit Device Number Valid (DNV) checks before accessing
PMCW fields from the cached SCHIB. It aims to prevent the use of invalid
data when no I/O device is associated with the subchannel.

> diff --git a/drivers/s390/cio/chp.c b/drivers/s390/cio/chp.c
> index c890f21a82ce3..eaf0527bff6cc 100644
> --- a/drivers/s390/cio/chp.c
> +++ b/drivers/s390/cio/chp.c
> @@ -78,6 +78,9 @@ u8 chp_get_sch_opm(struct subchannel *sch)
>  	int opm;
>  	int i;
>  
> +	if (!sch->schib.pmcw.dnv)
> +		return 0;
> +

[Severity: High]
This is a pre-existing issue, but does checking sch->schib.pmcw.dnv here
without holding sch->lock introduce a Time-of-Check to Time-of-Use (TOCTOU)
race?

Concurrent hardware events could trigger cio_update_schib(), which overwrites
sch->schib using a non-atomic memcpy() while holding sch->lock.

If this happens, could torn or invalid data be read from sch->schib.pmcw on
the subsequent lines when accessing sch->schib.pmcw.chpid[i]?

>  	opm = 0;
>  	chp_id_init(&chpid);
>  	for (i = 0; i < 8; i++) {

[ ... ]

> diff --git a/drivers/s390/cio/device_ops.c b/drivers/s390/cio/device_ops.c
> index c1ba4a19368f2..f2f7f8cba410b 100644
> --- a/drivers/s390/cio/device_ops.c
> +++ b/drivers/s390/cio/device_ops.c
> @@ -490,6 +490,8 @@ struct channel_path_desc_fmt0 *ccw_device_get_chp_desc(struct ccw_device *cdev,
>  	struct chp_id chpid;
>  
>  	sch = to_subchannel(cdev->dev.parent);
> +	if (!sch->schib.pmcw.dnv)
> +		return NULL;

[Severity: High]
This isn't a bug introduced by this patch, but doesn't this lockless DNV
validation before accessing PMCW fields also risk a data race?

Since callers cannot wrap these API functions in sch->lock (because functions
like chp_get_chp_desc() can block on memory allocations), is there a way to
safely read these fields without racing with cio_update_schib()?

>  	chp_id_init(&chpid);
>  	chpid.id = sch->schib.pmcw.chpid[chp_idx];
>  	return chp_get_chp_desc(chpid);
> @@ -510,6 +512,8 @@ u8 *ccw_device_get_util_str(struct ccw_device *cdev, int chp_idx)
>  	struct chp_id chpid;
>  	u8 *util_str;
>  
> +	if (!sch->schib.pmcw.dnv)
> +		return NULL;

[Severity: High]
This is a pre-existing issue, but is this lockless DNV validation also
subject to the same TOCTOU data race with cio_update_schib() as mentioned
above?

>  	chp_id_init(&chpid);
>  	chpid.id = sch->schib.pmcw.chpid[chp_idx];
>  	chp = chpid_to_chp(chpid);

[ ... ]

> @@ -662,6 +666,9 @@ int ccw_device_get_mdc(struct ccw_device *cdev, u8 mask)
>  	struct chp_id chpid;
>  	int mdc = 0, i;
>  
> +	if (!sch->schib.pmcw.dnv)
> +		return 0;
> +

[Severity: High]
This is a pre-existing issue, but could this lockless access to PMCW fields
lead to reading garbage data if cio_update_schib() updates sch->schib
concurrently?

>  	/* Adjust requested path mask to excluded varied off paths. */
>  	if (mask)
>  		mask &= sch->lpm;

[ ... ]

> @@ -798,6 +805,8 @@ int ccw_device_get_chpid(struct ccw_device *cdev, int chp_idx, u8 *chpid)
>  
>  	if ((chp_idx < 0) || (chp_idx > 7))
>  		return -EINVAL;
> +	if (!sch->schib.pmcw.dnv)
> +		return -ENODEV;

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless DNV
validation share the same TOCTOU data race with cio_update_schib() when
reading PMCW fields?

>  	mask = 0x80 >> chp_idx;
>  	if (!(sch->schib.pmcw.pim & mask))
>  		return -ENODEV;

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