Re: [PATCH v3 62/88] scsi: qla2xxx: Don't query firmware state while chip is down

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `qla2x00_fw_state_show` sysfs attribute handler leaks 10 bytes of uninitialized kernel stack memory to unprivileged userspace on older (non-FWI2 capable) adapters.
- [High] TOCTOU race condition in `qlafx00_fw_state_show()` due to lockless checks, allowing unsynchronized mailbox commands during ISP reset.
--

--- Patch [62]: [PATCH v3 62/88] scsi: qla2xxx: Don't query firmware state while chip is down ---
commit 7d6c905f305cb054a288e70e2b54f81eb14573f5
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Don't query firmware state while chip is down

This commit addresses an issue where qla2x00_fw_state_show() issues an
unsynchronized mailbox command during an ISP reset or PCI error recovery.
It updates the error path to avoid re-issuing qla2x00_get_firmware_state()
outside of the optrom_mutex. This ensures mailbox commands are only sent
when the chip is up and not EEH-busy.

> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index 6a87d3bb0b0eb..a4ca22024edeb 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -1678,10 +1678,8 @@ qla2x00_fw_state_show(struct device *dev, struct device_attribute *attr,
>  	rval = qla2x00_get_firmware_state(vha, state);
>  	mutex_unlock(&vha->hw->optrom_mutex);
>  out:
> -	if (rval != QLA_SUCCESS) {
> +	if (rval != QLA_SUCCESS)
>  		memset(state, -1, sizeof(state));
> -		rval = qla2x00_get_firmware_state(vha, state);
> -	}
>  
>  	return scnprintf(buf, PAGE_SIZE, "0x%x 0x%x 0x%x 0x%x 0x%x 0x%x\n",
>  	    state[0], state[1], state[2], state[3], state[4], state[5]);

[Severity: High]
This is a pre-existing issue, but does this function leak uninitialized kernel
stack memory to userspace on older adapters?

In qla2x00_fw_state_show(), the state array is declared on the stack without
initialization. If qla2x00_get_firmware_state() succeeds on a non-FWI2
adapter, only the first element is populated:

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

The remaining elements (state[1] through state[5]) remain uninitialized, but
they are unconditionally formatted by scnprintf() and exposed to userspace
when reading the sysfs attribute. Should the state array be zero-initialized
when it is declared?

[Severity: High]
This isn't a bug introduced by this patch, but while fixing the missing
serialization here, is the qlafx00-specific path still vulnerable to the
same adapter hang?

When qla2x00_fw_state_show() calls qlafx00_fw_state_show() earlier in the
function for fx00 adapters, the reset and EEH busy states are checked
without holding the optrom_mutex:

qlafx00_fw_state_show() {
        ...
        if (qla2x00_reset_active(vha))
                ql_log(ql_log_warn, vha, 0x70ce, "ISP reset active.\n");
        else if (!vha->hw->flags.eeh_busy) {
                rval = qlafx00_get_firmware_state(vha, state);
        }
        ...
}

If an ISP reset or PCI error recovery begins immediately after this lockless
check, does qlafx00_get_firmware_state() issue an unsynchronized mailbox
command to the resetting chip? Could this be prevented by applying the same
mutex protection to the qlafx00 branch?

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