Re: [PATCH v5 48/56] scsi: qla2xxx: Bound VP index against VP_CTRL IOCB bitmap size

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch prevents disabling virtual ports with an index > 128 but fails to prevent their creation. This creates an asymmetric lifecycle where high-index vports can be successfully created but will silently fail to be disabled, permanently leaking firmware resources.
--

--- Patch [48]: [PATCH v5 48/56] scsi: qla2xxx: Bound VP index against VP_CTRL IOCB bitmap size ---
commit d14a096bfdf302413bad8d30ac696540d3cc6f7a
Author: Nilesh Javali <njavali@marvell.com>

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

This commit adds bounds checking for vp_index when issuing VP_CTRL IOCBs
to prevent out-of-bounds writes to the vp_idx_map array. It explicitly
rejects commands for virtual ports with an index greater than 128.

> diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c
> --- 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]
This is a pre-existing issue, but by blocking control commands for virtual
ports with an index over 128, does this create an asymmetric lifecycle that
leaks firmware resources?

If the firmware advertises ha->max_npiv_vports up to 255, creating a vport
with an index > 128 can still succeed since qla24xx_vport_create() uses
VP_CONFIG_IOCB, which has an 8-bit index capable of addressing up to 255.

However, when deleting the vport, qla24xx_vport_delete() calls
qla24xx_disable_vp(), which in turn calls qla24xx_control_vp() to send
the disable command. With this new check, disabling the vport will fail
and return QLA_PARAMETER_ERROR.

Looking at qla24xx_vport_delete(), it ignores the return value of
qla24xx_disable_vp():

drivers/scsi/qla2xxx/qla_attr.c:qla24xx_vport_delete() {
    ...
    qla24xx_disable_vp(vha);
    qla2x00_wait_for_sess_deletion(vha);

    qla_nvme_delete(vha);
    qla_enode_stop(vha);
    qla_edb_stop(vha);

    vha->flags.delete_progress = 1;
    ...
}

Since the failure is ignored, the OS-side structures are destroyed and the
vp_idx is freed for reuse, but the firmware port remains fully active and
logged into the fabric. Reusing the ID later could then corrupt the firmware
state machine.

Should ha->max_npiv_vports be capped at 128 during initialization to
prevent creating these unmanageable virtual ports in the first place?

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