Re: [PATCH v6 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] Double completion of a BSG job resulting in use-after-free in the block layer.
- [High] Swapped semantics between LOAD and DUMP operations for MPI firmware.
--

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

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

This commit adds BSG vendor commands for loading and dumping MPI firmware
on 29xx adapters. It extends the existing BSG infrastructure by providing
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 b0ba927c54d71..8cad9a046ad9b 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
[ ... ]
> @@ -1768,6 +1768,203 @@ static int qla29xx_bsg_flash_block_write(struct bsg_job *bsg_job)
[ ... ]
> +static int qla2900_bsg_dump_mpi(struct bsg_job *bsg_job)
> +{
[ ... ]
> +	mutex_lock(&ha->optrom_mutex);
> +	rval = qla29xx_mpi_optrom_setup(bsg_job, vha, dmcmd->mpi_address, 1);

[Severity: High]
Are the semantics for DUMP and LOAD swapped in these functions?

It looks like the dump function sets is_update to 1 here, which reads from the
request payload and writes to the adapter.

> +	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);
[ ... ]
> +	rval = qla29xx_mpi_optrom_data(vha, opts, ha->optrom_buffer,
> +				       ha->optrom_region_start,
> +				       ha->optrom_region_size,
> +				       QLA29XX_MPI_OP_DUMP);
[ ... ]
> +static int qla2900_bsg_load_mpi(struct bsg_job *bsg_job)
> +{
[ ... ]
> +	mutex_lock(&ha->optrom_mutex);
> +	rval = qla29xx_mpi_optrom_setup(bsg_job, vha, lmcmd->mpi_address, 0);

[Severity: High]
Conversely, the load function sets is_update to 0 here, reading from the
adapter into the reply payload.

Could this accidentally overwrite the adapter firmware when a user merely
attempts to dump it?

> +	if (rval) {
> +		mutex_unlock(&ha->optrom_mutex);
> +		return rval;
> +	}
[ ... ]
> +	bsg_reply->reply_payload_rcv_len = ha->optrom_region_size;
> +	vfree(ha->optrom_buffer);
> +	ha->optrom_buffer = NULL;
> +	ha->optrom_state = QLA_SWAITING;
> +	mutex_unlock(&ha->optrom_mutex);
> +	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 returning a non-zero rval here cause a double completion?

If qla29xx_mpi_optrom_data() fails, rval is returned as an error code.
Since bsg_job_done() was already called above, the BSG caller
(fc_bsg_host_dispatch) might see the non-zero return value, treat it as a
dispatch failure, and call bsg_job_done() a second time on the same job.

Could this lead to a use-after-free in the block layer?

> +}
[ ... ]
> diff --git a/drivers/scsi/qla2xxx/qla_sup.c b/drivers/scsi/qla2xxx/qla_sup.c
> index ee7562bf8d383..2e5fde403442f 100644
> --- a/drivers/scsi/qla2xxx/qla_sup.c
> +++ b/drivers/scsi/qla2xxx/qla_sup.c
[ ... ]
> @@ -541,6 +541,123 @@ qla29xx_read_optrom_data(struct scsi_qla_host *vha, uint16_t reg_code,
[ ... ]
> +int
> +qla29xx_mpi_optrom_data(struct scsi_qla_host *vha, uint16_t opts, void *buf,
> +		       uint32_t offset, uint32_t length, enum qla29xx_mpi_optrom_op op)
> +{
[ ... ]
> +		if (op == QLA29XX_MPI_OP_DUMP) {
> +			options |= (BIT_0);
> +			options |= opts;
> +
> +			ql_log(ql_log_info, vha, 0x008b,
> +				"-> %s (DUMP): %#x <-(%#x words) (0x%x options) chunk (0x%x, 0x%x)\n",
> +				__func__, mpi_addr, burst, options, chunk_count,
> +				total_chunks);
> +
> +			memcpy(dcode, fwcode, burst * 4);
> +
> +			rval = qla29xx_load_dump_mpi(vha, options, mpi_addr, burst,
> +						     optrom_dma);

[Severity: High]
And similarly in qla29xx_mpi_optrom_data(), OP_DUMP maps to a device write by
setting BIT_0 and copying the host buffer to the DMA buffer before calling
qla29xx_load_dump_mpi().

Should this be OP_LOAD instead?

> +			if (rval) {
> +				ql_log(ql_log_fatal, vha, 0x0098,
> +					"Failed dump mpi firmware.\n");
> +				goto free_buf;
> +			}
[ ... ]

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