Re: [PATCH v2 53/60] scsi: qla2xxx: Bound VP index against VP_CTRL IOCB bitmap size

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch prevents out-of-bounds memory accesses by rejecting `vp_index > 128` during VP disable, but fails to clamp the maximum number of advertised VPs to 128. This creates an asymmetry where VPs > 128 can still be created and enabled, but their deletion will silently fail to disable the VP in firmware, leading to a permanent firmware resource leak.

Pre-existing issues:
- [High] A race condition between the async IOCB timeout handler and the ISR completion path can result in double completion executions and use-after-free of the SRB and on-stack completion variable.
--

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

scsi: qla2xxx: Bound VP index against VP_CTRL IOCB bitmap size

This commit fixes an out-of-bounds write in qla25xx_ctrlvp_iocb() by adding
bounds checking for vp_index against the fixed 128-bit vp_idx_map bitmap.
It also adds a check in qla24xx_control_vp() to reject requests with
vp_index > 128.

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> index 7072af5b42178..b7d9c1a53f3ce 100644
> --- a/drivers/scsi/qla2xxx/qla_mid.c
> +++ b/drivers/scsi/qla2xxx/qla_mid.c
> @@ -987,6 +987,14 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd)
>  	if (vp_index == 0 || vp_index >= ha->max_npiv_vports)
>  		return QLA_PARAMETER_ERROR;
>  
> +	/*
> +	 * The VP_CTRL IOCB selects the target VP through a fixed 128-bit
> +	 * (16-byte) vp_idx_map bitmap, so vp_index must fit within it even
> +	 * if firmware advertises more NPIV vports.
> +	 */
> +	if (vp_index > sizeof_field(struct vp_ctrl_entry_24xx, vp_idx_map) * 8)
> +		return QLA_PARAMETER_ERROR;
> +

[Severity: High]
If firmware advertises up to 255 VPs (since ha->max_npiv_vports is parsed from
mcp->mb[11] in qla2x00_get_fw_version()), does this new bounds check introduce
a firmware resource leak during VP deletion?

While this prevents the out-of-bounds write, VPs with vp_index > 128 can still
be successfully created because the maximum advertised limit is not clamped.
When deleting such a VP, qla24xx_vport_delete() calls qla24xx_disable_vp(),
which will now fail and return QLA_PARAMETER_ERROR due to this check.

Because the deletion process ignores this failure and continues fully freeing
the kernel's local data structures, won't the firmware still consider the VP
active, resulting in a permanent leak? Is it possible to clamp
ha->max_npiv_vports to 128 during initialization to prevent creating these
VPs in the first place?

>  	/* ref: INIT */
>  	sp = qla2x00_get_sp(base_vha, NULL, GFP_KERNEL);
>  	if (!sp)

[Severity: High]
This is a pre-existing issue, but does the on-stack completion variable used
alongside this SRB introduce a use-after-free risk due to a race condition
in the async IOCB timeout handler?

If qla2x00_async_iocb_timeout() executes and the abort fails, it attempts to
remove the SRB from the outstanding_cmds array, but unconditionally calls
sp->done(sp, QLA_FUNCTION_TIMEOUT) without verifying if the SRB was actually
found and removed:

drivers/scsi/qla2xxx/qla_init.c:qla2x00_async_iocb_timeout() {
    ...
    spin_unlock_irqrestore(sp->qpair->qp_lock_ptr, flags);
    sp->done(sp, QLA_FUNCTION_TIMEOUT);
    ...
}

If the normal ISR completes the command concurrently, it will remove the SRB
and also call sp->done(). This causes qla_ctrlvp_sp_done() to execute twice.

The first execution wakes up the submitter thread here, causing it to free
sp and destroy the on-stack comp variable. Won't the second execution then
call complete() on a dangling comp pointer, leading to a stack use-after-free?

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