Re: [PATCH v3 59/88] scsi: qla2xxx: Fix use-after-free of qpair work on queue teardown

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete fix for Use-After-Free of `qpair->q_work` on queue creation error path.
- [High] Race condition causing kernel panic if an interrupt fires before `INIT_WORK()`.
--

--- Patch [59]: [PATCH v3 59/88] scsi: qla2xxx: Fix use-after-free of qpair work on queue teardown ---
commit 7b4564aa91c2297e4133057969d048526e18052f
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Fix use-after-free of qpair work on queue teardown

The response queue MSI-X handler qla2xxx_msix_rsp_q() schedules
qla_do_work() via queue_work(ha->wq, &qpair->q_work). qla_do_work()
dereferences the qpair (vha, rsp) and takes qpair->qp_lock.

During teardown, qla2xxx_delete_qpair() deletes the response queue, which
calls free_irq() in qla25xx_free_rsp_que(), and then frees the queue and
the qpair. free_irq() waits for running hardirq handlers but does not
cancel work already placed on ha->wq. A still-pending q_work then runs
qla_do_work() against the freed qpair and response queue, causing a
use-after-free.

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index b7d9c1a53f3ce..33bfc61d81652 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c

[ ... ]

> @@ -606,6 +606,10 @@ qla25xx_free_rsp_que(struct scsi_qla_host *vha, struct rsp_que *rsp)
>  		rsp->msix->handle = NULL;
>  	}
>  
> +	/* Flush any queued response work before freeing the queue/qpair. */
> +	if (rsp->qpair && ha->wq)
> +		cancel_work_sync(&rsp->qpair->q_work);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this newly added guard
completely mitigate the use-after-free on the queue creation error path?

Looking at qla2xxx_create_qpair() in drivers/scsi/qla2xxx/qla_init.c,
rsp->qpair is only assigned after the request queue is successfully created:

drivers/scsi/qla2xxx/qla_init.c:qla2xxx_create_qpair() {
    ...
	ret = qla25xx_create_req_que(ha, options, vp_id, 0,
				     req_req_dma(qpair), req_req_size(qpair));
	if (!ret) {
		qpair->req = ha->req_q_map[vp_id];
        ...
		qpair->rsp->req = qpair->req;
		qpair->rsp->qpair = qpair;
    ...
}

If qla25xx_create_req_que() fails, the code jumps to fail_req and deletes
the response queue:

drivers/scsi/qla2xxx/qla_init.c:qla2xxx_create_qpair() {
    ...
fail_req:
	qla25xx_delete_rsp_que(vha, qpair->rsp);
    ...
}

When qla25xx_delete_rsp_que() calls qla25xx_free_rsp_que(), rsp->qpair will
be NULL. This bypasses the cancel_work_sync(), allowing any pending work
(scheduled by a spurious or early interrupt) to execute after qpair is freed
via kfree(qpair).

Is there a way to ensure the work is flushed even on this error path?


[Severity: High]
This is also a pre-existing issue, but is there a risk of enqueuing an
uninitialized work_struct if an interrupt fires too early during setup?

Looking at qla25xx_create_rsp_que() in drivers/scsi/qla2xxx/qla_mid.c,
the interrupt vector is requested before the work_struct is initialized:

drivers/scsi/qla2xxx/qla_mid.c:qla25xx_create_rsp_que() {
    ...
	ret = qla25xx_request_irq(ha, qpair, qpair->msix);
	if (ret)
		goto que_failed;
    ...
	if (qpair->hw->wq)
		INIT_WORK(&qpair->q_work, qla_do_work);
    ...
}

If a spurious interrupt fires immediately after qla25xx_request_irq(), it
triggers qla2xxx_msix_rsp_q() which calls queue_work() on qpair->q_work
before INIT_WORK() has been executed.

Additionally, if qla25xx_init_rsp_que() fails, the code jumps to que_failed
and INIT_WORK() is bypassed entirely, leaving the work completely
uninitialized when the ISR runs.

Could INIT_WORK() be moved before requesting the interrupt to prevent this?

>  	if (rsp->ring)
>  		dma_free_coherent(&ha->pdev->dev,
>  				  (rsp->length + 1) * rsp_entry_size,

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