Re: [PATCH v3 49/88] scsi: qla2xxx: Check entry_status in qla24xx_modify_vp_config()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing early reject check in qla84xx_verify_chip() leads to false success.
- [High] Ignored IOCB execution status in qla84xx_updatefw() and qla84xx_mgmt_cmd().
--

--- Patch [49]: [PATCH v3 49/88] scsi: qla2xxx: Check entry_status in qla24xx_modify_vp_config() ---
commit a6a888a28160758908720e759f4660c0e9650700
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Check entry_status in qla24xx_modify_vp_config()

The Modify VP Config completion handler labelled its first error branch
"error status" but tested vpmod->comp_status instead of
vpmod->entry_status. Because CS_COMPLETE is 0, the following
"comp_status != CS_COMPLETE" branch duplicated that test and was dead
code, and entry_status was never examined at all.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index b32ca8ed274d6..59023492c5a98 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -4401,10 +4401,10 @@ qla24xx_modify_vp_config(scsi_qla_host_t *vha)
>  	if (rval != QLA_SUCCESS) {
>  		ql_dbg(ql_dbg_mbx, vha, 0x10bd,
>  		    "Failed to issue VP config IOCB (%x).\n", rval);
> -	} else if (vpmod->comp_status != 0) {
> +	} else if (vpmod->entry_status != 0) {
>  		ql_dbg(ql_dbg_mbx, vha, 0x10be,
>  		    "Failed to complete IOCB -- error status (%x).\n",
> -		    vpmod->comp_status);
> +		    vpmod->entry_status);
>  		rval = QLA_FUNCTION_FAILED;
>  	} else if (vpmod->comp_status != cpu_to_le16(CS_COMPLETE)) {
>  		ql_dbg(ql_dbg_mbx, vha, 0x10bf,

[Severity: High]
This isn't a bug introduced by this patch, but while you are fixing this
missing entry_status check here, does qla84xx_verify_chip() suffer from the
exact same flaw?

During driver initialization or error recovery, if the firmware rejects the
verify IOCB early, it sets entry_status but leaves comp_status as 0 
(CS_COMPLETE) from the initial memset. The driver only evaluates
comp_status:

drivers/scsi/qla2xxx/qla_mbx.c:qla84xx_verify_chip() {
    ...
    status[0] = le16_to_cpu(mn->p.rsp.comp_status);
    status[1] = status[0] == CS_VCS_CHIP_FAILURE ?
        le16_to_cpu(mn->p.rsp.failure_code) : 0;
    ...
}

Could this result in a malfunctioning 84xx adapter, or one with an invalid
firmware image, being falsely marked as initialized and ready?

[Severity: High]
This is also a pre-existing issue, but do qla84xx_updatefw() and
qla84xx_mgmt_cmd() similarly ignore the IOCB execution status?

In both of these functions, the driver calls qla2x00_issue_iocb_timeout()
which verifies that the mailbox command was accepted. However, the driver
then unconditionally sets the result to DID_OK without checking the
entry_status or comp_status of the returned IOCB:

drivers/scsi/qla2xxx/qla_bsg.c:qla84xx_updatefw() {
    ...
    bsg_reply->result = DID_OK;
    ...
}

drivers/scsi/qla2xxx/qla_bsg.c:qla84xx_mgmt_cmd() {
    ...
    bsg_reply->result = DID_OK;
    ...
}

Would this cause BSG operations like firmware updates to silently fail
but report success to userspace, or return zeroed memory for read commands
instead of an error?

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