[PATCH 0/2] scsi: mpi3mr: Fix error handling and resource leak in mpi3mr_sas_port_add()

[PATCH 0/2] scsi: mpi3mr: Fix error handling and resource leak in mpi3mr_sas_port_add()

[Milan P. Gandhi]

This series fixes resource leaks and potential crashes in the
mpi3mr_sas_port_add() error path:

- Patch 1 adds a missing NULL check on sas_port_alloc_num() failure
  and ensures allocated ports are released via sas_port_free() if
  sas_port_add() fails.
- Patch 2 releases the target device reference acquired via
  mpi3mr_get_tgtdev_by_addr() if subsequent allocation errors occur.

Milan P. Gandhi (2):
  scsi: mpi3mr: Fix NULL pointer dereference in mpi3mr_sas_port_add()
  scsi: mpi3mr: Fix target device refcount leak in mpi3mr_sas_port_add()

 drivers/scsi/mpi3mr/mpi3mr_transport.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.55.0

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

[Milan P. Gandhi]

sas_port_alloc_num() can return NULL on memory allocation failure. The
return value is passed directly to sas_port_add() without a NULL check,
which causes a NULL pointer dereference.

Additionally, if sas_port_add() fails, the allocated port is not freed
before jumping to out_fail, leaking the sas_port structure. Call
sas_port_free() to properly release it.

Fixes: e22bae30667a ("scsi: mpi3mr: Add expander devices to STL")
Signed-off-by: Milan P. Gandhi <mgandhi@redhat.com>
---
 drivers/scsi/mpi3mr/mpi3mr_transport.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
index 240f67a8e2e3..ea2c04384a0e 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;
 	}
 
-- 
2.55.0

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

[sashiko-bot]

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 <mgandhi@redhat.com>

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/20260812103344.174247-1-mgandhi@redhat.com?part=1

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

[Milan P. Gandhi]

On Wed, Aug 12, 2026 at 10:43:34AM +0000, sashiko-bot@kernel.org 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 <mgandhi@redhat.com>
> 
> 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/20260812103344.174247-1-mgandhi@redhat.com?part=1
> 

[PATCH 2/2] scsi: mpi3mr: Fix target device refcount leak in mpi3mr_sas_port_add()

[Milan P. Gandhi]

mpi3mr_get_tgtdev_by_addr() increments the target device kref when it
returns a device. If a subsequent error triggers a goto out_fail after
the tgtdev reference is acquired, the reference is never released
because the out_fail path does not call mpi3mr_tgtdev_put(). This
prevents the target device structure from ever being freed.

Add a tgtdev put in the out_fail path, guarded by a NULL check since
tgtdev is only acquired for SAS_END_DEVICE types and the same cleanup
path is shared by earlier error cases where tgtdev is still NULL.

Fixes: e22bae30667a ("scsi: mpi3mr: Add expander devices to STL")
Signed-off-by: Milan P. Gandhi <mgandhi@redhat.com>
---
 drivers/scsi/mpi3mr/mpi3mr_transport.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
index ea2c04384a0e..232af978d737 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
@@ -1507,6 +1507,8 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
 	list_for_each_entry_safe(mr_sas_phy, next, &mr_sas_port->phy_list,
 	    port_siblings)
 		list_del(&mr_sas_phy->port_siblings);
+	if (tgtdev)
+		mpi3mr_tgtdev_put(tgtdev);
 	kfree(mr_sas_port);
 	return NULL;
 }
-- 
2.55.0

Re: [PATCH 2/2] scsi: mpi3mr: Fix target device refcount leak in mpi3mr_sas_port_add()

[sashiko-bot]

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 value of sas_end_device_alloc() and sas_expander_alloc() leads to an immediate NULL pointer dereference.
- [High] Failure of sas_rphy_add() is ignored and does not abort the operation, leaving the driver state inconsistent and leaking rphy and port structures.
--

--- Patch [2]: [PATCH 2/2] scsi: mpi3mr: Fix target device refcount leak in mpi3mr_sas_port_add() ---
commit 6d1c7833feb76d25951cbe5a46e2637e0f883658
Author: Milan P. Gandhi <mgandhi@redhat.com>

scsi: mpi3mr: Fix target device refcount leak in mpi3mr_sas_port_add()

This commit fixes a reference count leak for target devices in
mpi3mr_sas_port_add() when an error triggers the out_fail path. It adds
a NULL-checked mpi3mr_tgtdev_put() call to ensure the device reference
is properly released during cleanup.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_transport.c b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> index ea2c04384a0e1..232af978d7372 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_transport.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_transport.c
> @@ -1507,6 +1507,8 @@ static struct mpi3mr_sas_port *mpi3mr_sas_port_add(struct mpi3mr_ioc *mrioc,
>  	list_for_each_entry_safe(mr_sas_phy, next, &mr_sas_port->phy_list,
>  	    port_siblings)
>  		list_del(&mr_sas_phy->port_siblings);
> +	if (tgtdev)
> +		mpi3mr_tgtdev_put(tgtdev);
>  	kfree(mr_sas_port);
>  	return NULL;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the error
handling in this function, I noticed there is no check for the return value
of sas_end_device_alloc() or sas_expander_alloc() before dereferencing it:

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() returns NULL due to
memory constraints, will the assignment to rphy->identify cause a NULL
pointer dereference? Could we add a check and jump to the out_fail path
here?

[Severity: High]
This is also a pre-existing issue, but I noticed the failure of
sas_rphy_add() does not abort the operation:

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 (mr_sas_port->remote_identify.device_type == SAS_END_DEVICE) {
        tgtdev->dev_spec.sas_sata_inf.pend_sas_rphy_add = 0;
        tgtdev->dev_spec.sas_sata_inf.sas_transport_attached = 1;
        mpi3mr_tgtdev_put(tgtdev);
    }
    ...
}

If sas_rphy_add() fails, the code merely logs an error but proceeds to mark
the transport as attached (sas_transport_attached = 1). Does this leave the
driver state inconsistent and leak the rphy and port structures, since
teardown might attempt to remove a device that was never fully added?
Should this failure jump to an appropriate error handling label to unwind
the state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260812103344.174247-1-mgandhi@redhat.com?part=2

Re: [PATCH 0/2] scsi: mpi3mr: Fix error handling and resource leak in mpi3mr_sas_port_add()

[Laurence Oberman]

On Wed, 2026-08-12 at 16:03 +0530, Milan P. Gandhi wrote:
> This series fixes resource leaks and potential crashes in the
> mpi3mr_sas_port_add() error path:
> 
> - Patch 1 adds a missing NULL check on sas_port_alloc_num() failure
>   and ensures allocated ports are released via sas_port_free() if
>   sas_port_add() fails.
> - Patch 2 releases the target device reference acquired via
>   mpi3mr_get_tgtdev_by_addr() if subsequent allocation errors occur.
> 
> Milan P. Gandhi (2):
>   scsi: mpi3mr: Fix NULL pointer dereference in mpi3mr_sas_port_add()
>   scsi: mpi3mr: Fix target device refcount leak in
> mpi3mr_sas_port_add()
> 
>  drivers/scsi/mpi3mr/mpi3mr_transport.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
Given the Sashiko reports were pre-existing issues:
For this series it looks good.

Reviewed-by: Laurence Oberman <loberman@redhat.com>