Re: [PATCH 19/19] 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:
- [High] Lockless readers can observe a mismatched, torn state of `ned_lss` and `ned_ua` due to sequential 8-bit reads and writes.

Pre-existing issues:
- [Critical] Concurrent execution of path event handler and device reload leads to unlocked `memcpy` into freed `private->conf.data`.
- [High] sysfs `path_reset` attribute accesses `private->conf.gneq` without locks, racing with configuration data reallocation.
- [High] Concurrent execution of `dasd_eckd_generate_uid` and `dasd_eckd_read_conf` results in a Use-After-Free read of the dangling `private->conf.ned` pointer.
--

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

The CCW build path read the base address and LSS straight from conf.ned,
which could be freed by the reload worker. This patch caches unit address
and LSS in the private structure and reads the cached copies to prevent
use-after-free.

>     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.

[Severity: Critical]
This is a pre-existing issue, but does concurrent execution of the path event
handler and device reload lead to an unlocked memcpy into freed memory?

The path event worker do_pe_handler_work() calls rebuild_device_uid(), which
copies configuration data using:

memcpy(private->conf.data, data->rcd_buffer, DASD_ECKD_RCD_DATA_SIZE);

Concurrently, do_reload_device() calls dasd_eckd_clear_conf_data(), which
explicitly sets private->conf.data = NULL and frees the buffer.

Neither path acquires ccwdev_lock or state_mutex. Can this cause the memcpy
to write into freed heap memory or dereference a NULL destination pointer?

[Severity: High]
This is another pre-existing issue, but does the sysfs path_reset attribute
access private->conf.gneq without locks, racing with configuration data
reallocation?

The sysfs store function dasd_path_reset_store() calls dasd_eckd_reset_path(),
which calls get_fcx_max_data(). This function reads
private->conf.gneq->reserved2[7] without acquiring any locks.

Since do_reload_device() can concurrently execute dasd_eckd_clear_conf_data()
and free the memory block that conf.gneq points into, while failing to set
conf.gneq to NULL, can get_fcx_max_data() dereference a dangling pointer and
cause a use-after-free read?

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 5c1f328b7c8d7..fd23ac25a7da9 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -589,8 +589,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 = READ_ONCE(basepriv->ned_ua);
> +	pfxdata->base_lss = READ_ONCE(basepriv->ned_lss);

[Severity: High]
Can lockless readers observe a mismatched, torn state of ned_lss and ned_ua due
to sequential 8-bit reads?

Since ned_lss and ned_ua are separate 8-bit variables, they are read
sequentially here.

If a concurrent configuration change updates both variables, could a lockless
reader be preempted between the two reads and observe a torn state, such as
the old LSS but the new unit address?

[ ... ]

> @@ -807,6 +808,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 */
> +	WRITE_ONCE(private->ned_lss, private->conf.ned->ID);
> +	WRITE_ONCE(private->ned_ua, private->conf.ned->unit_addr);

[Severity: High]
This is a pre-existing issue, but does concurrent execution of
dasd_eckd_generate_uid() and dasd_eckd_read_conf() result in a use-after-free
read of the dangling private->conf.ned pointer here?

Since dasd_eckd_clear_conf_data() frees the configuration data buffer but
fails to nullify the derived private->conf.ned and private->conf.gneq pointers,
the lockless check for !private->conf.ned at the start of this function passes
because the pointer is dangling rather than NULL.

Does this cause the code to acquire ccwdev_lock and access freed memory inside
create_uid() and these newly added WRITE_ONCE() statements?

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