Re: [PATCH v2 20/60] scsi: qla2xxx: Enable get_firmware_state for 29xx

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Kernel stack memory is leaked to userspace because `qla2x00_get_firmware_state()` copies uninitialized stack variables into the `states` array on error, which is then exposed via sysfs.
- [High] The `qla2x00_fw_state_show()` function attempts to bypass retrieving the firmware state when the chip is down or EEH is busy, but due to a logic flaw, it unconditionally executes the command anyway outside the safety lock.
--

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

scsi: qla2xxx: Enable get_firmware_state for 29xx

This commit enables the get_firmware_state mailbox command for 29xx
adapters. It adds IS_QLA29XX() checks to ensure MBX_12 is properly
set up and reported.

> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index 742becc0dadf7..ee6bf83029911 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -2436,7 +2436,7 @@ qla2x00_mpi_fw_state_show(struct device *dev, struct device_attribute *attr,
>  	u16 mpi_state;
>  	struct qla_hw_data *ha = vha->hw;
>  
> -	if (!(IS_QLA27XX(ha) || IS_QLA28XX(ha)))
> +	if (!(IS_QLA27XX(ha) || IS_QLA28XX(ha) || IS_QLA29XX(ha)))
>  		return scnprintf(buf, PAGE_SIZE,
>  				"MPI state reporting is not supported for this HBA.\n");

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a logic flaw in
qla2x00_fw_state_show() in this same file. When the chip is down, it unlocks
the mutex and jumps to the out label, but then unconditionally executes the
mailbox command again outside the lock:

qla2x00_fw_state_show()
    if (qla2x00_chip_is_down(vha)) {
        mutex_unlock(&vha->hw->optrom_mutex);
        ql_log(ql_log_warn, vha, 0x707c, "ISP reset active.\n");
        goto out;
    }
    ...
out:
    if (rval != QLA_SUCCESS) {
        memset(state, -1, sizeof(state));
        rval = qla2x00_get_firmware_state(vha, state);
    }

Since rval is initialized to QLA_FUNCTION_FAILED, does this bypass the safety
checks and execute qla2x00_get_firmware_state() anyway, potentially causing a
firmware hang?

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index 9c78aa66e12b3..a91ac59dd9c06 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
[ ... ]
> @@ -2301,7 +2301,7 @@ qla2x00_get_firmware_state(scsi_qla_host_t *vha, uint16_t *states)
>  		states[3] = mcp->mb[4];
>  		states[4] = mcp->mb[5];
>  		states[5] = mcp->mb[6];  /* DPORT status */
> -		if (IS_QLA27XX(ha) || IS_QLA28XX(ha))
> +		if (IS_QLA27XX(ha) || IS_QLA28XX(ha) || IS_QLA29XX(ha))
>  			states[11] = mcp->mb[12]; /* MPI state. */
>  	}

[Severity: High]
This is a pre-existing issue, but it looks like there is a kernel stack memory
leak in qla2x00_get_firmware_state() affecting the states array.

The mailbox command structure mbx_cmd_t mc is allocated on the stack but not
zero-initialized. If qla2x00_mailbox_command() fails early and returns
without modifying mc.mb, the code unconditionally copies the uninitialized
variables into the states array:

    states[0] = mcp->mb[1];
    if (IS_FWI2_CAPABLE(vha->hw)) {
        states[1] = mcp->mb[2];
        ...

The sysfs handler then formats this state array directly into its output and
ignores the error return value.

Could this expose uninitialized kernel stack memory to userspace?

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