Re: [PATCH] scsi: scsi_dh_rdac: fix NULL pointer dereference in error handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] A malicious SCSI device can trigger a stack buffer overflow in `get_lun_info()` by providing an oversized `array_uniq_id_len` in VPD page 0xC8.
- [High] In the error handling path of `rdac_bus_attach()`, if an initialization step such as `check_ownership` or `set_mode_select` fails, the handler data `h` is freed without removing `h->node` from the RCU-protected `h->ctlr->dh_list`, causing a Use-After-Free.
- [High] A time-of-check to time-of-use (TOCTOU) race condition between `rdac_bus_detach()` and the `send_mode_select()` workqueue function allows detachment to proceed while work is actively executing, resulting in Use-After-Free of handler data.
--

--- Patch [1]: [PATCH] scsi: scsi_dh_rdac: fix NULL pointer dereference in error handling ---
commit 712793434a46fcd5b5270defe36dc411e0363100
Author: Aleksey Shishkin <aishishkin@cloud.ru>
Subject: scsi: scsi_dh_rdac: fix NULL pointer dereference in error handling

This patch fixes a logic error in error handling where the return code was
incorrectly overwritten to SCSI_DH_OK after a failed get_controller() call,
moving the assignment into the success block.

> diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
> index 88c8e36b221e2..ed02e1737bd79 100644
> --- a/drivers/scsi/device_handler/scsi_dh_rdac.c
> +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
> @@ -455,9 +455,9 @@ static int initialize_controller(struct scsi_device *sdev,
>  		else {
>  			h->sdev = sdev;
>  			list_add_rcu(&h->node, &h->ctlr->dh_list);
> +			err = SCSI_DH_OK;
>  		}

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free in the
error handling path.

When initialize_controller() succeeds, it adds the node to the RCU list here.
However, if a subsequent initialization step fails in rdac_bus_attach(),
the error path looks like this:

rdac_bus_attach() {
    ...
clean_ctlr:
	spin_lock(&list_lock);
	kref_put(&h->ctlr->kref, release_controller);
	spin_unlock(&list_lock);

failed:
	kfree(h);
}

Is it possible to reach kfree(h) without removing the node from the
controller's dh_list? If the controller remains alive and is shared by
another LUN, could subsequent RCU traversals of the list access this freed
memory?

>  		spin_unlock(&list_lock);
> -		err = SCSI_DH_OK;
>  	}
>  	return err;
>  }

[Severity: High]
This is a pre-existing issue, but does rdac_bus_detach() have a race condition
with the mode select workqueue?

In rdac_bus_detach(), the code locklessly checks ms_queued before flushing:

rdac_bus_detach() {
    ...
    if (h->ctlr && h->ctlr->ms_queued)
        flush_workqueue(kmpath_rdacd);
    ...
}

However, the worker thread send_mode_select() clears this flag early before
completing the potentially blocking SCSI command:

send_mode_select() {
    ...
    ctlr->ms_queued = 0;
    ctlr->ms_sdev = NULL;
    ...
}

Could rdac_bus_detach() run concurrently during this window, see ms_queued as
zero, skip the workqueue flush, and proceed to free the handler data while
the worker is still actively executing?

[Severity: Critical]
This is a pre-existing issue, but is there a potential stack buffer overflow
in get_lun_info()?

The function retrieves device data into inqp and uses array_uniq_id_len
directly for a memory copy:

get_lun_info() {
    ...
    memcpy(array_id, inqp->array_unique_id, inqp->array_uniq_id_len);
    ...
}

The destination buffer is a 16-byte fixed-size stack array declared in
rdac_bus_attach():

rdac_bus_attach() {
    char array_id[UNIQUE_ID_LEN];
    ...
}

Since array_uniq_id_len is provided by the device (potentially up to 255),
could an untrusted or misconfigured device provide an oversized length and
overflow the stack frame?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713063640.3081603-1-demonaxsh@gmail.com?part=1