Re: [PATCH v4 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 3 potential issue(s) to consider:

New issues:
- [High] Taking a reference on a vport that is in the process of being deleted leads to a deferred Use-After-Free because the doomed vport gets re-inserted into the global `host_map` btree.
- [High] The reference counting mechanism in `qla24xx_deallocate_vp_id` has a 10-second timeout that forcefully unlinks and frees the vport, which leads to a direct Use-After-Free in delayed reference holders.

Pre-existing issues:
- [Critical] Dropping `vport_slock` inside `list_for_each_entry_safe` loops leaves the cached next entry (`tvp`) unprotected, leading to a poisoned pointer dereference if the next entry is concurrently deleted.
--

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

scsi: qla2xxx: Hold vport reference in qla24xx_report_id_acquisition()

This commit addresses a use-after-free issue in the format 1 path. It takes
a vport reference under vport_slock during vp_list traversal to prevent
concurrent teardown.

> 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);
>  					break;
>  				}
>  			}

[Severity: High]
Since there is no check for a deletion state (such as the VPORT_DELETE flag)
before taking this reference, could we end up rescuing a vport that is
already in the process of being deleted?

If a concurrent qla24xx_vport_delete() clears vha->d_id and removes the
vport from the host_map, wouldn't the subsequent call to
qla_update_host_map() re-insert this dying vport back into the btree?

If so, does this result in a dangling pointer when the vport is finally
freed shortly after?

[ ... ]

> @@ -4284,6 +4285,8 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
>  			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]
Does the 10-second timeout mechanism in qla24xx_deallocate_vp_id() make this
decrement vulnerable to a use-after-free?

If qla24xx_deallocate_vp_id() polls for vref_count to reach 0 but times out,
it forcefully unlinks and frees the vport. If this thread is delayed for
longer than 10 seconds (e.g., due to system load or VM scheduling) after
taking the reference, won't this atomic_dec() and the set_bit() operations
above it end up modifying freed memory?

[Severity: Critical]
This is a pre-existing issue, but the commit message mentions matching the
reference idiom already used by other ha->vp_list traversals. Are those
other traversals safe from poisoned pointer dereferences?

Looking at qla2x00_do_dpc_all_vps() in drivers/scsi/qla2xxx/qla_mid.c:

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

Because vport_slock is dropped inside the list_for_each_entry_safe loop, the
cached next entry (tvp) is left unprotected.

If a concurrent operation like qla24xx_deallocate_vp_id() hits its timeout
or sees a 0 refcount on tvp and deletes it, its next and prev pointers
become poisoned.

When the loop resumes and tries to advance, won't it dereference LIST_POISON1?
This same pattern appears to exist in qla2x00_abort_isp() and
qla_pci_error_cleanup().

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