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

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

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but a Time-Of-Check to Time-Of-Use (TOCTOU) Use-After-Free vulnerability exists in world-readable sysfs handlers when the initialization function `dasd_eckd_check_characteristics()` encounters an error.
- [High] The patch fixes NULL pointer dereferences in world-readable sysfs show handlers by guarding them against an unallocated `device->private` during `dasd_generic_set_online()`. However, it misses the `copy_pair` sysfs store handler, leaving it vulnerable to a NULL pointer dereference.
- [High] The patch leaves a vulnerable window where `device->private` is allocated but its internal pointers (like `private->conf.gneq`) are still NULL, causing a NULL pointer dereference when a sysfs attribute is written.
--

--- Patch [3]: [PATCH 03/19] s390/dasd: Guard sysfs discipline callbacks against unallocated private data ---
commit 3d2b943842a29d7f0ee2ab694c84c9d52b911329
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 d356a9f8f016f..b64ca714b53e7 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
> @@ -1492,6 +1492,8 @@ static void dasd_eckd_reset_path(struct dasd_device *device, __u8 pm)
>  	struct dasd_eckd_private *private = device->private;
>  	unsigned long flags;
>  
> +	if (!private)
> +		return;
>  	if (!private->fcx_max_data)
>  		private->fcx_max_data = get_fcx_max_data(device);

[Severity: High]
This isn't a bug introduced by this patch, but does this leave a window
where private->conf.gneq is dereferenced while NULL?

In dasd_eckd_check_characteristics(), device->private is allocated and
assigned, but then blocking I/O (dasd_eckd_read_conf()) occurs before
private->conf.gneq is set.

If a sysfs write for reset_path occurs during this sleep, the new !private
check passes and get_fcx_max_data() is called:

drivers/s390/block/dasd_eckd.c:get_fcx_max_data() {
    ...
    fcx_in_gneq = private->conf.gneq->reserved2[7] & 0x04;
    ...
}

Can this cause a kernel panic if private->conf.gneq is still NULL?

[ ... ]

> @@ -1647,6 +1649,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: Critical]
This is a pre-existing issue, but does this lockless check introduce a
Time-Of-Check to Time-Of-Use (TOCTOU) Use-After-Free?

If dasd_generic_set_online() is running concurrently and
dasd_eckd_check_characteristics() encounters an error (like an I/O failure),
it manually frees private:

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

Since sysfs readers like dasd_eckd_is_ese() don't hold a lock protecting
device->private, could private be freed immediately after the !private
check but before private->vsq.vol_info.ese is accessed?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but it looks like the copy_pair
sysfs store handler is missing a similar safeguard.

If an administrator writes to copy_pair concurrently with
dasd_generic_set_online(), it executes dasd_copy_pair_store() which
locklessly accesses the discipline:

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

If device->discipline is NULL during the window before it is assigned,
could this cause a NULL pointer dereference?

Furthermore, even if discipline is assigned, dasd_eckd_pprc_enabled()
unconditionally dereferences device->private without checking if it is
allocated:

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

Should these locations be guarded as well?

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