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

[sashiko-bot@kernel.org]

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