Re: [PATCH v8 1/2] scsi: libsas: refactor sas_ex_to_ata() using new helper sas_ex_to_dev()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch adds a defensive NULL check for `ex_dev` to prevent a crash when callers pass a NULL device, but this merely shifts the crash to the fallback path in its main caller `transport_sas_phy_reset`.

Pre-existing issues:
- [High] `sas_find_dev_by_rphy()` returns an un-refcounted `domain_device` pointer, leading to a Use-After-Free if the device is concurrently removed.
- [High] Lockless and unprotected access to `ex_phy->port` causes a TOCTOU race, leading to NULL pointer dereference.
--

commit 7daa35d1e6df78b76bca7d9ad2296ce81f71c4d9
Author: Xingui Yang <yangxingui@huawei.com>

scsi: libsas: refactor sas_ex_to_ata() using new helper sas_ex_to_dev()

Introduce sas_ex_to_dev() to return any device type attached to an
expander phy. The new helper is then used by sas_ex_to_ata() to reduce
code duplication.

Also add a defensive NULL check for ex_dev to guard against callers
passing a NULL device.

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index f471ab464a78b..cb9d3b7482225 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -345,13 +345,15 @@ static void sas_set_ex_phy(struct domain_device *dev, int phy_id,
>  		 SAS_ADDR(phy->attached_sas_addr), type);
>  }
>  
> -/* check if we have an existing attached ata device on this expander phy */
> -struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id)
> +struct domain_device *sas_ex_to_dev(struct domain_device *ex_dev, int phy_id)
>  {
> -	struct ex_phy *ex_phy = &ex_dev->ex_dev.ex_phy[phy_id];
> -	struct domain_device *dev;
> +	struct ex_phy *ex_phy;
>  	struct sas_rphy *rphy;
>  
> +	if (!ex_dev)
> +		return NULL;
> +

[Severity: High]
Does this new check shift a crash into the fallback path of callers instead
of preventing it?

If a concurrent removal causes ex_dev to be NULL, this check allows
transport_sas_phy_reset() to receive NULL instead of crashing here. However,
transport_sas_phy_reset() then falls back to calling sas_smp_phy_control():

drivers/scsi/libsas/sas_init.c:transport_sas_phy_reset() {
    ...
    if (ata_dev && !hard_reset) {
        ...
    } else {
        return sas_smp_phy_control(ddev, phy->number, reset_type, NULL);
    }
    ...
}

Because ddev is still NULL, it is passed into sas_smp_phy_control(), which
calls smp_execute_task_sg(), eventually causing a crash when dereferencing
dev->port:

drivers/scsi/libsas/sas_expander.c:smp_execute_task_sg() {
    ...
    to_sas_internal(dev->port->ha->shost->transportt);
    ...
}

Is it safe to return NULL here without ensuring callers like
transport_sas_phy_reset() can handle it properly?

> +	ex_phy = &ex_dev->ex_dev.ex_phy[phy_id];
>  	if (!ex_phy->port)
>  		return NULL;

[Severity: High]
This is a pre-existing issue, but does this sequence have a time-of-check
to time-of-use race condition?

The code checks if ex_phy->port is NULL and then dereferences it on the
next line (rphy = ex_phy->port->rphy). If a concurrent thread like
sas_port_delete_phy() sets phy->port to NULL between these two lines, could
this cause a NULL pointer dereference?

> @@ -359,7 +361,13 @@ struct domain_device *sas_ex_to_ata(struct domain_device *ex_dev, int phy_id)
>  	if (!rphy)
>  		return NULL;
>  
> -	dev = sas_find_dev_by_rphy(rphy);
> +	return sas_find_dev_by_rphy(rphy);

[Severity: High]
This isn't a bug introduced by this patch, but does sas_find_dev_by_rphy()
return an un-refcounted pointer that could be freed while in use?

Looking at sas_find_dev_by_rphy(), it locates the device under a lock but
returns it without taking a reference:

drivers/scsi/libsas/sas_scsi_host.c:sas_find_dev_by_rphy() {
    ...
    spin_lock(&port->dev_list_lock);
    list_for_each_entry(dev, &port->dev_list, dev_list_node) {
        if (rphy == dev->rphy) {
            found_dev = dev;
            spin_unlock(&port->dev_list_lock);
            goto found;
        }
    ...
    return found_dev;
}

If the device is concurrently unregistered and freed (e.g., via
sas_free_device()), does this pointer become dangling before the caller can
safely use it?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623024304.714582-1-yangxingui@huawei.com?part=1