[PATCH] scsi: mpi3mr: Fix tgtdev reference count in mpi3mr_pcietopochg_evt_bh()

[PATCH] scsi: mpi3mr: Fix tgtdev reference count in mpi3mr_pcietopochg_evt_bh()

[Wentao Liang]

mpi3mr_get_tgtdev_by_handle() takes a reference on the target device
and the loop drops it with mpi3mr_tgtdev_put() at the end of every
iteration. The MPI3_EVENT_PCIE_TOPO_PS_NOT_RESPONDING branch drops the
same reference a second time before breaking out, so the reference is
put twice and the kref can be released prematurely.

Drop the redundant put and let the loop release the reference once.

Fixes: 8e653455547a ("scsi: mpi3mr: Add support for PCIe device event handling")
Cc: stable@vger.kernel.org
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/scsi/mpi3mr/mpi3mr_os.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
index 8a2575171e1f..0c974f2627d1 100644
--- a/drivers/scsi/mpi3mr/mpi3mr_os.c
+++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
@@ -2027,7 +2027,6 @@ static void mpi3mr_pcietopochg_evt_bh(struct mpi3mr_ioc *mrioc,
 			if (tgtdev->host_exposed)
 				mpi3mr_remove_tgtdev_from_host(mrioc, tgtdev);
 			mpi3mr_tgtdev_del_from_list(mrioc, tgtdev, false);
-			mpi3mr_tgtdev_put(tgtdev);
 			break;
 		default:
 			break;
-- 
2.34.1

Re: [PATCH] scsi: mpi3mr: Fix tgtdev reference count in mpi3mr_pcietopochg_evt_bh()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: mpi3mr: memory leak of tgtdev on PCIe removal due to removed kref put
- [High] scsi: mpi3mr: incomplete fix leaves double-put UAF vulnerability in SAS and status handlers
--

commit a438077929efbcec320166774a755489ffa128b2
Author: Wentao Liang <vulab@iscas.ac.cn>

scsi: mpi3mr: Fix tgtdev reference count in mpi3mr_pcietopochg_evt_bh()

This patch drops an extra put in the PCIe topology change event handler
that previously dropped the reference twice. This is intended to fix a
premature release of the target device kref.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index 991e29a8d382f..d6414d0186183 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -2053,7 +2053,6 @@ static void mpi3mr_pcietopochg_evt_bh(struct mpi3mr_ioc *mrioc,
>  			if (tgtdev->host_exposed)
>  				mpi3mr_remove_tgtdev_from_host(mrioc, tgtdev);
>  			mpi3mr_tgtdev_del_from_list(mrioc, tgtdev, false);
> -			mpi3mr_tgtdev_put(tgtdev);

[Severity: High]
Does removing this explicit put leak the tgtdev structure?

The device structure tgtdev is initialized with a refcount of 1 upon
allocation, and a second reference is taken when it is added to the internal
list. In mpi3mr_pcietopochg_evt_bh(), mpi3mr_tgtdev_del_from_list() drops the
list reference.

By removing this explicit put, doesn't the object end up with a stranded
refcount of 1 after the event handler drops its loop-local handle reference
at the end of the iteration? This appears to result in a permanent memory
leak on standard hardware hotplug paths.

>  			break;
>  		default:
>  			break;

[Severity: High]
Does this fix leave the same double-put vulnerability in the SAS and status
handlers?

The patch aims to fix a premature reference release by removing the explicit
put in the PCIe topology handler. However, looking at the source, the exact
same reference-dropping sequence remains in mpi3mr_sastopochg_evt_bh() and
mpi3mr_devstatuschg_evt_bh(). For example:

mpi3mr_devstatuschg_evt_bh() {
    ...
    if (cleanup) {
        mpi3mr_tgtdev_del_from_list(mrioc, tgtdev, false);
        mpi3mr_tgtdev_put(tgtdev);
    }
    ...
}

If a device fails to be removed from the list (e.g., state is not
MPI3MR_DEV_REMOVE_HS_STARTED), these handlers unconditionally drop the base
reference while the object remains in the list.

Later, when the device is actually removed from the list (such as during
mpi3mr_refresh_tgtdevs()), wouldn't the refcount drop to 0 prematurely,
causing a use-after-free during the subsequent put? Should these other
equivalent handlers be updated to properly address this sequence as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917150301.2158080-1-vulab@iscas.ac.cn?part=1