Re: [PATCH v6 03/18] s390/dasd: Guard sysfs discipline callbacks against unallocated private data

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

Pre-existing issues:
- [Critical] The patch fails to protect against a lockless Time-of-Check to Time-of-Use (TOCTOU) race where `device->private` is freed on the error path of `discipline->check_device()`, leading to a Use-After-Free or NULL pointer dereference.
- [High] The patch adds NULL checks for `device->private` in multiple sysfs-triggered callbacks but misses `dasd_eckd_pprc_enabled`, leaving a race condition that can panic the kernel.
- [High] The newly added `!private` checks are insufficient because callbacks subsequently dereference `private->conf.ned` before it is initialized.
- [High] In `dasd_release_space()`, an incorrect logical AND (`&&`) causes a NULL pointer dereference for disciplines without an `is_ese` callback, and silently bypasses the check for disciplines that have it.
- [High] The asynchronous attention interrupt handler path is vulnerable to the same `device->private` NULL pointer dereference race condition that the patch attempts to fix for sysfs.
--

--- Patch [3]: [PATCH v6 03/18] s390/dasd: Guard sysfs discipline callbacks against unallocated private data ---
commit 698838157896167de2e3f1afbbb4fdad63518bec
Author: Stefan Haberland <[email protected]>

s390/dasd: Guard sysfs discipline callbacks against unallocated private data

This patch adds NULL checks for device->private in several sysfs
show/store handlers inside dasd_eckd.c. It aims to fix a panic caused by
an unprivileged read of a sysfs attribute during the window between
discipline assignment and device->private allocation.

Cc: [email protected]

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 74fe73b5738ab..6e2fd445688cb 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c

[ ... ]

> @@ -1646,6 +1648,9 @@ static int dasd_eckd_is_ese(struct dasd_device *device)
>  {
>  	struct dasd_eckd_private *private = device->private;
>  
> +	if (!private)
> +		return 0;
> +
>  	return private->vsq.vol_info.ese;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this pattern leave the
code vulnerable to a Time-of-Check to Time-of-Use race?

If a sysfs read calls dasd_eckd_is_ese() during dasd_generic_set_online(),
it can pass the new !private check. If the dasd_eckd_check_characteristics()
error path simultaneously frees device->private locklessly:

drivers/s390/block/dasd_eckd.c:dasd_eckd_check_characteristics() {
    ...
out_err1:
    kfree(device->private);
    device->private = NULL;
    ...
}

Could the sysfs handler then resume and dereference the freed memory when
accessing private->vsq.vol_info.ese?

[Severity: High]
This is a pre-existing issue, but while looking at the is_ese callback,
does dasd_release_space() in drivers/s390/block/dasd_ioctl.c have an issue
with its short-circuit logic?

drivers/s390/block/dasd_ioctl.c:dasd_release_space() {
    ...
    if (!device->discipline->is_ese && !device->discipline->is_ese(device))
    ...
}

If the discipline does not implement is_ese (like FBA devices), wouldn't
the left operand evaluate to true, forcing evaluation of the right operand
and causing a NULL pointer dereference? And if it does implement is_ese,
wouldn't the left operand evaluate to false, bypassing the check entirely?

[ ... ]

> @@ -5935,8 +5965,11 @@ static int dasd_eckd_query_host_access(struct dasd_device *device,
>  	struct ccw1 *ccw;
>  	int rc;
>  
> +	if (!private)
> +		return -ENODEV;
> +
>  	/* not available for HYPER PAV alias devices */
> -	if (!device->block && private->lcu->pav == HYPER_PAV)
> +	if (!device->block && private->lcu && private->lcu->pav == HYPER_PAV)
>  		return -EOPNOTSUPP;

[Severity: High]
This isn't a bug introduced by this patch, but is this check sufficient to
prevent dereferencing uninitialized pointers later in the function?

Inside dasd_eckd_check_characteristics(), device->private is allocated
immediately, but private->conf.ned is not populated until
dasd_eckd_read_conf() succeeds. If this sysfs handler executes during that
window, the !private check passes, but further down in
dasd_eckd_query_host_access() (and similarly in dasd_eckd_read_vol_info()),
the code performs:

    prssdp->lss = private->conf.ned->ID;
    prssdp->volume = private->conf.ned->unit_addr;

Since private->conf.ned would still be NULL during this window, can this
cause a NULL pointer dereference?

[ ... ]

> @@ -6801,6 +6834,9 @@ static int dasd_eckd_hpf_enabled(struct dasd_device *device)
>  {
>  	struct dasd_eckd_private *private = device->private;
>  
> +	if (!private)
> +		return 0;
> +
>  	return private->fcx_max_data ? 1 : 0;
>  }

[Severity: High]
This is a pre-existing issue, but does dasd_eckd_pprc_enabled() also need
this NULL check?

If a user writes to the copy_pair sysfs attribute during device
initialization, dasd_copy_pair_store() locklessly calls the discipline
callback:

drivers/s390/block/dasd_devmap.c:dasd_copy_pair_store() {
    ...
    pprc_enabled = device->discipline->pprc_enabled(device);
    ...
}

Inside dasd_eckd_pprc_enabled(), device->private is unconditionally
dereferenced:

drivers/s390/block/dasd_eckd.c:dasd_eckd_pprc_enabled() {
    ...
    return private->rdc_data.facilities.PPRC_enabled;
}

Could this trigger a NULL pointer dereference if hit during the same
initialization window?

[Severity: High]
This is a pre-existing issue, but is the asynchronous attention interrupt
handler path vulnerable to this same race condition?

Since the subchannel is enabled during probe, a hardware attention interrupt
could arrive during the same device initialization window.
dasd_int_handler() locklessly calls device->discipline->check_attention(),
which schedules dasd_eckd_check_attention_work():

drivers/s390/block/dasd_eckd.c:dasd_eckd_check_attention() {
    ...
    INIT_WORK(&data->worker, dasd_eckd_check_attention_work);
    schedule_work(&data->worker);
    ...
}

This asynchronous work eventually calls dasd_eckd_cuir_quiesce() which
unconditionally dereferences private->lcu:

drivers/s390/block/dasd_eckd.c:dasd_eckd_cuir_quiesce() {
    ...
    list_for_each_entry_safe(dev, n, &private->lcu->active_devices,
                             alias_list) {
    ...
}

If device->private is not yet allocated, does this cause a NULL pointer
dereference?

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