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

Ivy Lopez <[email protected]>
Newsgroups org.kernel.vger.linux-scsi,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
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 <[email protected]>
---
 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.