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

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

[Ivy Lopez]

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 (native HBA, 64-bit host SG, 32-bit host
SG, and two legacy formats). None of these mappings are ever undone:
there is no dma_unmap_single() call anywhere in the file, on the
success path or any of the error paths that funnel through the
single cleanup label.

Every FSACTL_SEND_RAW_SRB ioctl that submits at least one SG entry
therefore leaks that many DMA mappings permanently. Under an IOMMU
or SWIOTLB this is a genuinely exhaustible resource: sustained use
(e.g. periodic smartctl -d aacraid,... polling) eventually drives
new DMA mappings to fail, surfacing as intermittent I/O failures
(aac_fib_send failing with -ENOMEM) and, left long enough, adapter
resets and system instability.

Fix this by tracking the DMA address returned from each of the five
dma_map_single() calls in a new per-entry array (sg_addr[]), zero
initialized alongside the existing sg_list[] array. In cleanup,
unmap each entry before freeing its backing memory, using the
sg_addr[i] != 0 check as a guard: some cleanup paths are reached
after sg_indx has been advanced for entry i but before that entry's
dma_map_single() call runs (e.g. a copy_from_user() failure), so
not every index in the cleanup loop was actually mapped.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=220504
Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
 drivers/scsi/aacraid/commctrl.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index bd82aeb679ae..d4dffd6f8af0 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;
 	u32 byte_count = 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 */
 	if(copy_from_user(&fibsize, &user_srb->count,sizeof(u32))){
 		dprintk((KERN_DEBUG"aacraid: Could not copy data size from user\n"));
 		rcode = -EFAULT;
@@ -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;
 			hbacmd->sge[i].addr_hi = cpu_to_le32((u32)(addr>>32));
 			hbacmd->sge[i].addr_lo = cpu_to_le32(
 						(u32)(addr & 0xffffffff));
@@ -752,7 +755,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;
 				psg->sg[i].addr[0] = cpu_to_le32(addr & 0xffffffff);
 				psg->sg[i].addr[1] = cpu_to_le32(addr>>32);
 				byte_count += sg_count[i];
@@ -808,7 +811,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;
 				psg->sg[i].addr[0] = cpu_to_le32(addr & 0xffffffff);
 				psg->sg[i].addr[1] = cpu_to_le32(addr>>32);
 				byte_count += sg_count[i];
@@ -865,7 +868,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				addr = dma_map_single(&dev->pdev->dev, p,
 						      usg->sg[i].count,
 						      data_dir);
-
+				sg_addr[i] = addr;
 				psg->sg[i].addr = cpu_to_le32(addr & 0xffffffff);
 				byte_count += usg->sg[i].count;
 				psg->sg[i].count = cpu_to_le32(sg_count[i]);
@@ -905,7 +908,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;
 				psg->sg[i].addr = cpu_to_le32(addr);
 				byte_count += sg_count[i];
 				psg->sg[i].count = cpu_to_le32(sg_count[i]);
@@ -986,8 +989,12 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 cleanup:
 	kfree(user_srbcmd);
 	if (rcode != -ERESTARTSYS) {
-		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);
 			kfree(sg_list[i]);
+		}
 		aac_fib_complete(srbfib);
 		aac_fib_free(srbfib);
 	}
-- 
2.55.0

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

[sashiko-bot]

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 <skunkolee@gmail.com>

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/20260809033449.286233-1-skunkolee@gmail.com?part=1

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

[Ivy Lopez]

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 (native HBA, 64-bit host SG, 32-bit host
SG, and two legacy formats). None of these mappings are ever undone:
there is no dma_unmap_single() call anywhere in the file, on the
success path or any of the error paths that funnel through the
single cleanup label.

Every FSACTL_SEND_RAW_SRB ioctl that submits at least one SG entry
therefore leaks that many DMA mappings permanently. Under an IOMMU
or SWIOTLB this is a genuinely exhaustible resource: sustained use
(e.g. periodic smartctl -d aacraid,... polling) eventually drives
new DMA mappings to fail, surfacing as intermittent I/O failures
(aac_fib_send failing with -ENOMEM) and, left long enough, adapter
resets and system instability.

Fix this by tracking the DMA address returned from each of the five
dma_map_single() calls in a new per-entry array (sg_addr[]). Each
entry is initialized to DMA_MAPPING_ERROR and checked with
dma_mapping_error() immediately after mapping, bailing out to
cleanup on failure rather than using a possibly-error address.
Entries are unmapped exactly once: for SRB_DataIn (hardware writes
via DMA), each entry is unmapped immediately before its
copy_to_user() in the existing per-entry loop, since on
non-coherent architectures dma_unmap_single() performs the cache
invalidation needed before the CPU can safely read what the device
wrote; unmapping only later in cleanup, after that read, could
return stale pre-DMA-completion data to userspace. Entries handled
this way are marked DMA_MAPPING_ERROR again so cleanup does not
unmap them a second time. All other entries (SRB_DataOut-only, or
any entry on an error path that never reaches the DataIn copy loop)
are unmapped once in cleanup, guarded by the same sentinel check.

v1 of this patch used 0 as the "unmapped" sentinel and unmapped
every entry only in cleanup, after any copy_to_user() had already
read from it. Both were wrong: 0 is a valid DMA address on some
platforms (DMA_MAPPING_ERROR is ~(dma_addr_t)0, not 0), so a mapping
that legitimately returned address 0 would never be unmapped; and
unmapping only in cleanup meant SRB_DataIn transfers could read
stale, not-yet-cache-invalidated data on non-coherent architectures.
Both issues were caught in review by an automated reviewer
(Sashiko AI) on the v1 submission.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=220504
Signed-off-by: Ivy Lopez <skunkolee@gmail.com>
---
 drivers/scsi/aacraid/commctrl.c | 43 +++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index bd82aeb679ae..27482d98c6fd 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;
 	u32 byte_count = 0;
@@ -517,6 +518,8 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 	}
 
 	memset(sg_list, 0, sizeof(sg_list)); /* cleanup may take issue */
+	for (i = 0; i < HBA_MAX_SG_EMBEDDED; i++)
+		sg_addr[i] = DMA_MAPPING_ERROR; /* mark all entries unmapped */
 	if(copy_from_user(&fibsize, &user_srb->count,sizeof(u32))){
 		dprintk((KERN_DEBUG"aacraid: Could not copy data size from user\n"));
 		rcode = -EFAULT;
@@ -690,6 +693,11 @@ 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);
+			if (dma_mapping_error(&dev->pdev->dev, addr)) {
+				rcode = -ENOMEM;
+				goto cleanup;
+			}
+			sg_addr[i] = addr;
 			hbacmd->sge[i].addr_hi = cpu_to_le32((u32)(addr>>32));
 			hbacmd->sge[i].addr_lo = cpu_to_le32(
 						(u32)(addr & 0xffffffff));
@@ -752,7 +760,11 @@ 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);
-
+				if (dma_mapping_error(&dev->pdev->dev, addr)) {
+					rcode = -ENOMEM;
+					goto cleanup;
+				}
+				sg_addr[i] = addr;
 				psg->sg[i].addr[0] = cpu_to_le32(addr & 0xffffffff);
 				psg->sg[i].addr[1] = cpu_to_le32(addr>>32);
 				byte_count += sg_count[i];
@@ -808,7 +820,11 @@ 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);
-
+				if (dma_mapping_error(&dev->pdev->dev, addr)) {
+					rcode = -ENOMEM;
+					goto cleanup;
+				}
+				sg_addr[i] = addr;
 				psg->sg[i].addr[0] = cpu_to_le32(addr & 0xffffffff);
 				psg->sg[i].addr[1] = cpu_to_le32(addr>>32);
 				byte_count += sg_count[i];
@@ -865,7 +881,11 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 				addr = dma_map_single(&dev->pdev->dev, p,
 						      usg->sg[i].count,
 						      data_dir);
-
+				if (dma_mapping_error(&dev->pdev->dev, addr)) {
+					rcode = -ENOMEM;
+					goto cleanup;
+				}
+				sg_addr[i] = addr;
 				psg->sg[i].addr = cpu_to_le32(addr & 0xffffffff);
 				byte_count += usg->sg[i].count;
 				psg->sg[i].count = cpu_to_le32(sg_count[i]);
@@ -905,7 +925,11 @@ 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);
-
+				if (dma_mapping_error(&dev->pdev->dev, addr)) {
+					rcode = -ENOMEM;
+					goto cleanup;
+				}
+				sg_addr[i] = addr;
 				psg->sg[i].addr = cpu_to_le32(addr);
 				byte_count += sg_count[i];
 				psg->sg[i].count = cpu_to_le32(sg_count[i]);
@@ -932,6 +956,11 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 
 	if (flags & SRB_DataIn) {
 		for(i = 0 ; i <= sg_indx; i++){
+			if (sg_addr[i] != DMA_MAPPING_ERROR) {
+				dma_unmap_single(&dev->pdev->dev, sg_addr[i],
+						 sg_count[i], data_dir);
+				sg_addr[i] = DMA_MAPPING_ERROR;
+			}
 			if (copy_to_user(sg_user[i], sg_list[i], sg_count[i])) {
 				dprintk((KERN_DEBUG"aacraid: Could not copy sg data to user\n"));
 				rcode = -EFAULT;
@@ -986,8 +1015,12 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 cleanup:
 	kfree(user_srbcmd);
 	if (rcode != -ERESTARTSYS) {
-		for (i = 0; i <= sg_indx; i++)
+		for (i = 0; i <= sg_indx; i++) {
+			if (sg_addr[i] != DMA_MAPPING_ERROR)
+				dma_unmap_single(&dev->pdev->dev, sg_addr[i],
+						 sg_count[i], data_dir);
 			kfree(sg_list[i]);
+		}
 		aac_fib_complete(srbfib);
 		aac_fib_free(srbfib);
 	}
-- 
2.55.0