Re: [PATCH v3 76/88] scsi: qla2xxx: Skip vport under deletion in report ID acquisition

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The patch attempts to fix a Use-After-Free (UAF) by checking `VPORT_DELETE` before incrementing `vref_count`, but introduces a Time-of-Check to Time-of-Use (TOCTOU) race condition that leaves the bug open.
- [Critical] Inconsistent locking across multiple threads protecting the `host_map` btree causes severe structural corruption.
- [High] Dropping a list lock inside `list_for_each_entry_safe` exposes the cached next element to concurrent destruction, causing a Use-After-Free.
- [High] The Deferred Processing Context (DPC) thread processes a dying virtual port, resulting in firmware state corruption and Use-After-Free.
--

--- Patch [76]: [PATCH v3 76/88] scsi: qla2xxx: Skip vport under deletion in report ID acquisition ---
commit 6dadc96ded15841eb3868816e697201cbe752d9d
Author: Nilesh Javali <njavali@marvell.com>
Subject: scsi: qla2xxx: Skip vport under deletion in report ID acquisition

This commit addresses an issue in qla24xx_report_id_acquisition() by
skipping vports that have VPORT_DELETE set before taking a reference.
This prevents dying vports from being re-registered in the host_map.

Reported-by: Sashiko <sashiko-dev@google.com>
Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -4268,6 +4268,8 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
>  			spin_lock_irqsave(&ha->vport_slock, flags);
>  			list_for_each_entry(vp, &ha->vp_list, list) {

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to drop the
vport_slock while iterating the vport list in other areas of the driver?

In functions like qla2x00_quiesce_io() and qla2x00_do_dpc_all_vps(), the
code uses list_for_each_entry_safe() but temporarily drops the lock:

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

Since list_for_each_entry_safe() caches the next element (tvp), could a
concurrent qla24xx_vport_delete() completely remove and free tvp while the
lock is dropped, leading to a use-after-free when the loop advances?

>  				if (vp_idx == vp->vp_idx) {
> +					if (test_bit(VPORT_DELETE, &vp->dpc_flags))
> +						break;

[Severity: Critical]
This isn't a bug introduced by this patch, but does checking VPORT_DELETE
here fully resolve the race condition?

In qla24xx_report_id_acquisition(), the vport_slock is dropped shortly after
this check and the vref_count increment:

qla2xxx/qla_mbx.c:qla24xx_report_id_acquisition() {
    ...
    spin_unlock_irqrestore(&ha->vport_slock, flags);

    if (!found)
        return;

    spin_lock_irqsave(&ha->vport_slock, flags);
    qla_update_host_map(vp, id);
    spin_unlock_irqrestore(&ha->vport_slock, flags);
    ...
}

During the unlocked window, could a concurrent qla24xx_vport_delete()
disable the vport and remove it from host_map?

If so, when the lock is reacquired, qla_update_host_map() would re-insert
the now-dying vport into the btree.

[Severity: High]
This isn't a bug introduced by this patch, but should a similar VPORT_DELETE
check be added to the DPC thread iteration?

In qla2xxx/qla_mid.c:qla2x00_do_dpc_all_vps(), the DPC thread iterates
vp_list and invokes qla2x00_do_dpc_vp(vp) without checking if VPORT_DELETE
is set.

If qla24xx_vport_delete() marks a vport with VPORT_DELETE and disables it
in firmware, could the concurrent DPC thread issue mailbox commands that
inadvertently resurrect the dying vport in firmware?

>  					found = 1;
>  					atomic_inc(&vp->vref_count);
>  					break;

[Severity: Critical]
This isn't a bug introduced by this patch, but does the host_map btree have
consistent locking across the driver?

When qla24xx_report_id_acquisition() modifies the btree for physical ports,
it appears to do so without holding vport_slock.

Meanwhile, qla24xx_disable_vp() modifies the btree using hardware_lock
instead of vport_slock:

qla2xxx/qla_mid.c:qla24xx_disable_vp() {
    spin_lock_irqsave(&vha->hw->hardware_lock, flags);
    qla_update_vp_map(vha, RESET_AL_PA);
    spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
}

Could simultaneous operations from these different threads with
non-overlapping locks cause structural corruption of the btree?

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