Re: [PATCH v1 08/10] mpi3mr: Fix SAS port allocation and registration error handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The reference to the target device (`tgtdev`) is leaked and the `pend_sas_rphy_add` state remains inconsistent on error paths.
- [High] Missing NULL pointer check after `rphy` allocation leads to a guaranteed NULL pointer dereference on allocation failure.
- [High] Failure of `sas_rphy_add()` is ignored, leaving the driver and device model in an invalid state.
--

commit 2228b7db89a5c405d93e9a1b01afe8d02a73da5c
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix SAS port allocation and registration error handling

During SAS port creation, the driver does not verify successful port
allocation before attempting registration, which can lead to a NULL
pointer dereference. Additionally, if registration fails, the allocated
port is not freed, resulting in a memory leak.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index 240f67a8e2e3b..1b793d86f7589 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> @@ -1428,9 +1428,15 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
>  	}
>  
>  	port = sas_port_alloc_num(mr_sas_node->parent_dev);
> +	if (!port) {
> +		ioc_err(mrioc, "failure at %s:%d/%s() (sas_port_alloc)!\n",
> +		    __FILE__, __LINE__, __func__);
> +		goto out_fail;

[Severity: High]
This is a pre-existing issue, but does jumping to out_fail here leak the
tgtdev reference?

If mpi3mr_sas_port_add() is called for a SAS_END_DEVICE, it acquires a
reference to tgtdev earlier in the function:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
    if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
        tgtdev = mpi3mr_get_tgtdev_by_addr(mrioc,
            mr_sas_port->remote_identify.sas_address,
            mr_sas_port->hba_port);
    ...
}

When sas_port_alloc_num() fails and the code jumps to out_fail, it frees
mr_sas_port but misses the necessary mpi3mr_tgtdev_put(tgtdev) cleanup:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
 out_fail:
    list_for_each_entry_safe(mr_sas_phy, next, &mr_sas_port->phy_list,
        port_siblings)
        list_del(&mr_sas_phy->port_siblings);
    kfree(mr_sas_port);
    return NULL;
}

Will this lead to a resource leak of the tgtdev structure that prevents
device removal?

> +	}
>  	if ((sas_port_add(port))) {
>  		ioc_err(mrioc, "failure at %s:%d/%s()!\n",
>  		    __FILE__, __LINE__, __func__);
> +		sas_port_free(port);
>  		goto out_fail;

[Severity: High]
This is a pre-existing issue, but this path also appears to leak the tgtdev
reference in the same way as described above when sas_port_add() fails.

>  	}

[Severity: High]
This is a pre-existing issue, but can this function unconditionally
dereference rphy if the allocation fails?

Further down in mpi3mr_sas_port_add(), rphy is allocated but immediately
dereferenced without a check:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
    if (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
        rphy = sas_end_device_alloc(port);
        tgtdev->dev_spec.sas_sata_inf.rphy = rphy;
    } else {
        rphy = sas_expander_alloc(port,
            mr_sas_port->remote_identify.device_type);
    }
    rphy->identify = mr_sas_port->remote_identify;
    ...
}

If sas_end_device_alloc() or sas_expander_alloc() return NULL under memory
pressure, wouldn't the assignment to rphy->identify cause a kernel panic?

[Severity: High]
This is a pre-existing issue, but is the driver left in an invalid state if
sas_rphy_add() fails?

Later in the same function, the return value of sas_rphy_add() is caught
but ignored:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
    if ((sas_rphy_add(rphy))) {
        ioc_err(mrioc, "failure at %s:%d/%s()!\n",
            __FILE__, __LINE__, __func__);
    }
    ...
}

If sas_rphy_add() fails, does the driver leak the rphy memory since
sas_port_delete() won't free it if it wasn't successfully added?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=8