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

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.
--

--- 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