Re: [PATCH v6 54/56] scsi: qla2xxx: Fix BSG job leak on validate flash image error path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check on `request_len` allows out-of-bounds heap read when accessing the variable-length `vendor_cmd` array from user-supplied BSG requests.
--

--- Patch [54]: [PATCH v6 54/56] scsi: qla2xxx: Fix BSG job leak on validate flash image error path ---
commit 9de1723bd73d3b6f0fef869a49941dbe6b59c23b
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Fix BSG job leak on validate flash image error path

qla28xx_validate_flash_image() returns QLA_SUCCESS (0) unconditionally,
telling the FC BSG transport that the driver owns and will complete the
request. But bsg_job_done() was guarded by "if (!rval)", so on the error
path neither the driver nor the transport completes the job, leaking
resources. This commit removes the guard and always calls bsg_job_done().

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index 4a9cf8da67a6b..76c57e574c220 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -3796,9 +3796,8 @@ static int qla28xx_validate_flash_image(struct bsg_job *bsg_job)
>  	bsg_reply->result = DID_OK << 16;
>  	bsg_reply->reply_payload_rcv_len = 0;
>  	bsg_job->reply_len = sizeof(struct fc_bsg_reply);
> -	if (!rval)
> -		bsg_job_done(bsg_job, bsg_reply->result,
> -			     bsg_reply->reply_payload_rcv_len);
> +	bsg_job_done(bsg_job, bsg_reply->result,
> +		     bsg_reply->reply_payload_rcv_len);

[Severity: High]
This is a pre-existing issue, but does this BSG ioctl path allow an
out-of-bounds heap read if userspace provides a maliciously small request_len?

When a process with CAP_SYS_RAWIO issues a BSG ioctl specifying
FC_BSG_HST_VENDOR with a small request_len (e.g., 4 bytes), the transport
layer expands cmdlen but never re-validates job->request_len:

drivers/scsi/scsi_transport_fc.c:fc_bsg_host_dispatch() {
    ...
    cmdlen += sizeof(struct fc_bsg_host_vendor);
    if ((shost->hostt->vendor_id == 0L) ||
        (bsg_request->rqst_data.h_vendor.vendor_id !=
         shost->hostt->vendor_id)) {
    ...
}

This leads to an immediate out-of-bounds read of vendor_id. Later in the
qla2xxx driver, the handler unconditionally reads vendor_cmd[0] before
verifying the message code:

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)
    ...
}

And specifically for the validate flash image command, it blindly reads
vendor_cmd[1] without verifying the request buffer is large enough:

drivers/scsi/qla2xxx/qla_bsg.c:qla28xx_do_validate_flash_image() {
    ...
    img_idx = bsg_request->rqst_data.h_vendor.vendor_cmd[1];
    ...
}

Should there be a bounds check on request_len in fc_bsg_host_dispatch() or
the driver to prevent this?

>  
>  	return QLA_SUCCESS;
>  }

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