Re: [PATCH v4 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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing bounds checking on user-provided bsg_job->request_len allows out-of-bounds reads from the kernel heap.
- [High] DMA mapping leaks and invalid unmaps occur on failure paths when mapping payloads via `dma_map_sg()`.
--

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

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

This commit fixes a BSG job leak in qla28xx_validate_flash_image() by
ensuring bsg_job_done() is called unconditionally. Since the function
returns QLA_SUCCESS, the FC BSG transport will not complete the job,
requiring the driver to do so to prevent the request from dangling.

> diff --git a/drivers/scsi/qla2xxx/qla_bsg.c b/drivers/scsi/qla2xxx/qla_bsg.c
> index 3bf3a7b96eb50..2f73c9af418d7 100644
> --- a/drivers/scsi/qla2xxx/qla_bsg.c
> +++ b/drivers/scsi/qla2xxx/qla_bsg.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential bounds
checking issue when handling user-provided bsg_job->request_len:

drivers/scsi/scsi_transport_fc.c:fc_bsg_host_dispatch() {
    ...
	case FC_BSG_HST_VENDOR:
		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)) {
    ...
}

Is it possible for this to result in an out-of-bounds read from the kernel heap?

The request_len is not validated against the new cmdlen size before accessing
vendor_id. If a user-space program creates a BSG request with a small
request_len but sets msgcode to FC_BSG_HST_VENDOR, would the transport layer
read vendor_id out of bounds?

This same pattern appears in the LLDD handler:

drivers/scsi/qla2xxx/qla_bsg.c:qla2x00_process_vendor_specific() {
    ...
	switch (bsg_request->rqst_data.h_vendor.vendor_cmd[0]) {
	case QL_VND_LOOPBACK:
		return qla2x00_process_loopback(bsg_job);
    ...
}

Does this function blindly access flexible array elements assuming they are
within the bounds of the user-provided request length?


[Severity: High]
This is also a pre-existing issue, but I found potential DMA mapping leaks and
invalid unmaps on failure paths when mapping payloads via dma_map_sg():

drivers/scsi/qla2xxx/qla_bsg.c:qla2x00_process_els() {
    ...
	req_sg_cnt =
		dma_map_sg(&ha->pdev->dev, bsg_job->request_payload.sg_list,
		bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);
	if (!req_sg_cnt) {
		dma_unmap_sg(&ha->pdev->dev, bsg_job->request_payload.sg_list,
		    bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);
		rval = -ENOMEM;
		goto done_free_fcport;
	}

	rsp_sg_cnt = dma_map_sg(&ha->pdev->dev, bsg_job->reply_payload.sg_list,
		bsg_job->reply_payload.sg_cnt, DMA_FROM_DEVICE);
        if (!rsp_sg_cnt) {
		dma_unmap_sg(&ha->pdev->dev, bsg_job->reply_payload.sg_list,
		    bsg_job->reply_payload.sg_cnt, DMA_FROM_DEVICE);
		rval = -ENOMEM;
		goto done_free_fcport;
	}
    ...
}

If dma_map_sg() fails for req_sg_cnt, does it incorrectly attempt to unmap the
failed list?

Additionally, on reply payload mapping failure, it jumps to done_free_fcport,
which bypasses the done_unmap_sg label. Does this permanently leak the
successfully mapped request payload?

A similar issue appears here:

drivers/scsi/qla2xxx/qla_bsg.c:qla2x00_process_ct() {
    ...
	req_sg_cnt =
		dma_map_sg(&ha->pdev->dev, bsg_job->request_payload.sg_list,
			bsg_job->request_payload.sg_cnt, DMA_TO_DEVICE);
	if (!req_sg_cnt) {
    ...
		goto done;
	}

	rsp_sg_cnt = dma_map_sg(&ha->pdev->dev, bsg_job->reply_payload.sg_list,
		bsg_job->reply_payload.sg_cnt, DMA_FROM_DEVICE);
	if (!rsp_sg_cnt) {
    ...
		goto done;
	}
    ...
}

If the reply payload mapping fails, it jumps directly to done, skipping the
done_unmap_sg block. Can this exhaust IOMMU space or DMA pools over time by
leaking the mapped request payload?

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