Re: [PATCH v2 11/13] scsi: qla2xxx: Re-acquire vport_slock and refresh tvp during vport DPC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] scsi: qla2xxx: use-after-free on vp in qla2x00_do_dpc_all_vps() on timeout
- [Critical] scsi: qla2xxx: unrefreshed tvp in vp_list loops causes hard lockup and UAF
--

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

scsi: qla2xxx: Re-acquire vport_slock and refresh tvp during vport DPC

This commit re-acquires vport_slock using a spin_trylock_irqsave() and
cond_resched() loop in qla2x00_do_dpc_all_vps(), refreshes the next list entry
tvp after re-locking in qla2x00_do_dpc_all_vps() and qla2x00_alert_all_vps(),
and adds a cond_resched() before qla2x00_loop_resync() in qla2x00_do_dpc_vp().

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index 754c806dac807..65dcfc1b04ef9 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c
> @@ -315,6 +315,10 @@ qla2x00_alert_all_vps(struct rsp_que *rsp, uint16_t *mb)
>  			/* Reference held under vport_slock keeps vha valid. */
>  			atomic_dec(&vha->vref_count);
>  			wake_up(&vha->vref_waitq);
> +			/* refresh tvp from the current list state */
> +			if (list_empty(&vha->list))
> +				break;
> +			tvp = list_next_entry(vha, list);
>  		}
>  		i++;
>  	}

[ ... ]

> @@ -446,11 +451,28 @@ qla2x00_do_dpc_all_vps(scsi_qla_host_t *vha)
>  			spin_unlock_irqrestore(&ha->vport_slock, flags);
>  
>  			qla2x00_do_dpc_vp(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)
> -				atomic_dec(&vp->vref_count);
> +			cond_resched();
> +
> +			/* Re-acquire lock with yield to avoid hard lockup if holder is delayed */
> +			while (!spin_trylock_irqsave(&ha->vport_slock, flags))
> +				cond_resched();
> +			/* Reference held under vport_slock keeps vp valid. */
> +			atomic_dec(&vp->vref_count);
> +			wake_up(&vp->vref_waitq);
> +			/*
> +			 * While the lock was dropped, the next entry (tvp) may
> +			 * have been removed via list_del_init(), making its
> +			 * list.next point to itself. list_for_each_entry_safe
> +			 * would then loop infinitely since the self-referencing
> +			 * pointer never matches the list head.
> +			 *
> +			 * If vp itself was removed (deallocate timeout), we
> +			 * cannot continue iterating from it, break out.
> +			 * Otherwise refresh tvp from the current list state.
> +			 */
> +			if (list_empty(&vp->list))
> +				break;

[Severity: Critical]
Can accessing vp here (and vha in qla2x00_alert_all_vps() above) trigger a
use-after-free when qla24xx_deallocate_vp_id() times out?

Neither qla2x00_do_dpc_all_vps() nor qla2x00_alert_all_vps() takes a
Scsi_Host reference via scsi_host_get() before dropping ha->vport_slock.
If a concurrent vport deletion in qla24xx_vport_delete() calls
qla24xx_deallocate_vp_id() and the wait for vref_count to reach zero times
out, qla24xx_deallocate_vp_id() forcibly removes vha from ha->vp_list even
while vref_count is still non-zero:

drivers/scsi/qla2xxx/qla_mid.c:qla24xx_deallocate_vp_id() {
	...
	if (!bailout) {
		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_init(&vha->list);
		qla_update_vp_map(vha, RESET_VP_IDX);
		spin_unlock_irqrestore(&ha->vport_slock, flags);
	}
}

Immediately after qla24xx_deallocate_vp_id() returns, qla24xx_vport_delete()
frees vha's resources and drops the final host reference:

drivers/scsi/qla2xxx/qla_attr.c:qla24xx_vport_delete() {
	...
	qla24xx_deallocate_vp_id(vha);
	...
	scsi_host_put(vha->host);
}

Because list_empty(&vp->list) only becomes true while vref_count > 0 when that
deallocate timeout occurs, wouldn't vp (or vha in qla2x00_alert_all_vps())
already be freed or in the middle of being freed by the time this thread
re-acquires ha->vport_slock and accesses vp->vref_count, vp->vref_waitq, and
vp->list?

> +			tvp = list_next_entry(vp, list);

[Severity: Critical]
What about the other list_for_each_entry_safe(vp, tvp, &ha->vp_list, list)
loops that drop and re-acquire ha->vport_slock without refreshing tvp?

Since qla24xx_deallocate_vp_id() uses list_del_init(&vha->list) when
vref_count == 0, any concurrent deletion of the prefetched next entry tvp
while ha->vport_slock is dropped sets tvp->list.next to point to itself and
frees tvp via qla24xx_vport_delete() -> scsi_host_put().

The following functions still iterate over ha->vp_list with
list_for_each_entry_safe(), drop ha->vport_slock inside the loop body, and do
not refresh tvp after re-acquiring the lock:

drivers/scsi/qla2xxx/qla_init.c:qla2x00_quiesce_io()
drivers/scsi/qla2xxx/qla_init.c:qla2x00_abort_isp_cleanup() (two loops)
drivers/scsi/qla2xxx/qla_init.c:qla2x00_abort_isp()
drivers/scsi/qla2xxx/qla_init.c:qla82xx_restart_isp()
drivers/scsi/qla2xxx/qla_os.c:qla_pci_error_cleanup() (two loops)

For example, in qla2x00_abort_isp():

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

			qla2x00_vp_abort_isp(vp);

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

If tvp is deleted while ha->vport_slock is dropped in any of those loops,
won't the next iteration dereference the freed tvp and then spin forever with
interrupts disabled because tvp->list.next points to itself?

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