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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: qla2xxx: unchecked vendor_cmd[0] read in qla24xx_bsg_request()
- [High] scsi: qla2xxx: out-of-bounds vendor_cmd[] reads on short BSG requests
--

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

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

This commit reverts commit 4cf38dd9465736141263ebb63375868311a0ec81 to
restore FC_BSG_HST_VENDOR handling for the Marvell qaucli management utility,
which passes a request_len smaller than sizeof(struct fc_bsg_request) plus
the vendor_cmd words checked by that commit.

> 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]);

[Severity: High]
Does removing all of the request_len checks reintroduce out-of-bounds heap
reads of bsg_request->rqst_data.h_vendor.vendor_cmd[] when userspace submits
a short FC_BSG_HST_VENDOR request?

In bsg_transport_sg_io_fn(), job->request is allocated with the exact
user-supplied hdr->request_len:

block/bsg-lib.c:bsg_transport_sg_io_fn() {
    ...
    job->request_len = hdr->request_len;
    job->request = memdup_user(uptr64(hdr->request), hdr->request_len);
    ...
}

And fc_bsg_host_dispatch() only verifies that job->request_len is at least
sizeof(uint32_t) (4 bytes) before calling into qla24xx_bsg_request() ->
qla2x00_process_vendor_specific().

In struct fc_bsg_request, rqst_data is a union whose total size is 16 bytes
(sized by struct fc_bsg_host_ct), making sizeof(struct fc_bsg_request) 20
bytes. However, struct fc_bsg_host_vendor only has an 8-byte vendor_id before
the flexible array vendor_cmd[]:

include/uapi/scsi/scsi_bsg_fc.h:
    struct fc_bsg_host_vendor {
        __u64 vendor_id;
        __u32 vendor_cmd[];
    };

So offsetof(struct fc_bsg_request, rqst_data.h_vendor.vendor_cmd) is 12
bytes, meaning vendor_cmd[0] is at bytes 12..15 and vendor_cmd[1] is at bytes
16..19 (both within a 20-byte struct fc_bsg_request). Commit 4cf38dd94657
rejected 20-byte qaucli requests because it checked against
sizeof(struct fc_bsg_request) + N * sizeof(uint32_t) (20 + 4*N) rather than
offsetof(struct fc_bsg_request, rqst_data.h_vendor.vendor_cmd) +
N * sizeof(uint32_t) (12 + 4*N).

Without bounds checks anchored at offsetof(struct fc_bsg_request,
rqst_data.h_vendor.vendor_cmd), short requests read past the end of
bsg_job->request in:
- qla2x00_process_vendor_specific() reading vendor_cmd[0]
- 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()
  reading vendor_cmd[1]
- qla2x00_process_loopback() reading vendor_cmd[1] and vendor_cmd[2]
- qlafx00_mgmt_cmd() overlaying a 36-byte struct qla_mt_iocb_rqst_fx00 at
  &vendor_cmd[1]

> @@ -3529,11 +3487,8 @@ qla24xx_bsg_request(struct bsg_job *bsg_job)
>  	}
>  
>  	/* Disable port will bring down the chip, allow enable command */
> -	if (bsg_request->msgcode == FC_BSG_HST_VENDOR &&
> -	    bsg_job->request_len >=
> -		sizeof(struct fc_bsg_request) + sizeof(uint32_t) &&
> -	    (bsg_request->rqst_data.h_vendor.vendor_cmd[0] == QL_VND_MANAGE_HOST_PORT ||
> -	     bsg_request->rqst_data.h_vendor.vendor_cmd[0] == QL_VND_GET_HOST_STATS))
> +	if (bsg_request->rqst_data.h_vendor.vendor_cmd[0] == QL_VND_MANAGE_HOST_PORT ||
> +	    bsg_request->rqst_data.h_vendor.vendor_cmd[0] == QL_VND_GET_HOST_STATS)
>  		goto skip_chip_chk;

[Severity: High]
In qla24xx_bsg_request(), does dropping the bsg_request->msgcode ==
FC_BSG_HST_VENDOR check and length check cause bsg_request->rqst_data.h_vendor.vendor_cmd[0]
to be read unconditionally on every BSG request, including non-vendor
requests?

In struct fc_bsg_request, rqst_data is a union:

include/uapi/scsi/scsi_bsg_fc.h:
    struct fc_bsg_request {
        __u32 msgcode;
        union {
            struct fc_bsg_host_add_rport    h_addrport;
            struct fc_bsg_host_del_rport    h_delrport;
            struct fc_bsg_host_els          h_els;
            struct fc_bsg_host_ct           h_ct;
            struct fc_bsg_host_vendor       h_vendor;

            struct fc_bsg_rport_els         r_els;
            struct fc_bsg_rport_ct          r_ct;
        } rqst_data;
    } __attribute__((packed));

Because rqst_data.h_vendor.vendor_cmd[0] is at byte offset 12..15:

1. For legitimate non-vendor BSG requests smaller than 16 bytes, such as
   FC_BSG_RPT_ELS (where msgcode + struct fc_bsg_rport_els is 5 bytes) or
   FC_BSG_HST_ELS_NOLOGIN (where msgcode + struct fc_bsg_host_els is 8 bytes),
   reading vendor_cmd[0] reads past the end of the bsg_job->request buffer
   allocated by memdup_user().

2. For FC_BSG_HST_CT and FC_BSG_RPT_CT requests, bytes 12..15 alias
   rqst_data.h_ct.preamble_word1 and rqst_data.r_ct.preamble_word2. If that
   field happens to equal QL_VND_MANAGE_HOST_PORT (0x26) or
   QL_VND_GET_HOST_STATS (0x24), qla24xx_bsg_request() jumps to skip_chip_chk
   and bypasses the port_isolated, qla2x00_chip_is_down(vha), and
   PFLG_DRIVER_REMOVING checks:

drivers/scsi/qla2xxx/qla_bsg.c:qla24xx_bsg_request() {
    ...
    if (bsg_request->rqst_data.h_vendor.vendor_cmd[0] == QL_VND_MANAGE_HOST_PORT ||
        bsg_request->rqst_data.h_vendor.vendor_cmd[0] == QL_VND_GET_HOST_STATS)
        goto skip_chip_chk;

    if (vha->hw->flags.port_isolated) {
        bsg_reply->result = DID_ERROR;
        /* operation not permitted */
        return -EPERM;
    }

    if (qla2x00_chip_is_down(vha)) {
        ...
        return -EBUSY;
    }

    if (test_bit(PFLG_DRIVER_REMOVING, &vha->pci_flags)) {
        SET_DID_STATUS(bsg_reply->result, DID_ERROR);
        return -EIO;
    }

skip_chip_chk:
    ...
}

Should qla24xx_bsg_request() retain the bsg_request->msgcode ==
FC_BSG_HST_VENDOR check along with a check that bsg_job->request_len >=
offsetof(struct fc_bsg_request, rqst_data.h_vendor.vendor_cmd) +
sizeof(uint32_t)?

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