* Re: [PATCH] scsi: aacraid: fix DMA mapping leak in aac_send_raw_srb()
2026-08-09 3:34 [PATCH] scsi: aacraid: fix DMA mapping leak in aac_send_raw_srb() Ivy Lopez
@ 2026-08-09 3:48 ` sashiko-bot
2026-08-10 22:27 ` [PATCH v2] " Ivy Lopez
1 sibling, 0 replies; 3+ messages in thread
From: sashiko-bot @ 2026-08-09 3:48 UTC (permalink / raw)
To: Ivy Lopez; +Cc: linux-scsi
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
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH v2] scsi: aacraid: fix DMA mapping leak in aac_send_raw_srb()
2026-08-09 3:34 [PATCH] scsi: aacraid: fix DMA mapping leak in aac_send_raw_srb() Ivy Lopez
2026-08-09 3:48 ` sashiko-bot
@ 2026-08-10 22:27 ` Ivy Lopez
1 sibling, 0 replies; 3+ messages in thread
From: Ivy Lopez @ 2026-08-10 22:27 UTC (permalink / raw)
To: aacraid, James.Bottomley, martin.petersen
Cc: linux-scsi, linux-kernel, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread