Re: [PATCH 03/14] Revert "scsi: qla2xxx: Validate BSG request_len before reading vendor_cmd[]"

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: qla2xxx: unchecked request_len in qla2x00_process_vendor_specific()
--

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

Revert "scsi: qla2xxx: Validate BSG request_len before reading vendor_cmd[]"

This commit reverts commit 4cf38dd9465736141263ebb63375868311a0ec81, which
added bsg_job->request_len validation before reading elements of
vendor_cmd[] in drivers/scsi/qla2xxx/qla_bsg.c, because those checks
rejected legitimate requests from the qaucli management utility.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index ab559048dbb82..19a514a153aef 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -160,12 +160,6 @@ qla24xx_proc_fcp_prio_cfg_cmd(struct bsg_job *bsg_job)
>  		goto exit_fcp_prio_cfg;
>  	}
>  
> -	if (bsg_job->request_len <
> -	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t)) {
> -		ret = -EINVAL;
> -		goto exit_fcp_prio_cfg;
> -	}
> -
>  	/* Get the sub command */
>  	oper = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
>  
> @@ -764,10 +758,6 @@ qla2x00_process_loopback(struct bsg_job *bsg_job)
>  		return -EIO;
>  	}
>  
> -	if (bsg_job->request_len <
> -	    sizeof(struct fc_bsg_request) + 3 * sizeof(uint32_t))
> -		return -EINVAL;
> -
>  	memset(&elreq, 0, sizeof(elreq));
>  
>  	elreq.req_sg_cnt = dma_map_sg(&ha->pdev->dev,
> @@ -1000,10 +990,6 @@ qla84xx_reset(struct bsg_job *bsg_job)
>  		return -EINVAL;
>  	}
>  
> -	if (bsg_job->request_len <
> -	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t))
> -		return -EINVAL;
> -
>  	flag = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
>  
>  	rval = qla84xx_reset_chip(vha, flag == A84_ISSUE_RESET_DIAG_FW);
> @@ -1048,10 +1034,6 @@ qla84xx_updatefw(struct bsg_job *bsg_job)
>  		return -EINVAL;
>  	}
>  
> -	if (bsg_job->request_len <
> -	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t))
> -		return -EINVAL;
> -
>  	sg_cnt = dma_map_sg(&ha->pdev->dev, bsg_job->request_payload.sg_list,
>  		bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);
>  	if (!sg_cnt) {
> @@ -1529,15 +1511,9 @@ qla2x00_read_optrom(struct bsg_job *bsg_job)
>  	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;
> -	uint32_t start;
> +	uint32_t start = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
>  	int rval = 0;
>  
> -	if (bsg_job->request_len <
> -	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t))
> -		return -EINVAL;
> -
> -	start = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
> -
>  	if (ha->flags.nic_core_reset_hdlr_active)
>  		return -EBUSY;
>  
> @@ -1580,15 +1556,9 @@ qla2x00_update_optrom(struct bsg_job *bsg_job)
>  	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;
> -	uint32_t start;
> +	uint32_t start = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
>  	int rval = 0;
>  
> -	if (bsg_job->request_len <
> -	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t))
> -		return -EINVAL;
> -
> -	start = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
> -
>  	mutex_lock(&ha->optrom_mutex);
>  	rval = qla2x00_optrom_setup(bsg_job, vha, start, 1);
>  	if (rval) {
> @@ -2453,11 +2423,6 @@ qlafx00_mgmt_cmd(struct bsg_job *bsg_job)
>  	struct fc_port *fcport;
>  	char  *type = "FC_BSG_HST_FX_MGMT";
>  
> -	if (bsg_job->request_len <
> -	    sizeof(struct fc_bsg_request) + sizeof(uint32_t) +
> -	    sizeof(struct qla_mt_iocb_rqst_fx00))
> -		return -EINVAL;
> -
>  	/* Copy the IOCB specific information */
>  	piocb_rqst = (struct qla_mt_iocb_rqst_fx00 *)
>  	    &bsg_request->rqst_data.h_vendor.vendor_cmd[1];
> @@ -3379,13 +3344,6 @@ qla2x00_process_vendor_specific(struct scsi_qla_host *vha, struct bsg_job *bsg_j
>  {
>  	struct fc_bsg_request *bsg_request = bsg_job->request;
>  
> -	if (bsg_job->request_len <
> -	    sizeof(struct fc_bsg_request) + sizeof(uint32_t)) {
> -		ql_log(ql_log_warn, vha, 0x7000,
> -		       "BSG request too small for vendor cmd.\n");
> -		return -EINVAL;
> -	}
> -
>  	ql_dbg(ql_dbg_edif, vha, 0x911b, "%s FC_BSG_HST_VENDOR cmd[0]=0x%x\n",
>  	    __func__, bsg_request->rqst_data.h_vendor.vendor_cmd[0]);
>  

[ ... ]

> @@ -3842,10 +3797,6 @@ static int qla28xx_validate_flash_image(struct bsg_job *bsg_job)
>  	if (!IS_QLA28XX(ha) || vha->vp_idx != 0)
>  		return -EPERM;
>  
> -	if (bsg_job->request_len <
> -	    sizeof(struct fc_bsg_request) + 2 * sizeof(uint32_t))
> -		return -EINVAL;
> -
>  	mutex_lock(&ha->optrom_mutex);
>  	rval = qla28xx_do_validate_flash_image(bsg_job, &state);
>  	if (rval)

[Severity: High]
Does removing all of these bsg_job->request_len checks without a replacement
reintroduce out-of-bounds heap reads of
bsg_request->rqst_data.h_vendor.vendor_cmd[]?

In block/bsg-lib.c, bsg_transport_sg_io_fn() allocates job->request with the
exact user-supplied hdr->request_len:

    job->request = memdup_user(uptr64(hdr->request), hdr->request_len);

In drivers/scsi/scsi_transport_fc.c, fc_bsg_host_dispatch() only checks
job->request_len < sizeof(uint32_t) before matching FC_BSG_HST_VENDOR and
checking vendor_id, without verifying that job->request_len covers any
elements of the flexible array rqst_data.h_vendor.vendor_cmd[].

Without length checks in drivers/scsi/qla2xxx/qla_bsg.c, a short
FC_BSG_HST_VENDOR request (for example 12 or 16 bytes) can read past the end
of bsg_job->request in:

- qla2x00_process_vendor_specific(), which reads vendor_cmd[0] (bytes 12..15)
- qla24xx_proc_fcp_prio_cfg_cmd(), qla84xx_reset(), qla84xx_updatefw(),
  qla2x00_read_optrom(), qla2x00_update_optrom(), and
  qla28xx_validate_flash_image() -> qla28xx_do_validate_flash_image(), which
  read vendor_cmd[1] (bytes 16..19)
- qla2x00_process_loopback(), which reads vendor_cmd[1] and vendor_cmd[2]
  (bytes 16..23) later in the function:

	elreq.options = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
	elreq.iteration_count =
	    bsg_request->rqst_data.h_vendor.vendor_cmd[2];

- qlafx00_mgmt_cmd(), which overlays and reads the 32-byte
  struct qla_mt_iocb_rqst_fx00 starting at vendor_cmd[1] (bytes 16..47)

Commit 4cf38dd9465736141263ebb63375868311a0ec81 over-estimated the header
size by 8 bytes because sizeof(struct fc_bsg_request) is 20 bytes (sized by
the 16-byte h_ct union member), whereas
offsetof(struct fc_bsg_request, rqst_data.h_vendor.vendor_cmd) is 12 bytes.

Could these checks be updated to use
offsetof(struct fc_bsg_request, rqst_data.h_vendor.vendor_cmd) as the base
offset instead of reverting them completely, so that qaucli requests succeed
while still preventing out-of-bounds reads of vendor_cmd[]?

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