Re: [PATCH v1 09/10] mpi3mr: Fix SAS PHY cleanup in host addition error paths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unhandled Error Path / NULL Pointer Dereference when adding host PHYs.
--

commit 5d2df761c30d63d86fc3554a56dabf8408d62592
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix SAS PHY cleanup in host addition error paths

When adding a SAS host, the driver allocates a PHY array and subsequently
creates individual SAS PHYs. If a later step fails, the error path exits
without cleaning up previously allocated resources, resulting in leaks of
both the PHY array and any registered SAS PHYs.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index 1b793d86f7589..0236bbfcff6d9 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
[ ... ]
> @@ -1264,7 +1265,7 @@ void mpi3mr_sas_host_add(struct mpi3mr_ioc *mrioc)
>  
>  		if (!(mpi3mr_get_hba_port_by_id(mrioc, port_id)))
>  			if (!mpi3mr_alloc_hba_port(mrioc, port_id))
> -				goto out;
> +				goto out_free_phy;
>  
>  		mrioc->sas_hba.phy[i].handle = mrioc->sas_hba.handle;
>  		mrioc->sas_hba.phy[i].phy_id = i;

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to ignore the return
value of mpi3mr_add_host_phy() which is called just after this block?

If mpi3mr_add_host_phy() fails (e.g., at sas_phy_add()), it returns -1. At
this point, mr_sas_phy->remote_identify.sas_address is already populated with
the attached device's SAS address, but mr_sas_phy->phy is left as NULL.

Later, when the attached device triggers a device addition event,
mpi3mr_sas_port_add() successfully matches the failed PHY because the
remote_identify.sas_address is populated:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
		if ((mr_sas_node->phy[i].remote_identify.sas_address !=
		    mr_sas_port->remote_identify.sas_address) ||
		    (mr_sas_node->phy[i].hba_port != hba_port))
			continue;
    ...
}

It then unconditionally passes the NULL mr_sas_phy->phy to sas_port_add_phy(),
which dereferences it without checking, causing a kernel panic:

drivers/scsi/mpi3mr/mpi3mr_transport.c:mpi3mr_sas_port_add() {
    ...
		sas_port_add_phy(port, mr_sas_phy->phy);
    ...
}

Should mpi3mr_sas_host_add() handle the failure of mpi3mr_add_host_phy() and
jump to the new out_free_phy label?

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