* [PATCH 1/2] scsi: aacraid: Rearrange order of struct aac_srb_unit
2024-07-11 21:57 [PATCH 0/2] scsi: aacraid: struct sgmap: Replace 1-element arrays with flexible arrays Kees Cook
@ 2024-07-11 21:57 ` Kees Cook
2024-07-11 21:57 ` [PATCH 2/2] scsi: aacraid: struct {user,}sgmap{,64,raw}: Replace 1-element arrays with flexible arrays Kees Cook
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2024-07-11 21:57 UTC (permalink / raw)
To: Adaptec OEM Raid Solutions
Cc: Kees Cook, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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 <kees@kernel.org>
---
Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
drivers/scsi/aacraid/aacraid.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 6f0417f6f8a1..8e7a0a5cb7aa 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -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;
};
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/2] scsi: aacraid: struct {user,}sgmap{,64,raw}: Replace 1-element arrays with flexible arrays
2024-07-11 21:57 [PATCH 0/2] scsi: aacraid: struct sgmap: Replace 1-element arrays with flexible arrays Kees Cook
2024-07-11 21:57 ` [PATCH 1/2] scsi: aacraid: Rearrange order of struct aac_srb_unit Kees Cook
@ 2024-07-11 21:57 ` Kees Cook
2024-08-03 1:40 ` [PATCH 0/2] scsi: aacraid: struct sgmap: " Martin K. Petersen
2024-08-05 21:17 ` Martin K. Petersen
3 siblings, 0 replies; 5+ messages in thread
From: Kees Cook @ 2024-07-11 21:57 UTC (permalink / raw)
To: Adaptec OEM Raid Solutions
Cc: Kees Cook, James E.J. Bottomley, Martin K. Petersen, linux-scsi,
linux-kernel, linux-hardening
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 <kees@kernel.org>
---
Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
---
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(-)
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 497c6dd5df91..ec3834bda111 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -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;
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 8e7a0a5cb7aa..1d09d3ac6aa4 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -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
diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index e7cc927ed952..68240d6f27ab 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -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));
diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
index bd99c5492b7d..fee857236991 100644
--- a/drivers/scsi/aacraid/comminit.c
+++ b/drivers/scsi/aacraid/comminit.c
@@ -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;
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 25cee03d7f97..47287559c768 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -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);
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread