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

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

Pre-existing issues:
- [High] Lockless `memcpy` of `private->conf.data` in the IOCTL path is vulnerable to concurrent free/NULL assignment by the reload worker, potentially leaking kernel heap memory.
- [High] Unsynchronized access to `private->conf.gneq` in `get_fcx_max_data` can lead to a Use-After-Free or NULL pointer dereference during a sysfs path reset.
--

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

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

The CCW build path reads the base address and LSS straight from conf.ned,
which can be concurrently freed by the reload worker. Read the cached copies
instead, which are refreshed under the ccwdev lock.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 57e927926be1a..d9c5c02c3af18 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 = READ_ONCE(basepriv->ned_ua);
> +	pfxdata->base_lss = READ_ONCE(basepriv->ned_lss);

[Severity: High]
This is a pre-existing issue, but since we are fixing reload worker races
here, does the same race apply to the IOCTL path in dasd_eckd_fill_info()?

The BIODASDINFO2 ioctl locklessly reads private->conf.len and
private->conf.data for a memcpy():

dasd_eckd_fill_info() {
    ...
    info->confdata_size = min_t(unsigned long, private->conf.len,
                                sizeof(info->configuration_data));
    memcpy(info->configuration_data, private->conf.data,
           info->confdata_size);
    ...
}

Concurrently, the reload worker frees the memory and clears the pointer:

dasd_eckd_clear_conf_data() {
    ...
    private->conf.data = NULL;
    private->conf.len = 0;
    ...
}

Could this result in a NULL pointer dereference or copying freed heap memory
to user-space if the ioctl races with the reload worker?

[Severity: High]
This is also a pre-existing issue, but does a similar race exist with
private->conf.gneq during a sysfs path reset?

Writing to the sysfs path_reset attribute can invoke get_fcx_max_data()
locklessly:

dasd_path_reset_store() {
    ...
    if (device->discipline && device->discipline->reset_path)
        device->discipline->reset_path(device, (__u8) val);
    ...
}

get_fcx_max_data() {
    ...
    fcx_in_gneq = private->conf.gneq->reserved2[7] & 0x04;
    ...
}

If the reload worker calls dasd_eckd_clear_conf_data(), it frees the
underlying buffer without clearing the conf.gneq pointer itself (only
conf.data is set to NULL). Could a concurrent path_reset write trigger a
use-after-free here?

[ ... ]

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