Re: [PATCH v3 71/88] scsi: qla2xxx: Avoid req_q_map double-read in qla2x00_error_entry()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing bounds and NULL pointer checks on hardware-supplied queue index in QLAFX00 handlers lead to OOB reads and wild pointer dereferences.
- [High] The patch transforms a NULL pointer dereference into a Use-After-Free and fails to prevent compiler reloads due to a missing READ_ONCE().
- [High] Out-of-bounds array access on `req_q_map` before validation in `qla2x00_status_entry`.
- [High] Lack of bounds checking for target QID index on `req_q_map` array access in `qlt_ctio_to_cmd`.
--

--- Patch [71]: [PATCH v3 71/88] scsi: qla2xxx: Avoid req_q_map double-read in qla2x00_error_entry() ---
commit c14818342918ef21459d32bbecb1126af60c505c
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Avoid req_q_map double-read in qla2x00_error_entry()

This patch attempts to fix a potential double-read of the request queue map
in the error entry ISR. It caches the map slot into a local variable to
perform the NULL check and subsequent use. The issue was observed during
queue teardown when the map slot is cleared concurrently.

> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index 91a8344fea6c7..829671937f92e 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -3927,10 +3927,12 @@ qla2x00_error_entry(scsi_qla_host_t *vha, struct rsp_que *rsp, sts_entry_t *pkt)
[ ... ]
>  
> -	if (que >= ha->max_req_queues || !ha->req_q_map[que])
> +	if (que >= ha->max_req_queues)
>  		goto fatal;
>  
>  	req = ha->req_q_map[que];
> +	if (!req)
> +		goto fatal;

[Severity: High]
This isn't a bug introduced by this patch, but caching the pointer without
READ_ONCE() might allow the compiler to reload the pointer from memory,
potentially re-introducing the NULL dereference.

Additionally, does this code allow a use-after-free?

As mentioned in the commit message, qla25xx_free_req_que() runs concurrently
and clears the slot before freeing the queue:

> drivers/scsi/qla2xxx/qla_mid.c:qla25xx_free_req_que() {
> ...
>         mutex_lock(&ha->mq_lock);
>         ha->req_q_map[que_id] = NULL;
>         clear_bit(que_id, ha->req_qid_map);
>         mutex_unlock(&ha->mq_lock);
>     }
>     kfree(req->outstanding_cmds);
>     kfree(req);
> }

Since this ISR does not synchronize with the teardown, could the ISR fetch
the pointer just before it is cleared, pass the NULL check, and then access
req after it has been freed by the other CPU?


[Severity: Critical]
This is a pre-existing issue, but does qlafx00_status_entry() lack bounds
and NULL pointer checks on the hardware-supplied queue index?

> drivers/scsi/qla2xxx/qla_mr.c:qlafx00_status_entry() {
>     ...
>     que = MSW(hindex);
>     req = ha->req_q_map[que];
> 
>     /* Validate handle. */
>     if (handle < req->num_outstanding_cmds)
>     ...
> }

Could a malformed hardware response trigger an out-of-bounds read and
subsequent wild pointer dereference if que exceeds max_req_queues or req
is NULL?

This same pattern appears to be present in qlafx00_multistatus_entry()
and qlafx00_error_entry().


[Severity: High]
This is a pre-existing issue, but does qla2x00_status_entry() access the
array before validating the index bounds?

> drivers/scsi/qla2xxx/qla_isr.c:qla2x00_status_entry() {
>     ...
>     que = MSW(sts->handle);
>     req = ha->req_q_map[que];
> 
>     /* Check for invalid queue pointer */
>     if (req == NULL ||
>         que >= find_first_zero_bit(ha->req_qid_map, ha->max_req_queues)) {
>     ...
> }

Can a corrupted hardware input cause an out-of-bounds memory read during
the assignment to req, before the bounds check evaluates?


[Severity: High]
This is a pre-existing issue, but does qlt_ctio_to_cmd() access the
req_q_map array without bounds checking?

> drivers/scsi/qla2xxx/qla_target.c:qlt_ctio_to_cmd() {
>     ...
>     int qid = GET_QID(handle);
>     ...
>     } else if (vha->hw->req_q_map[qid]) {
>         ql_dbg(ql_dbg_tgt_mgt, vha, 0x1000a,
>             "qla_target(%d): CTIO completion with different QID %d handle %x\n",
>             vha->vp_idx, rsp->id, handle);
>         req = vha->hw->req_q_map[qid];
>     ...
> }

Could an untrusted hardware handle with a qid exceeding max_req_queues
cause an out-of-bounds read?

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