Re: [PATCH v7 03/19] 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:

New issues:
- [Medium] Lockless reads of `device->private` lack `READ_ONCE()`, leaving them vulnerable to compiler reloading and TOCTOU races.
- [Low] A race condition between reading `private->lcu` and `private->features` allows unsupported I/O on HYPER PAV alias devices.

Pre-existing issues:
- [High] The patch misses a NULL pointer dereference in `dasd_eckd_pprc_enabled()` reachable via the `copy_pair` sysfs attribute.
- [High] Asynchronous path event handlers unconditionally dereference `device->private` and are not canceled on initialization failure.
- [High] Lockless access to `device->private` in sysfs handlers allows a use-after-free if device initialization fails.
--

--- Patch [3]: [PATCH v7 03/19] s390/dasd: Guard sysfs discipline callbacks against unallocated private data ---
commit ef202481581d23c8dd03c5042f532b8675ad8998
Author: Stefan Haberland <[email protected]>

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

This commit guards several sysfs show/store handlers against unallocated
device->private pointers during device initialization.

Fixes: c729696bcf8b ("s390/dasd: Recognise data for ESE volumes")
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

[Severity: High]
This is a pre-existing issue, but does this patch miss guarding
dasd_eckd_pprc_enabled()?

When a device is set online, device->discipline is assigned before
check_device() allocates device->private. If the copy_pair sysfs attribute
is written during this window, dasd_copy_pair_store() locklessly invokes
device->discipline->pprc_enabled(device):

    if (device->discipline->pprc_enabled) {
        pprc_enabled = device->discipline->pprc_enabled(device);

The callback unconditionally dereferences
private->rdc_data.facilities.PPRC_enabled, which seems like it could still
cause a NULL pointer dereference.

[Severity: High]
This is also a pre-existing issue, but are asynchronous path event handlers
safe from this same NULL pointer dereference?

If a path event occurs while device->private is unallocated, the CIO layer
triggers dasd_eckd_pe_handler() which schedules do_pe_handler_work().
Functions like dasd_eckd_read_fc_security() then unconditionally dereference
private:

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

Additionally, if dasd_eckd_check_characteristics() encounters an error, its
out_err1 path frees device->private but does not appear to cancel the pending
pe_handler_worker workqueue, which could lead to a panic later.

[Severity: High]
This is another pre-existing issue, but is the lockless access to
device->private safe from use-after-free if device initialization fails?

While the new guards prevent NULL pointer dereferences, device->private is
accessed locklessly in these handlers. If dasd_eckd_check_characteristics()
encounters an error, it calls kfree(device->private). If a concurrent sysfs
reader has just passed the if (!private) check, could it proceed to
dereference the freed memory?

[ ... ]

> @@ -1831,7 +1848,11 @@ static int dasd_eckd_read_ext_pool_info(struct dasd_device *device)
>  static int dasd_eckd_ext_size(struct dasd_device *device)
>  {
>  	struct dasd_eckd_private *private = device->private;
> -	struct dasd_ext_pool_sum eps = private->eps;
> +	struct dasd_ext_pool_sum eps;
> +
> +	if (!private)
> +		return 0;
> +	eps = private->eps;

[Severity: Medium]
Should we use READ_ONCE() when assigning private from device->private?

Because device->private is accessed locklessly and can be set to NULL
concurrently by the error path, could the compiler optimize this by
reloading the pointer from device->private after the NULL check? This might
allow a TOCTOU NULL pointer dereference if it gets set to NULL between the
check and the reload.

>  
>  	if (!eps.flags.extent_size_valid)
>  		return 0;

[ ... ]

> @@ -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)

[Severity: Low]
Could checking private->lcu here introduce a race with the subsequent
private->features check?

A sysfs reader thread could read private->lcu as NULL before it is populated
and bypass this abort. If preempted, check_device() could finish populating
both lcu and features. When the reader resumes, it could evaluate the newly
initialized features and proceed to issue the PSF command. Would this allow
an unsupported command on HYPER PAV aliases?

>  		return -EOPNOTSUPP;
>  
>  	/* may not be supported by the storage server */

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