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

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

Pre-existing issues:
- [High] Transient zeroing of `uid.type` disables alias device verification in lockless CCW-build paths.
- [High] Concurrent sysfs write to `path_reset` triggers a Use-After-Free or NULL pointer dereference on `conf.gneq`.
- [High] Concurrent `BIODASDINFO` / `BIODASDINFO2` ioctl triggers a Use-After-Free or NULL pointer dereference on `conf.data`.
- [High] Concurrent execution of path event handlers and configuration reload causes memory corruption and UAF on `conf.data` and `conf.ned`.
- [High] TOCTOU NULL pointer dereference in `dasd_eckd_generate_uid`.
--

--- Patch [17]: [PATCH v5 17/17] s390/dasd: Read cached unit address and LSS in the CCW build path ---
commit 962f258b40a4684c7ad8e7e9f2ae8c0f71efe3ff
Author: Stefan Haberland <[email protected]>

s390/dasd: Read cached unit address and LSS in the CCW build path

This commit resolves a use-after-free in the CCW build paths by caching
the unit address and LSS from conf.ned into the private device data. This
prevents a race condition where the reload worker frees conf.ned while
I/O requests are being built.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index eb2a2f8fac644..6548ee0c01d90 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;
>  	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 since this patch fixes lockless access
to ned_lss and ned_ua, could a similar problem occur with uid.type?

In drivers/s390/block/dasd_eckd.c, both prefix_LRE() and prepare_itcw()
read startpriv->uid.type locklessly during I/O submission:

    if (startpriv->uid.type == UA_BASE_PAV_ALIAS)
        pfxdata->validity.verify_base = 1;

Concurrently, a configuration reload calls create_uid() under ccwdev_lock,
which executes memset(uid, 0, sizeof(struct dasd_uid)).

Could this transient zeroing of the uid structure disable alias device
verification during these CCW-build paths?

[ ... ]

> @@ -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 isn't a bug introduced by this patch, but looking at the earlier checks
in dasd_eckd_generate_uid() in drivers/s390/block/dasd_eckd.c, there appears
to be a potential Time-of-Check to Time-of-Use race:

    if (!private->conf.ned || !private->conf.gneq)
        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;

Since the NULL check for private->conf.ned happens outside the ccwdev_lock,
could a concurrent reconfiguration (like in dasd_eckd_identify_conf_parts())
locklessly set conf->ned to NULL? If so, would this result in a NULL pointer
dereference when it is subsequently used inside create_uid() or when caching
private->ned_lss?

[ ... ]

While reviewing the fixes for conf.ned, I noticed a few other pre-existing
synchronization issues related to configuration reloads and path events in
drivers/s390/block/dasd_eckd.c.

[Severity: High]
This is a pre-existing issue, but is there a potential use-after-free
in get_fcx_max_data() during a concurrent sysfs path_reset?

When dasd_eckd_clear_conf_data() frees device->path[i].conf_data, it
appears to leave private->conf.gneq pointing to the freed memory. Could
a concurrent sysfs write to path_reset call get_fcx_max_data() and
locklessly read private->conf.gneq->reserved2[7], dereferencing the
dangling pointer?

[Severity: High]
This isn't a bug introduced by this patch, but does the BIODASDINFO ioctl
have a similar race with device reloads?

Looking at dasd_eckd_fill_info(), it locklessly reads private->conf.len
and performs a memcpy() from private->conf.data. If the reload worker
calls dasd_eckd_clear_conf_data() concurrently, it frees the underlying
buffer and sets conf.data = NULL. Could the ioctl thread read a non-zero
length but execute the memcpy() after the buffer is freed or NULLed,
leading to a crash or a memory leak to userspace?

[Severity: High]
This is a pre-existing issue, but is there a lack of mutual exclusion
between path event handlers and configuration reloads?

The do_pe_handler_work() and do_reload_device() workers might execute
concurrently. If do_reload_device() calls dasd_eckd_clear_conf_data(),
could rebuild_device_uid() in the PE handler concurrently execute a
lockless memcpy() into the freed private->conf.data? Would it then call
dasd_eckd_generate_uid(), which dereferences the dangling conf.ned pointer?

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