Re: [PATCH v6 50/56] scsi: qla2xxx: Hold vport reference in qla24xx_report_id_acquisition()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Missing memory barrier before `atomic_dec(&vp->vref_count)` allows compiler/CPU reordering, reopening the use-after-free window.

Pre-existing issues:
- [High] Missing lock serialization on `host_map` btree in `qla_update_host_map()`.
- [High] Unchecked VPORT_DELETE state in `qla24xx_report_id_acquisition()` allows a dying vport to be re-inserted into the host map btree.
- [High] Unprotected list traversal and access of `vp_fcports` in `qla24xx_report_id_acquisition` (Format 2 path) can lead to Use-After-Free.
- [High] Pre-existing `list_for_each_entry_safe` iteration across lock drops allows the cached next element to be freed.
--

--- Patch [50]: [PATCH v6 50/56] scsi: qla2xxx: Hold vport reference in qla24xx_report_id_acquisition() ---
commit 63a71c77c454d2559780ee0faf9f716b8a83c5f1
Author: Nilesh Javali <njavali@marvell.com>
Subject: scsi: qla2xxx: Hold vport reference in qla24xx_report_id_acquisition()

This commit fixes a use-after-free window in the format 1 path by
taking a vport reference under the vport_slock during id acquisition.
The lock is held while searching the list, and the reference ensures
the vport remains valid during subsequent lockless setup.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index 59023492c5a98..ba4a4764de1fb 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -4267,6 +4267,7 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
>  			list_for_each_entry(vp, &ha->vp_list, list) {
>  				if (vp_idx == vp->vp_idx) {
>  					found = 1;
> +					atomic_inc(&vp->vref_count);

[Severity: High]
This is a pre-existing issue, but does this loop need to check whether
the vport is actively being deleted?

If a concurrent thread removes the vport from the host map and sets the
deletion state, could this loop erroneously take a reference on the dying
vport and re-insert it into the host map below, creating a dangling pointer
once the reference is dropped?

>  					break;
>  				}
>  			}
> @@ -4284,6 +4285,8 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
>  			spin_unlock_irqrestore(&ha->vport_slock, flags);
>  
>  			if (!found)
>  				return;
>  
>  			qla_update_host_map(vp, id);

[Severity: High]
This isn't a bug introduced by this patch, but does dropping the vport_slock
leave qla_update_host_map() unprotected?

The btree functions are used locklessly in this operation. If another thread
modifies the host map concurrently, can this result in data races and btree
corruption?

>  
>  			/*
>  			 * Cannot configure here as we are still sitting on the
>  			 * response queue. Handle it in dpc context.
>  			 */
>  			set_bit(VP_IDX_ACQUIRED, &vp->vp_flags);
>  			set_bit(REGISTER_FC4_NEEDED, &vp->dpc_flags);
>  			set_bit(REGISTER_FDMI_NEEDED, &vp->dpc_flags);
> +
> +			atomic_dec(&vp->vref_count);

[Severity: High]
Can this regression allow the compiler or CPU to reorder the set_bit()
operations?

Since void-returning atomic operations like atomic_dec() and set_bit()
do not imply memory barriers, could the CPU reorder the memory stores to
become visible after the reference count is decremented? 

If a concurrent thread polls the reference count, observes it drop to zero,
unlinks, and frees the vport, would the delayed set_bit() stores corrupt the
freed memory?

>  		}
>  		set_bit(VP_DPC_NEEDED, &vha->dpc_flags);

[ ... ]


[Severity: High]
This is a pre-existing issue, but is the vp_fcports list traversed safely
in the Format 2 path of qla24xx_report_id_acquisition()?

drivers/scsi/qla2xxx/qla_mbx.c:qla24xx_report_id_acquisition() {
    ...
	} else if (rptid_entry->format == 2) {
        ...
		list_for_each_entry(fcport, &vha->vp_fcports, list) {
			fcport->scan_state = QLA_FCPORT_SCAN;
			fcport->n2n_flag = 0;
		}
    ...
}

Unlike the Format 1 path, this traversal appears to happen without holding
the session lock. If a concurrent session deletion unlinks and frees the
fcport elements during this loop, could this lead to list corruption and a
use-after-free?


[Severity: High]
This is also a pre-existing issue, but does list_for_each_entry_safe()
across a lock drop open a window for the cached next element to be freed?

drivers/scsi/qla2xxx/qla_mid.c:qla2x00_do_dpc_all_vps() {
    ...
	list_for_each_entry_safe(vp, tvp, &ha->vp_list, list) {
		if (vp->vp_idx) {
			atomic_inc(&vp->vref_count);
			spin_unlock_irqrestore(&ha->vport_slock, flags);

			qla2x00_do_dpc_vp(vp);

			spin_lock_irqsave(&ha->vport_slock, flags);
			atomic_dec(&vp->vref_count);
		}
	}
    ...
}

Because the macro caches the next element (tvp) before dropping the lock,
could a concurrent removal free tvp while the lock is dropped, causing a
use-after-free when the loop advances?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723050413.3897522-1-njavali@marvell.com?part=50