Re: [PATCH v2 10/13] scsi: qla2xxx: Fix use-after-free races during vport deletion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] scsi: qla2xxx: infinite loop and UAF in vp_list loops with list_del_init()
- [High] scsi: qla2xxx: missing vport validation and tvp refresh in vp_list loops
--

commit 24f27bc8c1d9341721d99c592298135dfbc0b9d5
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Fix use-after-free races during vport deletion

This commit adds vport validation checks (vp_idx, delete_progress, and
VPORT_DELETE) to several ha->vp_list traversals and switches
qla24xx_deallocate_vp_id() from list_del() to list_del_init() to prevent
use-after-free crashes during vport deletion.

> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index 7898521b76d4d..c02571a6f0c71 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -7573,15 +7573,17 @@ qla2x00_abort_isp_cleanup(scsi_qla_host_t *vha)
>  
>  		spin_lock_irqsave(&ha->vport_slock, flags);
>  		list_for_each_entry_safe(vp, tvp, &ha->vp_list, list) {
> -			atomic_inc(&vp->vref_count);
> -			spin_unlock_irqrestore(&ha->vport_slock, flags);
> +			if (vp->vp_idx && !vp->flags.delete_progress) {
> +				atomic_inc(&vp->vref_count);
> +				spin_unlock_irqrestore(&ha->vport_slock, flags);
>  
> -			qla2x00_mark_all_devices_lost(vp);
> +				qla2x00_mark_all_devices_lost(vp);
>  
> -			spin_lock_irqsave(&ha->vport_slock, flags);
> -			/* Check if vp is still valid before accessing it */
> -			if (vp->vp_idx && !vp->flags.delete_progress)
> +				spin_lock_irqsave(&ha->vport_slock, flags);
> +				/* Reference held under vport_slock keeps vp valid. */
>  				atomic_dec(&vp->vref_count);
> +				wake_up(&vp->vref_waitq);
> +			}
>  		}
>  		spin_unlock_irqrestore(&ha->vport_slock, flags);

[Severity: High]
The commit message notes that it validates vp_idx and delete_progress across
all remaining vp_list traversals, but what about qla2x00_quiesce_io() and
qla82xx_restart_isp() in drivers/scsi/qla2xxx/qla_init.c?

In qla2x00_quiesce_io(), ha->vp_list is traversed and ha->vport_slock is
dropped without checking vp->vp_idx or !vp->flags.delete_progress, and
without waking vp->vref_waitq:

drivers/scsi/qla2xxx/qla_init.c:qla2x00_quiesce_io() {
    ...
		spin_lock_irqsave(&ha->vport_slock, flags);
		list_for_each_entry_safe(vp, tvp, &ha->vp_list, list) {
			atomic_inc(&vp->vref_count);
			spin_unlock_irqrestore(&ha->vport_slock, flags);

			qla2x00_mark_all_devices_lost(vp);

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

Similarly, qla82xx_restart_isp() checks vp->vp_idx (which is not cleared by
qla24xx_deallocate_vp_id()), but does not check !vp->flags.delete_progress or
call wake_up(&vp->vref_waitq):

drivers/scsi/qla2xxx/qla_init.c:qla82xx_restart_isp() {
    ...
		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);

				qla2x00_vp_abort_isp(vp);

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

Since NPIV is supported on QLA8031, QLA82XX, and QLA8044 adapters, if
qla24xx_vport_delete() runs concurrently with qla2x00_quiesce_io() or
qla82xx_restart_isp() and deletes the prefetched next entry tvp while
ha->vport_slock is dropped, won't the next iteration advance vp to the
removed and freed tvp, enter the loop body because !vp->flags.delete_progress
is not checked, and access freed vp and vp->vp_fcports memory in
qla2x00_mark_all_devices_lost(vp) or qla2x00_vp_abort_isp(vp)?

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index 90f09d36705a6..754c806dac807 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c
> @@ -79,7 +79,7 @@ qla24xx_deallocate_vp_id(scsi_qla_host_t *vha)
>  	for (i = 0; i < 500; i++) {
>  		spin_lock_irqsave(&ha->vport_slock, flags);
>  		if (atomic_read(&vha->vref_count) == 0) {
> -			list_del(&vha->list);
> +			list_del_init(&vha->list);
>  			qla_update_vp_map(vha, RESET_VP_IDX);
>  			bailout = 1;
>  		}
> @@ -94,7 +94,7 @@ qla24xx_deallocate_vp_id(scsi_qla_host_t *vha)
>  		ql_log(ql_log_info, vha, 0xfffa,
>  			"vha->vref_count=%u timeout\n", vha->vref_count.counter);
>  		spin_lock_irqsave(&ha->vport_slock, flags);
> -		list_del(&vha->list);
> +		list_del_init(&vha->list);
>  		qla_update_vp_map(vha, RESET_VP_IDX);
>  		spin_unlock_irqrestore(&ha->vport_slock, flags);
>  	}

[Severity: Critical]
Can changing list_del(&vha->list) to list_del_init(&vha->list) in
qla24xx_deallocate_vp_id() still trigger a use-after-free and also cause an
infinite loop with interrupts disabled in the list_for_each_entry_safe()
traversals?

When ha->vp_list is traversed using list_for_each_entry_safe(vp, tvp,
&ha->vp_list, list) in qla2x00_abort_isp_cleanup(), qla2x00_abort_isp(),
qla_pci_error_cleanup(), qla2x00_do_dpc_all_vps(), qla2x00_alert_all_vps(),
qla2x00_quiesce_io(), and qla82xx_restart_isp(), the macro prefetches the
next entry tvp = list_next_entry(vp, list) before running the loop body. The
loop body only increments vp->vref_count (leaving tvp->vref_count at 0) and
then drops ha->vport_slock.

While ha->vport_slock is dropped during vp processing, a concurrent
qla24xx_vport_delete() -> qla24xx_deallocate_vp_id(tvp) can acquire
ha->vport_slock, observe atomic_read(&tvp->vref_count) == 0, unlink tvp with
list_del_init(&tvp->list), and release ha->vport_slock, after which
qla24xx_vport_delete() frees tvp via scsi_host_put(tvp->host).

When the traversal thread finishes processing vp, re-acquires ha->vport_slock
with spin_lock_irqsave(), and advances the loop (vp = tvp, tvp =
list_next_entry(tvp, list)):

1. It dereferences tvp after it has been unlinked and potentially freed by
scsi_host_put(tvp->host), causing a use-after-free on tvp->list.next and
subsequently on vp->vp_idx, vp->flags.delete_progress, or vp->dpc_flags.

2. Because list_del_init(&tvp->list) sets tvp->list.next = &tvp->list,
list_next_entry(tvp, list) evaluates to tvp itself. Since &tvp->list !=
&ha->vp_list, the loop termination condition is never met, and subsequent
iterations skip the body (because delete_progress or VPORT_DELETE is set)
without dropping ha->vport_slock, spinning forever on the self-linked entry
with local interrupts disabled.

A follow-up commit in the series, commit a9b1443e24db ("scsi: qla2xxx:
Re-acquire vport_slock and refresh tvp during vport DPC"), refreshes tvp =
list_next_entry(vp, list) after re-acquiring ha->vport_slock in
qla2x00_do_dpc_all_vps() and qla2x00_alert_all_vps(), but leaves the
list_for_each_entry_safe() loops in qla2x00_abort_isp_cleanup(),
qla2x00_abort_isp(), qla_pci_error_cleanup(), qla2x00_quiesce_io(), and
qla82xx_restart_isp() without tvp refreshes. Should tvp be refreshed after
re-acquiring ha->vport_slock in all of those loops as well?

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