Re: [PATCH 1/2] scsi: mpi3mr: Fix NULL pointer dereference in mpi3mr_sas_port_add()

On Wed, Aug 12, 2026 at 10:43:34AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing NULL check for the return values of sas_end_device_alloc() and sas_expander_alloc().
> - [High] Ignored return value of sas_rphy_add() leads to a resource leak and invalid system state.
Thanks for the automated review. 

The flagged rphy NULL check and sas_rphy_add() error handling are
pre-existing issues in mpi3mr_sas_port_add(). 

I will keep this 2-patch series focused on fixing the sas_port allocation/free 
bugs and target device refcount leak, and I will look into addressing the 
rphy error handling in a separate follow-up patch.

Thanks,
Milan
> --
> 
> --- Patch [1]: [PATCH 1/2] scsi: mpi3mr: Fix NULL pointer dereference in mpi3mr_sas_port_add() ---
> commit 52d7de23ae9ec8ed8dec5b684daecf405f00a60e
> Author: Milan P. Gandhi <[email protected]>
> 
> scsi: mpi3mr: Fix NULL pointer dereference in mpi3mr_sas_port_add()
> 
> sas_port_alloc_num() can return NULL on memory allocation failure, which
> causes a NULL pointer dereference when passed to sas_port_add().
> Additionally, if sas_port_add() fails, the allocated port is leaked.
> This patch adds a NULL check and calls sas_port_free() on failure.
> 
> > diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> > index 240f67a8e2e3b..ea2c04384a0e1 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()!\n",
> > +		    __FILE__, __LINE__, __func__);
> > +		goto out_fail;
> > +	}
> >  	if ((sas_port_add(port))) {
> >  		ioc_err(mrioc, "failure at %s:%d/%s()!\n",
> >  		    __FILE__, __LINE__, __func__);
> > +		sas_port_free(port);
> >  		goto out_fail;
> >  	}
> >  
> 
> Further down in 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;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this code lead to a NULL
> pointer dereference if memory allocation fails? If sas_end_device_alloc() or
> sas_expander_alloc() returns NULL, it appears we dereference rphy directly on
> the following line without checking it.
> 
> 	if ((sas_rphy_add(rphy))) {
> 		ioc_err(mrioc, "failure at %s:%d/%s()!\n",
> 		    __FILE__, __LINE__, __func__);
> 	}
> 
> [Severity: High]
> This is a pre-existing issue, but does ignoring the return value of
> sas_rphy_add() here lead to resource leaks and invalid system state?
> 
> The failure is only logged, and execution continues without aborting or
> cleaning up the rphy. If we leave an unregistered device in the state
> structures, could it cause an oops during teardown?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
>