Re: [PATCH v3 54/88] 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Payload truncation and loss of vendor response on error paths in BSG handlers.
- [High] Out-of-bounds read of `vendor_cmd` array and other flexible fields due to insufficient `request_len` validation in the BSG transport and qla2xxx driver.
--

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

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

This commit removes the conditional check around bsg_job_done() in
qla28xx_validate_flash_image() to ensure the BSG job is always completed.
Since the function unconditionally returns QLA_SUCCESS, the transport layer
will not complete the job, so the driver must do it to prevent resource
leaks on error paths.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index e7739cead967a..0c9174d6c887a 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c
> @@ -3782,9 +3782,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);
>  
>  	return QLA_SUCCESS;
>  }

[Severity: High]
This is a pre-existing issue, but other BSG handlers seem to share the
same flawed error-handling pattern that was just fixed here. For example, 
in drivers/scsi/qla2xxx/qla_bsg.c:qla2x00_mailbox_passthru():

        if (!ret)
                bsg_job_done(bsg_job, bsg_reply->result, 
                             bsg_reply->reply_payload_rcv_len);
        kfree(req_data);
        return ret;

And similarly in qla2x00_manage_host_stats():

        if (!ret)
                bsg_job_done(bsg_job, bsg_reply->result,
                             bsg_reply->reply_payload_rcv_len);
        return ret;

When ret != 0, bsg_job_done() is skipped and the error code is returned
to the transport. Does this cause the transport's error path to force
reply_payload_rcv_len = 0, discarding the mbx_out registers and vendor 
status?

If so, user space applications relying on the BSG vendor response 
payload to diagnose command failures might receive truncated payloads.

[Severity: High]
This is also a pre-existing issue, but could there be an out-of-bounds
read of the vendor_cmd array due to insufficient request_len validation?

In drivers/scsi/scsi_transport_fc.c:fc_bsg_host_dispatch(), the switch 
statement updates cmdlen but appears to skip bounds-checking 
job->request_len against it:

        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)) {

Then, down in drivers/scsi/qla2xxx/qla_bsg.c:qla24xx_bsg_request(), the 
driver blindly accesses vendor_cmd[0] before checking msgcode or 
request_len:

        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 in qla2x00_process_loopback(), it accesses vendor_cmd[1] and [2]:

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

If a privileged user sends a BSG command with a request_len smaller than 
the expected size, could this lead to an out-of-bounds read past the 
allocated buffer?

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