Re: [PATCH v3 85/88] scsi: qla2xxx: Validate BSG request_len before reading vendor_cmd[]

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The newly added bounds checks incorrectly use `sizeof(struct fc_bsg_request)` to calculate the required payload size, artificially rejecting valid, properly sized userspace requests and breaking the UAPI.

Pre-existing issues:
- [High] The patch fails to add `request_len` validation to `qla84xx_mgmt_cmd` and `qla24xx_iidma`, leaving them vulnerable to out-of-bounds heap reads.
- [High] The transport layer `fc_bsg_host_dispatch()` does not actually guarantee that `request_len` covers `vendor_id`, leading to a heap out-of-bounds read before reaching the driver.
- [High] Leaked `vha` and `qpair` busy reference counters in the `qlafx00_mgmt_cmd` error path.
- [High] Concurrent BSG requests to QLFC_FCP_PRIO_SET_CONFIG and QLFC_FCP_PRIO_GET_CONFIG can race on the unprotected `ha->fcp_prio_cfg` pointer, leading to memory leaks, data corruption, Use-After-Free, or NULL pointer dereferences.
- [High] Concurrent loopback BSG requests can cause a TOCTOU race on the loopback state and wait incorrectly on a shared completion variable.
--

--- Patch [85]: [PATCH v3 85/88] scsi: qla2xxx: Validate BSG request_len before reading vendor_cmd[] ---
commit a39aea7848cc6caa6a7176a258e927c5587f88e5
Author: Nilesh Javali <njavali@marvell.com>
Subject: scsi: qla2xxx: Validate BSG request_len before reading vendor_cmd[]

This commit attempts to fix out-of-bounds reads in the transport handlers
by adding a central length guard in qla2x00_process_vendor_specific and
additional request length checks in various sub-handlers.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index cb7227298b24b..e671c3de8c058 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -160,6 +160,12 @@ 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;
> +	}
> +

[Severity: Critical]
Does this inflate the requirement to 28 bytes when only 20 bytes are actually
needed to safely read vendor_cmd[1]?

[Severity: High]
This is a pre-existing issue, but looking at the rest of this function,
can concurrent requests race on the unprotected ha->fcp_prio_cfg pointer?

If two concurrent requests execute the set config and get config operations,
could one thread free the pointer and set it to null while the other is
copying from it to userspace, leading to a null pointer dereference or
use-after-free?

[ ... ]

> @@ -758,6 +764,10 @@ 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));

[Severity: High]
This isn't a bug introduced by this patch, but further down in this function,
concurrent loopback requests perform a lockless read of the loopback state.

Can multiple threads bypass the check, issue hardware commands, and wait on
the same shared ha->dcbx_comp completion variable? Since complete() only wakes
one thread, wouldn't the other time out and trigger an unjustified abort of
the hardware?

[ ... ]

> @@ -2397,6 +2427,11 @@ 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 */

[Severity: High]
This isn't a bug introduced by this patch, but looking at the error path for
qla2x00_start_sp() in this function, the code manually calls mempool_free()
instead of qla2x00_rel_sp().

Does this skip decrementing the busy reference counters for the hardware state
and queue pair, permanently leaking them and preventing the adapter from being
safely unloaded?

[ ... ]

> @@ -3318,6 +3353,13 @@ 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;
> +	}
> +

[Severity: Critical]
Does this central check mathematically overcalculate the required payload size
and artificially reject legacy tools?

The size of struct fc_bsg_request is 20 bytes due to the union, but the
vendor_cmd flexible array starts at offset 12. Wouldn't a 16-byte payload
safely cover vendor_cmd[0]? It appears enforcing a minimum of 24 bytes here
will break backwards compatibility for legitimate userspace tools sending
valid 16-byte requests.

[Severity: High]
This is a pre-existing issue, but while we are adding bounds checks, it looks
like qla84xx_mgmt_cmd() and qla24xx_iidma() might also need validation.

They cast the payload starting at offset 20 to structs like qla_bsg_a84_mgmt
or qla_port_param and access fields up to offset 39 without verifying if the
request length is sufficiently large. Could this lead to out-of-bounds heap
reads?

[Severity: High]
This is also a pre-existing issue, but looking further up the stack in the
generic transport layer, does fc_bsg_host_dispatch() in
drivers/scsi/scsi_transport_fc.c actually guarantee that the request length
covers the vendor_id field?

It increments cmdlen but accesses vendor_id immediately without verifying if
the user supplied at least 8 bytes, which seems to allow an out-of-bounds read
before the request even reaches the driver.

[ ... ]

> @@ -3461,8 +3503,11 @@ qla24xx_bsg_request(struct bsg_job *bsg_job)
>  	}
>  
>  	/* Disable port will bring down the chip, allow enable command */
> -	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)
> +	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))
>  		goto skip_chip_chk;

[Severity: Critical]
Similar to the check in qla2x00_process_vendor_specific(), does this fail to
bypass the chip checks for valid, tightly-packed 16-byte requests because it
inflates the required length to 24 bytes?

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