Re: [PATCH v3 3/3] scsi: libsas: Handle errors in sas_ex_discover_expander()

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

Pre-existing issues:
- [High] Missing check for `sas_expander_alloc()` failure leads to an invalid pointer dereference and kernel panic.
- [High] Ignoring the return value of `sas_rphy_add()` leads to a kernel panic upon teardown.
--

--- Patch [3]: [PATCH v3 3/3] scsi: libsas: Handle errors in  sas_ex_discover_expander() ---
commit ae2723570a72c0c97de94806125a00a3b79e17cd
Author: Eshaan Deshmukh <[email protected]>

scsi: libsas: Handle errors in sas_ex_discover_expander()
    
This commit improves error handling in sas_ex_discover_expander() by checking
the return values of sas_port_alloc() and sas_port_add(). If either function
fails, it cleans up the child device and port allocations before returning.

> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index ab6afbad37feb..a83493f574c30 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -911,7 +911,6 @@ static struct domain_device *sas_ex_discover_expander(
>  	struct sas_rphy *rphy;
>  	struct sas_expander_device *edev;
>  	struct asd_sas_port *port;
> -	int res;
>  
>  	if (phy->routing_attr == DIRECT_ROUTING) {
>  		pr_warn("ex %016llx:%02d:D <--> ex %016llx:0x%x is not allowed\n",
> @@ -925,9 +924,13 @@ static struct domain_device *sas_ex_discover_expander(
>  		return NULL;
>  
>  	phy->port = sas_port_alloc(&parent->rphy->dev, phy_id);
> -	/* FIXME: better error handling */
> -	BUG_ON(sas_port_add(phy->port) != 0);
> +	if (!phy->port) {
> +		goto out_put_device;
> +	}
>  
> +	if (sas_port_add(phy->port)) {
> +		goto out_free_port;
> +	}
>  
>  	switch (phy->attached_dev_type) {
>  	case SAS_EDGE_EXPANDER_DEVICE:

[Severity: High]
This is a pre-existing issue, but can this lead to an invalid pointer
dereference?

If sas_expander_alloc() fails under memory pressure and returns NULL, it is
assigned to rphy in the switch statement below:

sas_ex_discover_expander() {
    ...
    case SAS_EDGE_EXPANDER_DEVICE:
        rphy = sas_expander_alloc(phy->port, SAS_EDGE_EXPANDER_DEVICE);
    ...
}

The code later blindly offsets and dereferences this:

sas_ex_discover_expander() {
    ...
    edev = rphy_to_expander_device(rphy);
    ...
    edev->level = parent_ex->level + 1;
    ...
}

Since rphy is not at offset 0 within struct sas_expander_device,
container_of creates an invalid pointer for edev. Dereferencing it
causes a kernel panic. Should we check if rphy is NULL before continuing?

[ ... ]

> @@ -966,19 +969,29 @@ static struct domain_device *sas_ex_discover_expander(
>  	list_add_tail(&child->dev_list_node, &parent->port->dev_list);
>  	spin_unlock_irq(&parent->port->dev_list_lock);
>  
> -	res = sas_discover_expander(child);
> -	if (res) {
> -		sas_rphy_delete(rphy);
> -		spin_lock_irq(&parent->port->dev_list_lock);
> -		list_del(&child->dev_list_node);
> -		spin_unlock_irq(&parent->port->dev_list_lock);
> -		sas_put_device(child);
> -		sas_port_delete(phy->port);
> -		phy->port = NULL;
> -		return NULL;
> -	}
> +	if (sas_discover_expander(child))
> +		goto out_delete_rphy;
> +

[Severity: High]
This is a pre-existing issue, but can ignoring the return value of
sas_rphy_add() cause a crash during teardown?

Earlier in this function, sas_rphy_add(rphy) is called without checking if it
was successful. If it fails, device_add() cleans up and sets dev->p to NULL.

If a subsequent function like sas_discover_expander(child) fails here, the
error path goes to out_delete_rphy:

sas_ex_discover_expander() {
    ...
    out_delete_rphy:
        sas_rphy_delete(rphy);
    ...
}

This path unconditionally calls device_del() inside sas_rphy_delete().
Calling device_del() on a device that was never successfully added will
dereference the NULL dev->p, causing a panic. Does sas_rphy_add() need
error handling?

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