Skip to contentSkip to conversation

scsi: aacraid: struct sgmap: Replace 1-element arrays with flexible arrays

PATCH Applied Kees Cook 2 participants 5 messages Jul 11, 2024 – Aug 5, 2024

Subsystems (2)
SCSI Aacraid SCSI RAID driver
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

cover scsi: aacraid: struct sgmap: Replace 1-element arrays with flexible arrays

Hi,

This replaces some of the last remaining uses in the kernel of 1-element "fake" flexible arrays with modern C99 flexible arrays. Some refactoring is done to ease this, and binary differences are identified. For the on stack size changes in patch 2, the "yes, that is the source of the binary differences" debugging patch can be found here[1].

Thanks!

-Kees

[1] https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=dev/v6.10-rc2/1-element&id=45e6226bcbc5e982541754eca7ac29f403e82f5e

Kees Cook (2):
scsi: aacraid: Rearrange order of struct aac_srb_unit
scsi: aacraid: struct {user,}sgmap{,64,raw}: Replace 1-element arrays
with flexible arrays

 drivers/scsi/aacraid/aachba.c   | 26 ++++++++++++--------------
 drivers/scsi/aacraid/aacraid.h  | 17 ++++++-----------
 drivers/scsi/aacraid/commctrl.c |  4 ++--
 drivers/scsi/aacraid/comminit.c |  3 +--
 drivers/scsi/aacraid/commsup.c  |  5 +++--
 5 files changed, 24 insertions(+), 31 deletions(-)
Signature · 1 line
2.34.1
author
#1 lore raw

PATCH 1/2 scsi: aacraid: Rearrange order of struct aac_srb_unit

struct aac_srb_unit contains struct aac_srb, which contains struct sgmap, which ends in a (currently) "fake" (1-element) flexible array. Converting this to a flexible array is needed so that runtime bounds checking won't think the array is fixed size (i.e. under CONFIG_FORTIFY_SOURCE=y and/or CONFIG_UBSAN_BOUNDS=y), as other parts of aacraid use struct sgmap as a flexible array.

It is not legal to have a flexible array in the middle of a structure, so it either needs to be split up or rearranged so that it is at the end of the structure. Luckily, struct aac_srb_unit, which is exclusively consumed/updated by aac_send_safw_bmic_cmd(), does not depend on member ordering.

The values set in the on-stack struct aac_srb_unit instance "srbu" by the only two callers, aac_issue_safw_bmic_identify() and aac_get_safw_ciss_luns(), do not contain anything in srbu.srb.sgmap.sg, and they both implicitly initialize srbu.srb.sgmap.count to 0 during memset(). For example:

        memset(&srbu, 0, sizeof(struct aac_srb_unit));

        srbcmd = &srbu.srb;
        srbcmd->flags   = cpu_to_le32(SRB_DataIn);
        srbcmd->cdb[0]  = CISS_REPORT_PHYSICAL_LUNS;
        srbcmd->cdb[1]  = 2; /* extended reporting */
        srbcmd->cdb[8]  = (u8)(datasize >> 8);
        srbcmd->cdb[9]  = (u8)(datasize);

        rcode = aac_send_safw_bmic_cmd(dev, &srbu, phys_luns, datasize);

During aac_send_safw_bmic_cmd(), a separate srb is mapped into DMA, and has srbu.srb copied into it:

        srb = fib_data(fibptr);
        memcpy(srb, &srbu->srb, sizeof(struct aac_srb));

Only then is srb.sgmap.count written and srb->sg populated:

        srb->count              = cpu_to_le32(xfer_len);

        sg64 = (struct sgmap64 *)&srb->sg;
        sg64->count             = cpu_to_le32(1);
        sg64->sg[0].addr[1]     = cpu_to_le32(upper_32_bits(addr));
        sg64->sg[0].addr[0]     = cpu_to_le32(lower_32_bits(addr));
        sg64->sg[0].count       = cpu_to_le32(xfer_len);

But this is happening in the DMA memory, not in srbu.srb. An attempt to copy the changes back to srbu does happen:

        /*
         * Copy the updated data for other dumping or other usage if
         * needed
         */
        memcpy(&srbu->srb, srb, sizeof(struct aac_srb));

But this was never correct: the sg64 (3 u32s) overlap of srb.sg (2 u32s) always meant that srbu.srb would have held truncated information and any attempt to walk srbu.srb.sg.sg based on the value of srbu.srb.sg.count would result in attempting to parse past the end of srbu.srb.sg.sg[0] into srbu.srb_reply.

After getting a reply from hardware, the reply is copied into srbu.srb_reply:

        srb_reply = (struct aac_srb_reply *)fib_data(fibptr);
        memcpy(&srbu->srb_reply, srb_reply, sizeof(struct aac_srb_reply));

This has always been fixed-size, so there's no issue here. It is worth noting that the two callers _never check_ srbu contents -- neither srbu.srb nor srbu.srb_reply is examined. (They depend on the mapped xfer_buf instead.)

Therefore, the ordering of members in struct aac_srb_unit does not matter, and the flexible array member can moved to the end.

(Additionally, the two memcpy()s that update srbu could be entirely removed as they are never consumed, but I left that as-is.)

Signed-off-by Kees Cook <[email protected]>

---
Cc: Adaptec OEM Raid Solutions <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: [email protected]

---
 drivers/scsi/aacraid/aacraid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/aacraid/aacraid.h +1 -1
index 6f0417f6f8a1..8e7a0a5cb7aa 100644
@@ -2029,8 +2029,8 @@ struct aac_srb_reply
 };
 
 struct aac_srb_unit {
-	struct aac_srb		srb;
 	struct aac_srb_reply	srb_reply;
+	struct aac_srb		srb;
 };
 
 /*
Signature · 1 line
2.34.1
author
#2 lore raw

PATCH 2/2 scsi: aacraid: struct {user,}sgmap{,64,raw}: Replace 1-element arrays with flexible arrays

In reply to Kees Cook

Replace the deprecated[1] use of 1-element arrays in struct sgmap, struct sgmap64, struct sgmapraw, struct user_sgmap, and struct user_sgmap64 with modern flexible arrays. Additionally remove struct user_sgmapraw as it is unused.

The resulting binary output differences from this change are limited only to stack space consumption of the smaller "srbu" variable in aac_issue_safw_bmic_identify() and aac_get_safw_ciss_luns(), as well as the smaller associated pair of memcpy()s in aac_send_safw_bmic_cmd(). Artificially growing the size of srbu back to its prior size removes all binary differences[2].

As an aside, after studying the aacraid driver code I wonder how aac_send_wellness_command() ever works. It is reporting a size 4 bytes too small for what it has constructed in memory in the DMA region:
sgentry64 is size 12, whereas sgentry is size 8. Perhaps the hardware doesn't care. (Regardless, it is unchanged by this patch.)

Link https://github.com/KSPP/linux/issues/79 [1]
Link https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/commit/?h=dev/v6.10-rc2/1-element&id=45e6226bcbc5e982541754eca7ac29f403e82f5e [2]
Signed-off-by Kees Cook <[email protected]>

---
Cc: Adaptec OEM Raid Solutions <[email protected]>
Cc: "James E.J. Bottomley" <[email protected]>
Cc: "Martin K. Petersen" <[email protected]>
Cc: [email protected]

---
 drivers/scsi/aacraid/aachba.c   | 26 ++++++++++++--------------
 drivers/scsi/aacraid/aacraid.h  | 15 +++++----------
 drivers/scsi/aacraid/commctrl.c |  4 ++--
 drivers/scsi/aacraid/comminit.c |  3 +--
 drivers/scsi/aacraid/commsup.c  |  5 +++--
 5 files changed, 23 insertions(+), 30 deletions(-)
drivers/scsi/aacraid/aachba.c +12 -14
index 497c6dd5df91..ec3834bda111 100644
@@ -1267,7 +1267,7 @@ static int aac_read_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
 			return ret;
 		command = ContainerRawIo;
 		fibsize = sizeof(struct aac_raw_io) +
-			((le32_to_cpu(readcmd->sg.count)-1) * sizeof(struct sgentryraw));
+			(le32_to_cpu(readcmd->sg.count) * sizeof(struct sgentryraw));
 	}
 
 	BUG_ON(fibsize > (fib->dev->max_fib_size - sizeof(struct aac_fibhdr)));

@@ -1302,7 +1302,7 @@ static int aac_read_block64(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u
 	if (ret < 0)
 		return ret;
 	fibsize = sizeof(struct aac_read64) +
-		((le32_to_cpu(readcmd->sg.count) - 1) *
+		(le32_to_cpu(readcmd->sg.count) *
 		 sizeof (struct sgentry64));
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));

@@ -1337,7 +1337,7 @@ static int aac_read_block(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u32
 	if (ret < 0)
 		return ret;
 	fibsize = sizeof(struct aac_read) +
-			((le32_to_cpu(readcmd->sg.count) - 1) *
+			(le32_to_cpu(readcmd->sg.count) *
 			 sizeof (struct sgentry));
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));

@@ -1401,7 +1401,7 @@ static int aac_write_raw_io(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u
 			return ret;
 		command = ContainerRawIo;
 		fibsize = sizeof(struct aac_raw_io) +
-			((le32_to_cpu(writecmd->sg.count)-1) * sizeof (struct sgentryraw));
+			(le32_to_cpu(writecmd->sg.count) * sizeof(struct sgentryraw));
 	}
 
 	BUG_ON(fibsize > (fib->dev->max_fib_size - sizeof(struct aac_fibhdr)));

@@ -1436,7 +1436,7 @@ static int aac_write_block64(struct fib * fib, struct scsi_cmnd * cmd, u64 lba,
 	if (ret < 0)
 		return ret;
 	fibsize = sizeof(struct aac_write64) +
-		((le32_to_cpu(writecmd->sg.count) - 1) *
+		(le32_to_cpu(writecmd->sg.count) *
 		 sizeof (struct sgentry64));
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));

@@ -1473,7 +1473,7 @@ static int aac_write_block(struct fib * fib, struct scsi_cmnd * cmd, u64 lba, u3
 	if (ret < 0)
 		return ret;
 	fibsize = sizeof(struct aac_write) +
-		((le32_to_cpu(writecmd->sg.count) - 1) *
+		(le32_to_cpu(writecmd->sg.count) *
 		 sizeof (struct sgentry));
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));

@@ -1592,9 +1592,9 @@ static int aac_scsi_64(struct fib * fib, struct scsi_cmnd * cmd)
 	/*
 	 *	Build Scatter/Gather list
 	 */
-	fibsize = sizeof (struct aac_srb) - sizeof (struct sgentry) +
+	fibsize = sizeof(struct aac_srb) +
 		((le32_to_cpu(srbcmd->sg.count) & 0xff) *
-		 sizeof (struct sgentry64));
+		 sizeof(struct sgentry64));
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));
 

@@ -1624,7 +1624,7 @@ static int aac_scsi_32(struct fib * fib, struct scsi_cmnd * cmd)
 	 *	Build Scatter/Gather list
 	 */
 	fibsize = sizeof (struct aac_srb) +
-		(((le32_to_cpu(srbcmd->sg.count) & 0xff) - 1) *
+		((le32_to_cpu(srbcmd->sg.count) & 0xff) *
 		 sizeof (struct sgentry));
 	BUG_ON (fibsize > (fib->dev->max_fib_size -
 				sizeof(struct aac_fibhdr)));

@@ -1693,8 +1693,7 @@ static int aac_send_safw_bmic_cmd(struct aac_dev *dev,
 	fibptr->hw_fib_va->header.XferState &=
 		~cpu_to_le32(FastResponseCapable);
 
-	fibsize  = sizeof(struct aac_srb) - sizeof(struct sgentry) +
-						sizeof(struct sgentry64);
+	fibsize = sizeof(struct aac_srb) + sizeof(struct sgentry64);
 
 	/* allocate DMA buffer for response */
 	addr = dma_map_single(&dev->pdev->dev, xfer_buf, xfer_len,

@@ -2267,7 +2266,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
 		dev->a_ops.adapter_bounds = aac_bounds_32;
 		dev->scsi_host_ptr->sg_tablesize = (dev->max_fib_size -
 			sizeof(struct aac_fibhdr) -
-			sizeof(struct aac_write) + sizeof(struct sgentry)) /
+			sizeof(struct aac_write)) /
 				sizeof(struct sgentry);
 		if (dev->dac_support) {
 			dev->a_ops.adapter_read = aac_read_block64;

@@ -2278,8 +2277,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
 			dev->scsi_host_ptr->sg_tablesize =
 				(dev->max_fib_size -
 				sizeof(struct aac_fibhdr) -
-				sizeof(struct aac_write64) +
-				sizeof(struct sgentry64)) /
+				sizeof(struct aac_write64)) /
 					sizeof(struct sgentry64);
 		} else {
 			dev->a_ops.adapter_read = aac_read_block;
drivers/scsi/aacraid/aacraid.h +5 -10
index 8e7a0a5cb7aa..1d09d3ac6aa4 100644
@@ -507,32 +507,27 @@ struct sge_ieee1212 {
 
 struct sgmap {
 	__le32		count;
-	struct sgentry	sg[1];
+	struct sgentry	sg[];
 };
 
 struct user_sgmap {
 	u32		count;
-	struct user_sgentry	sg[1];
+	struct user_sgentry	sg[];
 };
 
 struct sgmap64 {
 	__le32		count;
-	struct sgentry64 sg[1];
+	struct sgentry64 sg[];
 };
 
 struct user_sgmap64 {
 	u32		count;
-	struct user_sgentry64 sg[1];
+	struct user_sgentry64 sg[];
 };
 
 struct sgmapraw {
 	__le32		  count;
-	struct sgentryraw sg[1];
-};
-
-struct user_sgmapraw {
-	u32		  count;
-	struct user_sgentryraw sg[1];
+	struct sgentryraw sg[];
 };
 
 struct creation_info
drivers/scsi/aacraid/commctrl.c +2 -2
index e7cc927ed952..68240d6f27ab 100644
@@ -523,7 +523,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 		goto cleanup;
 	}
 
-	if ((fibsize < (sizeof(struct user_aac_srb) - sizeof(struct user_sgentry))) ||
+	if ((fibsize < sizeof(struct user_aac_srb)) ||
 	    (fibsize > (dev->max_fib_size - sizeof(struct aac_fibhdr)))) {
 		rcode = -EINVAL;
 		goto cleanup;

@@ -561,7 +561,7 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 		rcode = -EINVAL;
 		goto cleanup;
 	}
-	actual_fibsize = sizeof(struct aac_srb) - sizeof(struct sgentry) +
+	actual_fibsize = sizeof(struct aac_srb) +
 		((user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry));
 	actual_fibsize64 = actual_fibsize + (user_srbcmd->sg.count & 0xff) *
 	  (sizeof(struct sgentry64) - sizeof(struct sgentry));
drivers/scsi/aacraid/comminit.c +1 -2
index bd99c5492b7d..fee857236991 100644
@@ -522,8 +522,7 @@ struct aac_dev *aac_init_adapter(struct aac_dev *dev)
 	spin_lock_init(&dev->iq_lock);
 	dev->max_fib_size = sizeof(struct hw_fib);
 	dev->sg_tablesize = host->sg_tablesize = (dev->max_fib_size
-		- sizeof(struct aac_fibhdr)
-		- sizeof(struct aac_write) + sizeof(struct sgentry))
+		- sizeof(struct aac_fibhdr) - sizeof(struct aac_write))
 			/ sizeof(struct sgentry);
 	dev->comm_interface = AAC_COMM_PRODUCER;
 	dev->raw_io_interface = dev->raw_io_64 = 0;
drivers/scsi/aacraid/commsup.c +3 -2
index 25cee03d7f97..47287559c768 100644
@@ -2327,8 +2327,9 @@ static int aac_send_wellness_command(struct aac_dev *dev, char *wellness_str,
 	sg64->sg[0].addr[0] = cpu_to_le32((u32)(addr & 0xffffffff));
 	sg64->sg[0].count = cpu_to_le32(datasize);
 
-	ret = aac_fib_send(ScsiPortCommand64, fibptr, sizeof(struct aac_srb),
-				FsaNormal, 1, 1, NULL, NULL);
+	ret = aac_fib_send(ScsiPortCommand64, fibptr,
+			   sizeof(struct aac_srb) + sizeof(struct sgentry),
+			   FsaNormal, 1, 1, NULL, NULL);
 
 	dma_free_coherent(&dev->pdev->dev, datasize, dma_buf, addr);
Signature · 1 line
2.34.1
Applied
#3 lore raw
In reply to Kees Cook

Kees,

Quoted 6 lines

This replaces some of the last remaining uses in the kernel of 1-element "fake" flexible arrays with modern C99 flexible arrays. Some refactoring is done to ease this, and binary differences are identified. For the on stack size changes in patch 2, the "yes, that is the source of the binary differences" debugging patch can be found here[1].

Applied to 6.12/scsi-staging, thanks!

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
Applied
#4 lore raw
In reply to Kees Cook
Quoted 9 lines · Kees Cook

This replaces some of the last remaining uses in the kernel of 1-element "fake" flexible arrays with modern C99 flexible arrays. Some refactoring is done to ease this, and binary differences are identified. For the on stack size changes in patch 2, the "yes, that is the source of the binary differences" debugging patch can be found here[1].

Thanks!

[...]

Applied to 6.12/scsi-queue, thanks!

[1/2] scsi: aacraid: Rearrange order of struct aac_srb_unit

      https://git.kernel.org/mkp/scsi/c/6e5860b0ad49
[2/2] scsi: aacraid: struct {user,}sgmap{,64,raw}: Replace 1-element arrays with flexible arrays
      https://git.kernel.org/mkp/scsi/c/fdb1db6ea7f6
Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering