Re: [PATCH v4 08/56] scsi: qla2xxx: Add BSG MPI firmware load/dump for 29xx

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Returning a non-zero error code from `qla2900_bsg_load_mpi()` after explicitly calling `bsg_job_done()` leads to a double completion of the block request.
- [High] The implementations for the LOAD and DUMP MPI commands are perfectly inverted, leading to unintended firmware overwrite or data discard.
--

--- Patch [8]: [PATCH v4 08/56] scsi: qla2xxx: Add BSG MPI firmware load/dump for 29xx ---
commit 472cec56cca22244e315ce21a1660801bbbc160d
Author: Manish Rangankar <mrangankar@marvell.com>

scsi: qla2xxx: Add BSG MPI firmware load/dump for 29xx

This patch adds BSG vendor commands for loading and dumping MPI firmware on
29xx adapters. It extends the existing BSG infrastructure with the necessary
mailbox wrappers and flash helpers for MPI operations.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index 92a1af81c057f..5dd7e5e969ab0 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -1820,6 +1820,203 @@ static int qla29xx_bsg_flash_block_write(struct bsg_job *bsg_job)

[ ... ]

> +static int qla2900_bsg_dump_mpi(struct bsg_job *bsg_job)
> +{
> +	struct fc_bsg_request *bsg_req = bsg_job->request;
> +	struct fc_bsg_reply *bsg_reply = bsg_job->reply;
> +	struct Scsi_Host *host = fc_bsg_to_shost(bsg_job);
> +	scsi_qla_host_t *vha = shost_priv(host);
> +	struct qla_hw_data *ha = vha->hw;
> +	struct qla_load_dump_mpi *dmcmd;
> +	uint16_t opts = 0;
> +	int rval = 0;
> +
> +	if (bsg_job->request_len < sizeof(struct fc_bsg_request) +
> +	    2 * sizeof(uint32_t) + sizeof(struct qla_load_dump_mpi))
> +		return -EINVAL;
> +
> +	if (!IS_QLA29XX(ha)) {
> +		ql_log(ql_log_warn, vha, 0x706c,
> +		       "%s: MPI dump not supported on this adapter.\n",
> +		       __func__);
> +		return -EINVAL;
> +	}
> +
> +	dmcmd =
> +	(struct qla_load_dump_mpi *)&bsg_req->rqst_data.h_vendor.vendor_cmd[2];
> +
> +	ql_log(ql_log_info, vha, 0xffff,
> +	       "%s: mpi_address 0x%x mpi options 0x%x length 0x%x\n",
> +		__func__, dmcmd->mpi_address, dmcmd->options, dmcmd->length);
> +
> +	mutex_lock(&ha->optrom_mutex);
> +	rval = qla29xx_mpi_optrom_setup(bsg_job, vha, dmcmd->mpi_address, 1);
> +	if (rval) {
> +		mutex_unlock(&ha->optrom_mutex);
> +		return rval;
> +	}
> +
> +	sg_copy_to_buffer(bsg_job->request_payload.sg_list,
> +			  bsg_job->request_payload.sg_cnt, ha->optrom_buffer,
> +			  ha->optrom_region_size);

[Severity: High]
Are the implementations for the DUMP and LOAD MPI commands inverted here?

In drivers/scsi/qla2xxx/qla_bsg.c:qla2900_bsg_dump_mpi(), this path configures
the operation as a write (is_update=1) and takes data from the user's
request_payload. This matches the behavior of a firmware load, which could
inadvertently write uninitialized or incorrect data to the device when a user
is attempting to dump the firmware.

[ ... ]

> +static int qla2900_bsg_load_mpi(struct bsg_job *bsg_job)
> +{
> +	struct fc_bsg_request *bsg_req = bsg_job->request;
> +	struct fc_bsg_reply *bsg_reply = bsg_job->reply;
> +	struct Scsi_Host *host = fc_bsg_to_shost(bsg_job);
> +	scsi_qla_host_t *vha = shost_priv(host);
> +	struct qla_hw_data *ha = vha->hw;
> +	struct qla_load_dump_mpi *lmcmd;
> +	uint16_t opts = 0;
> +	int rval = 0;
> +
> +	if (bsg_job->request_len < sizeof(struct fc_bsg_request) +
> +	    2 * sizeof(uint32_t) + sizeof(struct qla_load_dump_mpi))
> +		return -EINVAL;
> +
> +	if (!IS_QLA29XX(ha)) {
> +		ql_log(ql_log_warn, vha, 0x706d,
> +		       "%s: MPI load not supported on this adapter.\n",
> +		       __func__);
> +		return -EINVAL;
> +	}
> +
> +	lmcmd =
> +	(struct qla_load_dump_mpi *)&bsg_req->rqst_data.h_vendor.vendor_cmd[2];
> +
> +	ql_log(ql_log_info, vha, 0xffff,
> +	       "%s: mpi_address 0x%x mpi options 0x%x length 0x%x\n",
> +		__func__, lmcmd->mpi_address, lmcmd->options, lmcmd->length);
> +
> +	mutex_lock(&ha->optrom_mutex);
> +	rval = qla29xx_mpi_optrom_setup(bsg_job, vha, lmcmd->mpi_address, 0);
> +	if (rval) {
> +		mutex_unlock(&ha->optrom_mutex);
> +		return rval;
> +	}
> +
> +	check_and_set_mbc_bits(lmcmd->options, opts, QLA_LDM_SECURE_ENABLE,
> +			       BIT_3);

[Severity: High]
Does this execute a read/dump flow during a load command?

In drivers/scsi/qla2xxx/qla_bsg.c:qla2900_bsg_load_mpi(), the function
configures the operation as a read (is_update=0) and later copies data to
the user's reply_payload. This behaves like a dump instead of a load, causing
the firmware image provided by the user to be discarded rather than written to
the device.

[ ... ]

> +	bsg_job->reply_len = sizeof(struct fc_bsg_reply);
> +	bsg_job_done(bsg_job, bsg_reply->result,
> +		     bsg_reply->reply_payload_rcv_len);
> +
> +	return rval;
> +}

[Severity: High]
Does this code risk a double completion of the block request?

If qla29xx_mpi_optrom_data() fails and sets an error code in rval, this path
calls bsg_job_done() and then returns the non-zero rval.

When a BSG vendor-specific request handler returns a non-zero value, the SCSI
FC transport layer (fc_bsg_host_dispatch) assumes the driver rejected the
request and falls back to completing the job itself. Since bsg_job_done() was
already called, this could complete the same block request twice, which could
lead to a use-after-free or list corruption.

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