Re: [PATCH] scsi: aacraid: fix DMA mapping leak in aac_send_raw_srb()

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

New issues:
- [High] DMA buffers are read by the CPU before they are unmapped, causing stale data to be copied to user space on non-coherent architectures.
- [Medium] The driver uses 0 as a sentinel value to track unmapped DMA addresses, permanently leaking valid mappings at address 0.

Pre-existing issues:
- [High] The driver does not check the return value of `dma_map_single()`, causing invalid error addresses to be improperly unmapped.
- [High] Permanent leak of heap memory and DMA mappings when the operation is interrupted by a signal.
- [High] Uninitialized stack variables are passed to `copy_to_user()` when `sg.count == 0`.
- [High] The driver leaks uninitialized kernel heap memory to user space when the hardware transfers less data than requested.
- [High] Use-After-Free in sync_mode when `wait_for_completion_interruptible` is interrupted by a signal.
--

--- Patch [1]: [PATCH] scsi: aacraid: fix DMA mapping leak in aac_send_raw_srb() ---
commit 863e921c48579ca39e3e4f977e819659b4711e60
Author: Ivy Lopez <[email protected]>

scsi: aacraid: fix DMA mapping leak in aac_send_raw_srb()

aac_send_raw_srb() maps each scatter/gather entry for DMA via
dma_map_single() across five separate code paths, depending on the
adapter type and SG format. This commit introduces tracking for DMA
addresses in a new sg_addr[] array and unmaps them in the cleanup
path to prevent permanent resource exhaustion.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=220504

> diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
> index bd82aeb679aed..d4dffd6f8af0b 100644
> --- a/drivers/scsi/aacraid/commctrl.c
> +++ b/drivers/scsi/aacraid/commctrl.c
> @@ -492,6 +492,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
>  	u32 data_dir;
>  	void __user *sg_user[HBA_MAX_SG_EMBEDDED];
>  	void *sg_list[HBA_MAX_SG_EMBEDDED];
> +	dma_addr_t sg_addr[HBA_MAX_SG_EMBEDDED];
>  	u32 sg_count[HBA_MAX_SG_EMBEDDED];
>  	u32 sg_indx = 0;
[ ... ]
> @@ -517,6 +518,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
>  	}
>  
>  	memset(sg_list, 0, sizeof(sg_list)); /* cleanup may take issue */
> +	memset(sg_addr, 0, sizeof(sg_addr)); /* mark all entries unmapped */

[Severity: Medium]
Is it safe to use 0 as a sentinel value here?

On many platforms and IOMMUs, 0 is a valid DMA address. If dma_map_single()
successfully returns 0, the cleanup loop's check of if (sg_addr[i]) will
evaluate to false, skipping dma_unmap_single() and leaking that mapping.

>  	if(copy_from_user(&fibsize, &user_srb->count,sizeof(u32))){
[ ... ]
> @@ -690,6 +692,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
>  			}
>  			addr = dma_map_single(&dev->pdev->dev, p, sg_count[i],
>  					      data_dir);
> +			sg_addr[i] = addr;

[Severity: High]
This is a pre-existing issue, but does this code properly validate the DMA
mapping?

If dma_map_single() fails (e.g., due to IOMMU space exhaustion), it typically
returns an error address. Since this is never checked using
dma_mapping_error(), this non-zero error address is saved and later passed
directly to dma_unmap_single() during cleanup.

>  			hbacmd->sge[i].addr_hi = cpu_to_le32((u32)(addr>>32));
[ ... ]
> @@ -986,8 +989,12 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)

[Severity: High]
This is a pre-existing issue, but does aac_send_raw_srb() leak uninitialized
kernel heap memory to user space before reaching the cleanup path?

The scatter-gather buffers are allocated via kmalloc() without __GFP_ZERO.
These buffers may remain uninitialized if SRB_DataOut is not set. If the
hardware completes the command but transfers less data than requested, the
subsequent copy_to_user() will unconditionally copy the remaining
uninitialized bytes to user space.

[Severity: High]
This is also a pre-existing issue, but if sg.count is 0 and flags & SRB_DataIn
is set, does the copy loop read uninitialized stack memory?

When sg.count is 0, the buffer allocation loops are skipped, leaving sg_indx
as 0, and both the sg_user and sg_count arrays uninitialized. Because the
copy loop right before this cleanup block iterates with i <= sg_indx, it
executes once for i=0 and calls copy_to_user() with stack garbage.

>  cleanup:
>  	kfree(user_srbcmd);
>  	if (rcode != -ERESTARTSYS) {

[Severity: High]
This is a pre-existing issue, but does skipping this block on -ERESTARTSYS
cause a permanent memory and DMA mapping leak?

Since sg_list and sg_addr are local stack variables, they are destroyed
when the function returns. If aac_fib_send() is interrupted by a signal,
the memory and mappings are left stranded with no way to free them when
the hardware eventually completes the command.

> -		for (i = 0; i <= sg_indx; i++)
> +		for (i = 0; i <= sg_indx; i++) {
> +			if (sg_addr[i])
> +				dma_unmap_single(&dev->pdev->dev, sg_addr[i],
> +						 sg_count[i], data_dir);

[Severity: High]
Can this result in stale data being copied to user space on non-coherent
architectures?

The CPU reads from the DMA buffers earlier during copy_to_user(). Because
dma_unmap_single() performs cache invalidation on non-coherent architectures,
placing it here in the cleanup block means it executes after the CPU has
already read the data, returning stale data to user space.

>  			kfree(sg_list[i]);
> +		}
>  		aac_fib_complete(srbfib);
>  		aac_fib_free(srbfib);

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk here if
aac_fib_send() operates in sync_mode and is interrupted by a signal?

If wait_for_completion_interruptible() returns -EFAULT on a signal, rcode
won't equal -ERESTARTSYS, causing aac_fib_free() to run here. However,
dev->sync_fib might still point to this freed fib, which
aac_src_intr_message() could later attempt to access when the hardware
completes the synchronous command.

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1