Re: [PATCH v2 55/60] scsi: qla2xxx: Hold vport reference in qla24xx_report_id_acquisition()

[sashiko-bot@kernel.org]

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

New issues:
- [High] The decrement of `vp->vref_count` lacks a release memory barrier and breaks the driver's locking idiom, potentially leading to a use-after-free write on weakly ordered architectures.

Pre-existing issues:
- [High] The exact same use-after-free vulnerability fixed by this patch in `qla24xx_report_id_acquisition` is present in the NVMe FC path via `qla2xxx_get_vha_from_vp_idx`.
- [High] Target mode processing uses `qla_find_host_by_vp_idx` which returns an unreferenced `vha` pointer, opening a race condition for UAF during vport teardown.
- [High] btree operations on `ha->host_map` via `qla_update_host_map()` are performed without holding `vport_slock`.
- [High] Missing `sess_lock` during `vp_fcports` list traversal in Format 2 branch.
- [High] Invalid use of list_for_each_entry_safe across dropped spinlocks in DPC and alert handlers.
- [High] `qla24xx_report_id_acquisition` ignores `VPORT_DELETE` and can resurrect a deleted vport.
--

commit 95ed94124983f94c25ebc87bace3b12cf63aa94f
Author: Nilesh Javali <njavali@marvell.com>
scsi: qla2xxx: Hold vport reference in qla24xx_report_id_acquisition()

This commit fixes a use-after-free by taking a vport reference
(vref_count) while holding vport_slock when finding a virtual port,
and dropping it after use. This ensures the vport isn't freed by a
concurrent qla24xx_deallocate_vp_id().

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index 8a001b489fc0e..bfb931eb14f61 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -4275,6 +4275,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 if the vport
is actively being deleted?

If qla24xx_vport_delete() is concurrently removing the vport and has already
taken it out of the host_map, would we resurrect it here?

We take a reference and later re-register it without checking for
VPORT_DELETE on vp->dpc_flags, which might lead to a use-after-free when the
system later queries the map.

>  					break;
>  				}
>  			}
> @@ -4292,6 +4293,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]
Could the weakly ordered nature of atomic_dec() allow the preceding set_bit()
operations to be reordered after the decrement?

In the kernel, set_bit() and atomic_dec() do not imply memory barriers.
On weakly ordered architectures, the atomic_dec() could become visible
before the stores to vp->vp_flags and vp->dpc_flags.

If qla24xx_deallocate_vp_id() polls vref_count concurrently, could it
observe vref_count == 0 early, proceeding to unlink the vport before the
flags are fully updated?

Would it be safer to enclose the atomic_dec() within vport_slock, matching
how the reference is acquired?

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

[Severity: High]
This is a pre-existing issue, but are we modifying the host_map without
holding the required lock in this same function?

Just before the snippet above, qla_update_host_map() evaluates to
qla_update_vp_map() which inserts the vp into the ha->host_map btree. The
documentation for qla_update_vp_map() states it requires vport_slock:

drivers/scsi/qla2xxx/qla_mid.c:qla_update_vp_map() {
    /* vport_slock to be held by the caller */
    ...
    btree_insert32(&ha->host_map, ...);
}

Since we dropped ha->vport_slock before this call, could concurrent
modifications corrupt the btree?


[Severity: High]
This isn't a bug introduced by this patch, but should we be holding
sess_lock while iterating vp_fcports in the Format 2 path of
qla24xx_report_id_acquisition()?

In the Format 1 path, the list is traversed with sess_lock held.
However, in the Format 2 path, we traverse the list and call
qla2x00_find_fcport_by_wwpn() without holding vha->hw->tgt.sess_lock:

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

    fcport = qla2x00_find_fcport_by_wwpn(vha,
        rptid_entry->u.f2.port_name, 1);
    ...
}

Could this lack of locking lead to list corruption or use-after-free if
vp_fcports is modified concurrently?


[Severity: High]
This is a pre-existing issue, but does the NVMe FC path suffer from a similar
use-after-free vulnerability as the one fixed in this patch?

When calling qla2xxx_get_vha_from_vp_idx() in qla_nvme.c, the lock is dropped
and the vha pointer is returned without incrementing vref_count:

drivers/scsi/qla2xxx/qla_nvme.c:qla2xxx_get_vha_from_vp_idx() {
    if (vha->vp_idx == vp_index) {
        spin_unlock_irqrestore(&ha->vport_slock, flags);
        return vha;
    }
}

The caller, qla2xxx_process_purls_iocb(), then heavily uses this unreferenced
pointer. Could a concurrent qla24xx_deallocate_vp_id() tear down the vport
and free it, causing a use-after-free in the NVMe response path?


[Severity: High]
This is a pre-existing issue, but does qla_find_host_by_vp_idx() also leak
unreferenced pointers in target mode?

drivers/scsi/qla2xxx/qla_mid.c:qla_find_host_by_vp_idx() {
    if (likely(test_bit(vp_idx, ha->vp_idx_map)))
        return ha->vp_map[vp_idx].vha;
}

Various target mode functions route commands to the vport returned by this
lookup. Since no reference is taken, couldn't qla24xx_deallocate_vp_id()
free the vha concurrently?


[Severity: High]
This is a pre-existing issue, but is it safe to use list_for_each_entry_safe()
when dropping the spinlock in DPC and alert handlers?

In qla2x00_do_dpc_all_vps(), the lock is dropped inside the safe iteration
loop:

drivers/scsi/qla2xxx/qla_mid.c:qla2x00_do_dpc_all_vps() {
    spin_lock_irqsave(&ha->vport_slock, flags);
    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);
            ...
            spin_lock_irqsave(&ha->vport_slock, flags);
}

Since list_for_each_entry_safe() caches the next element (tvp), if a
concurrent deletion removes tvp from the list and frees it while the lock
is dropped, won't resuming the loop result in a use-after-free?
A similar pattern exists in qla2x00_alert_all_vps().

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