Re: [PATCH v5 02/17] s390/dasd: Guard sysfs discipline callbacks against unallocated private data

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

Pre-existing issues:
- [Critical] The patch fails to fully resolve the initialization race for the `host_access_count` sysfs attribute, resulting in a NULL pointer dereference. While it adds a `!private` check to `dasd_eckd_query_host_access()`, it overlooks that `private->lcu` remains NULL until populated later by `dasd_alias_make_device_known_to_lcu()`. An unprivileged sysfs read during this window will bypass the `!private` check and unconditionally dereference `private->lcu->pav`, causing a kernel panic.
- [Critical] The `if (!private)` checks added by the patch fail to protect against concurrent freeing of `device->private` during device initialization errors, leading to a Use-After-Free and memory corruption. If `dasd_eckd_check_characteristics` fails, its error path frees `device->private`. A concurrent unprivileged sysfs read can fetch the non-NULL pointer just before it is freed, bypass the check, and subsequently dereference or write to the freed memory.
- [High] Missing NULL pointer checks in `dasd_copy_pair_store` and `dasd_eckd_pprc_enabled` allow a kernel panic when configuring copy pairs via sysfs during device initialization. The sysfs store handler `dasd_copy_pair_store` does not verify if `device->discipline` is allocated before dereferencing `device->discipline->pprc_enabled`. Furthermore, if `device->discipline` is assigned but `device->private` is not yet allocated, `dasd_eckd_pprc_enabled` unconditionally dereferences `device->private`, resulting in a kernel panic.
- [High] Missing NULL pointer check for `device->private` in `dasd_eckd_read_fc_security()`, leading to a kernel panic triggered by asynchronous path events during initialization. If a hardware path event occurs after `device->discipline` is assigned but before `device->private` is allocated, `do_pe_handler_work()` executes and calls `dasd_eckd_read_fc_security()`, which unconditionally dereferences `private`.
--

--- Patch [2]: [PATCH v5 02/17] s390/dasd: Guard sysfs discipline callbacks against unallocated private data ---
commit d022f6e90af16afe7ae1766d9ba490832f14cca6
Author: Stefan Haberland <[email protected]>

s390/dasd: Guard sysfs discipline callbacks against unallocated private data
    
Several sysfs show/store handlers call a discipline callback that
dereferences device->private, either directly or through the
DASD_DEFINE_ATTR() macro. During dasd_generic_set_online() the discipline
is assigned before check_device() allocates device->private, so an
unprivileged read of one of these world-readable attributes in that window
dereferences a NULL pointer and panics.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 74fe73b5738ab..a820d68122d93 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -1666,6 +1674,9 @@ static int dasd_eckd_space_configured(struct dasd_device *device)
>  	struct dasd_eckd_private *private = device->private;
>  	int rc;
>  
> +	if (!private)
> +		return 0;
> +

[Severity: Critical]
This is a pre-existing issue, but does this check protect against concurrent
freeing of device->private during device initialization errors?

If dasd_eckd_check_characteristics() fails, its error path frees
device->private without synchronization against concurrent lockless sysfs
readers:

    kfree(device->private);
    device->private = NULL;

Could a concurrent unprivileged sysfs read fetch the non-NULL pointer, pass
this check, and then perform a Use-After-Free when dasd_eckd_read_vol_info()
copies data into the freed private->vsq buffer?

>  	rc = dasd_eckd_read_vol_info(device);
[ ... ]
> @@ -5935,6 +5965,9 @@ 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)

[Severity: Critical]
This is a pre-existing issue, but could this still lead to a NULL pointer
dereference?

While this check ensures private is not NULL, private->lcu can remain NULL
until it is populated later by dasd_alias_make_device_known_to_lcu().

If an unprivileged sysfs read triggers dasd_eckd_host_access_count() which
calls dasd_eckd_query_host_access() during this window, wouldn't it bypass
the !private check and unconditionally dereference private->lcu->pav?

>  		return -EOPNOTSUPP;

[Severity: High]
This is a pre-existing issue, but I noticed a couple of other places that
might need similar NULL pointer checks.

In dasd_copy_pair_store() in drivers/s390/block/dasd_devmap.c, could
device->discipline be dereferenced while it is NULL when a user writes to
the copy_pair sysfs attribute during device initialization?

    if (device->discipline->pprc_enabled) {

Additionally, if device->discipline is set but device->private is not yet
allocated, could dasd_eckd_pprc_enabled() in drivers/s390/block/dasd_eckd.c
unconditionally dereference device->private?

    return private->rdc_data.facilities.PPRC_enabled;

[Severity: High]
This is also a pre-existing issue, but is a NULL check for device->private
needed in dasd_eckd_read_fc_security() as well?

If a hardware path event occurs after device->discipline is assigned but
before device->private is allocated, do_pe_handler_work() executes and calls
dasd_eckd_read_fc_security().

Could this unconditionally dereference private?

    rc = chsc_scud(private->uid.ssid, (u64 *)esm, &esm_valid);

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