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

[sashiko-bot@kernel.org]

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