Re: [PATCH v6 18/18] s390/dasd: Read cached unit address and LSS in the CCW build path

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

New issues:
- [Low] Missing READ_ONCE()/WRITE_ONCE() annotations for locklessly accessed cached variables.

Pre-existing issues:
- [High] Lockless CCW build paths read `uid.type` which is subject to a transient zero during configuration reload, resulting in malformed commands.
- [High] Race condition in `dasd_eckd_generate_uid` and `rebuild_device_uid` against configuration reload workers allowing Use-After-Free and kernel heap corruption.
- [High] Lockless access to `private->conf.data` in `dasd_eckd_fill_info` can lead to a Use-After-Free and leak freed kernel memory to userspace.
--

--- Patch [18]: [PATCH v6 18/18] s390/dasd: Read cached unit address and LSS in the CCW build path ---
commit db725aecd487c6d786378910088d76a932cca049
Author: Stefan Haberland <[email protected]>
Date:   Sat Aug 1 11:10:08 2026 +0000

    s390/dasd: Read cached unit address and LSS in the CCW build path
    
    The CCW build path (prefix_LRE, the full-track prefix and dso_ras) read the
    base address and LSS straight from conf.ned. That buffer is freed and
    reallocated by the reload worker (do_reload_device - dasd_eckd_read_conf -
    dasd_eckd_clear_conf_data), so a configuration change concurrent with I/O
    can free conf.ned while a request is being built.
    Use-after-free reported by KASAN in prefix_LRE.
    
    Read the cached copies instead.
    The unit address is already kept in uid.real_unit_addr, and the LSS is now
    cached in ned_lss. Both are refreshed under the ccwdev lock in
    dasd_eckd_generate_uid whenever the configuration is (re)read.
    Also fix for prepare for read subsystem data (prssd) users.
    
    Reviewed-by: Jan Höppner <[email protected]>
    Signed-off-by: Stefan Haberland <[email protected]>

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 09ef6d11f0ce7..816ea66fb2fe4 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -588,8 +588,9 @@ static int prefix_LRE(struct ccw1 *ccw, struct PFX_eckd_data *pfxdata,
>  		return -EINVAL;
>  	}
>  	pfxdata->format = format;
> -	pfxdata->base_address = basepriv->conf.ned->unit_addr;
> -	pfxdata->base_lss = basepriv->conf.ned->ID;
> +	/* cached copies - conf.ned may be freed under us by the reload worker */
> +	pfxdata->base_address = basepriv->ned_ua;
> +	pfxdata->base_lss = basepriv->ned_lss;

[Severity: Low]
Are READ_ONCE() and WRITE_ONCE() annotations needed for these cached
variables?

The variables private->ned_lss and private->ned_ua are updated in
dasd_eckd_generate_uid() under the ccwdev_lock, but are intentionally read
locklessly here in prefix_LRE().

The Linux kernel memory model typically requires READ_ONCE() and WRITE_ONCE()
for concurrent lockless accesses to prevent compiler optimizations like load
fusing and to document the intentional data race, which avoids KCSAN
warnings.

>  	pfxdata->validity.define_extent = 1;
>  
>  	/* private uid is kept up to date, conf_data may be outdated */

[Severity: High]
This is a pre-existing issue, but does this lockless read of
startpriv->uid.type risk seeing a transient zero?

Just after this comment, prefix_LRE() and prepare_itcw() locklessly read
startpriv->uid.type. When a reload worker concurrently calls create_uid(),
it memsets the entire uid structure to zero:

drivers/s390/block/dasd_eckd.c:create_uid() {
    ...
    memset(uid, 0, sizeof(struct dasd_uid));
    ...
}

If prefix_LRE() or prepare_itcw() reads startpriv->uid.type while it is
transiently zeroed, the reader may observe the type as 0 (UA_BASE_DEVICE)
instead of UA_BASE_PAV_ALIAS or UA_HYPER_PAV_ALIAS.

Consequently, the driver will silently fail to set the verify_base and
hyper_pav flags in the PFX hardware command, which could lead to malformed
storage requests, rejected I/Os, or potential data corruption.

[ ... ]

> @@ -806,6 +807,9 @@ static int dasd_eckd_generate_uid(struct dasd_device *device)
>  		return -ENODEV;
>  	spin_lock_irqsave(get_ccwdev_lock(device->cdev), flags);
>  	create_uid(&private->conf, &private->uid);
> +	/* cache LSS and unit address for the lockless CCW-build path */
> +	private->ned_lss = private->conf.ned->ID;
> +	private->ned_ua = private->conf.ned->unit_addr;
>  	spin_unlock_irqrestore(get_ccwdev_lock(device->cdev), flags);
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk involving
private->conf.ned in this function?

Just before the lock is acquired, dasd_eckd_generate_uid() locklessly checks:

drivers/s390/block/dasd_eckd.c:dasd_eckd_generate_uid() {
    ...
    if (!private->conf.ned || !private->conf.gneq)
        return -ENODEV;
    spin_lock_irqsave(...)
    ...
}

Concurrently, a reload worker executing dasd_eckd_read_conf() calls
dasd_eckd_clear_conf_data(), which frees the underlying conf.data memory
without acquiring ccwdev_lock.

Since conf.ned points inside the freed conf.data allocation,
dasd_eckd_generate_uid() can pass the lockless check but dereference freed
memory once inside the lock.

Additionally, rebuild_device_uid() writes directly to private->conf.data
using memcpy() without locking. If the reload worker frees conf.data
concurrently, rebuild_device_uid() might overwrite freed slab memory, causing
kernel heap corruption.

[Severity: High]
This is a pre-existing issue, but does dasd_eckd_fill_info() also risk a
use-after-free that could leak freed memory to userspace?

The BIODASDINFO ioctl path calls dasd_ioctl_information(), which invokes
dasd_eckd_fill_info() without acquiring any device locks:

drivers/s390/block/dasd_eckd.c:dasd_eckd_fill_info() {
    ...
    memcpy(info->configuration_data, private->conf.data, info->confdata_size);
    ...
}

Concurrently, a reload worker executing dasd_eckd_reload_device() calls
dasd_eckd_clear_conf_data(), setting private->conf.data to NULL and freeing
the underlying memory:

drivers/s390/block/dasd_eckd.c:dasd_eckd_clear_conf_data() {
    ...
    private->conf.data = NULL;
    ...
    kfree(device->path[i].conf_data);
}

If the ioctl thread reads the private->conf.data pointer just before the
reload worker frees it, the memcpy() will read from freed slab memory, leaking
potentially sensitive kernel data to userspace. If it reads NULL, it will
cause a kernel panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=18