Re: [PATCH v9 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:

Pre-existing issues:
- [High] Incomplete defensive NULL check in `sas_ex_to_dev()` defers a kernel panic to `transport_sas_phy_reset()`.
- [High] Race condition in `sas_ex_to_dev()` due to unlocked access to `ex_phy->port`.
- [High] Returning an un-refcounted `domain_device` pointer exposes callers to a Use-After-Free.
--

commit cd70af1a5d33d82f2c6199f5fc26b59a70b7d45c
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..fc6d8f3c9dcae 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -345,13 +345,16 @@ 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)
> +/* return the domain device attached to an expander phy */
> +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]
This is a pre-existing issue, but does adding this defensive NULL check here
fail to protect the caller from the same NULL pointer?

When sas_ex_to_ata() safely returns NULL because ex_dev is NULL, could the
caller logic fall through and crash by deferring the NULL pointer dereference?

For example, in drivers/scsi/libsas/sas_init.c:transport_sas_phy_reset():

    ddev = sas_ex_to_ata(ex_dev, phy->number);
    if (ddev) {
        ...
    } else {
        return sas_smp_phy_control(ddev, phy->number, reset_type, NULL);
    }

Would this result in a kernel panic inside sas_smp_phy_control() if ddev
is NULL (e.g., due to concurrent device removal)?

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

[Severity: High]
This is a pre-existing issue, but is there a race condition here due to
unlocked access to ex_phy->port?

Device teardown paths can call sas_port_delete(phy->port) and set
phy->port to NULL concurrently. Since we access ex_phy->port locklessly
and without READ_ONCE(), could the port be freed between the check and
the dereference of ex_phy->port->rphy, leading to a panic or use-after-free?

>  	if (!rphy)
>  		return NULL;
>  
> -	dev = sas_find_dev_by_rphy(rphy);
> +	return sas_find_dev_by_rphy(rphy);

[Severity: High]
This is a pre-existing issue, but does returning the domain_device pointer
without incrementing its reference count expose callers to a use-after-free?

Looking at drivers/scsi/libsas/sas_scsi_host.c:sas_find_dev_by_rphy():

    spin_lock_irqsave(&ha->phy_port_lock, flags);
    ...
                found_dev = dev;
    ...
    spin_unlock_irqrestore(&ha->phy_port_lock, flags);
    return found_dev;

If a device is concurrently unregistered, sas_put_device() can drop its
kref to 0 and call kfree(dev). When sas_ex_to_dev() passes this un-refcounted
pointer up to callers, won't they read freed memory when attempting to
interact with the device?

> +}

[ ... ]

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