Re: [PATCH 1/3][next] scsi: smartpqi: Replace one-element array with flexible-array member
From: Gustavo A. R. Silva
Date: Mon Feb 06 2023 - 17:28:16 EST
On 2/6/23 15:58, Don.Brace@xxxxxxxxxxxxx wrote:
________________________________
From: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
Sent: Wednesday, September 21, 2022 11:28 PM
To: Kevin Barnett <kevin.barnett@xxxxxxxxxxxxx>; Don Brace - C33706 <Don.Brace@xxxxxxxxxxxxx>; storagedev <storagedev@xxxxxxxxxxxxx>; James E.J. Bottomley <jejb@xxxxxxxxxxxxx>; Martin K. Petersen <martin.petersen@xxxxxxxxxx>
Cc: linux-scsi@xxxxxxxxxxxxxxx <linux-scsi@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>; linux-hardening@xxxxxxxxxxxxxxx <linux-hardening@xxxxxxxxxxxxxxx>
Subject: [PATCH 1/3][next] scsi: smartpqi: Replace one-element array with flexible-array member
[Some people who received this message don't often get email from gustavoars@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
One-element arrays are deprecated, and we are replacing them with flexible
array members instead. So, replace one-element array with flexible-array
member in struct MR_DRV_RAID_MAP and refactor the the rest of the code
accordingly.
It seems that the addition of sizeof(struct report_log_lun) in all the
places that are modified by this patch is due to the fact that
the one-element array struct report_log_lun lun_entries[1]; always
contributes to the size of the containing structure struct
report_log_lun_list.
Notice that at line 1267 while allocating memory for an instance of
struct report_log_lun_list, some _extra_ space seems to be allocated
for one element of type struct report_log_lun, which is the type of
the elements in array lun_entries:
1267 internal_logdev_list = kmalloc(logdev_data_length +
1268 sizeof(struct report_log_lun), GFP_KERNEL);
However, at line 1275 just logdev_data_length bytes are copied into
internal_logdev_list (remember that we allocated space for logdev_data_length +
sizeof(struct report_log_lun) bytes at line 1267), and then exactly
sizeof(struct report_log_lun) bytes are being zeroing out at line 1276.
1275 memcpy(internal_logdev_list, logdev_data, logdev_data_length);
1276 memset((u8 *)internal_logdev_list + logdev_data_length, 0,
1277 sizeof(struct report_log_lun));
All the above makes think that it's just fine if we transform array
lun_entries into a flexible-array member and just don't allocate
that extra sizeof(struct report_log_lun) bytes of space. With this
we can remove that memset() call and we also need to modify the code
that updates the total length (internal_logdev_list->header.list_length)
of array lun_entries at line 1278:
1278 put_unaligned_be32(logdev_list_length +
1279 sizeof(struct report_log_lun),
1280 &internal_logdev_list->header.list_length);
This helps with the ongoing efforts to tighten the FORTIFY_SOURCE routines
on memcpy().
Link: https://github.com/KSPP/linux/issues/79
Link: https://github.com/KSPP/linux/issues/204
Signed-off-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx>
NAK: What is actually happening is that we were taking on an extra list entry that is all zeros for the controller itself. This is intentional. These changes will break the driver.
Oh, great to know. :)
So, in this case, what do you think about this, instead:
diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h
index af27bb0f3133..228838eb3686 100644
--- a/drivers/scsi/smartpqi/smartpqi.h
+++ b/drivers/scsi/smartpqi/smartpqi.h
@@ -954,7 +954,7 @@ struct report_log_lun {
struct report_log_lun_list {
struct report_lun_header header;
- struct report_log_lun lun_entries[1];
+ struct report_log_lun lun_entries[];
};
struct report_phys_lun_8byte_wwid {
diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index d0446d4d4465..af8f1a8e9f8f 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -1277,6 +1277,10 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info,
logdev_data_length = sizeof(struct report_lun_header) +
logdev_list_length;
+ /*
+ * Notice that we take on an extra list entry (struct report_log_lun)
+ * that is all zeros for the controller itself.
+ */
internal_logdev_list = kmalloc(logdev_data_length +
sizeof(struct report_log_lun), GFP_KERNEL);
if (!internal_logdev_list) {
Thanks for the feedback!
--
Gustavo
Thanks,
Don Brace <don.brace@xxxxxxxxxxxxx>
---
And of course, it'd be great if maintainers can confirm what I described
in the changelog text. :)
drivers/scsi/smartpqi/smartpqi.h | 2 +-
drivers/scsi/smartpqi/smartpqi_init.c | 10 +++-------
2 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/smartpqi/smartpqi.h b/drivers/scsi/smartpqi/smartpqi.h
index e550b12e525a..d1756c9d1112 100644
--- a/drivers/scsi/smartpqi/smartpqi.h
+++ b/drivers/scsi/smartpqi/smartpqi.h
@@ -954,7 +954,7 @@ struct report_log_lun {
struct report_log_lun_list {
struct report_lun_header header;
- struct report_log_lun lun_entries[1];
+ struct report_log_lun lun_entries[];
};
struct report_phys_lun_8byte_wwid {
diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c
index b971fbe3b3a1..544cd18a90d7 100644
--- a/drivers/scsi/smartpqi/smartpqi_init.c
+++ b/drivers/scsi/smartpqi/smartpqi_init.c
@@ -1264,8 +1264,7 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info,
logdev_data_length = sizeof(struct report_lun_header) +
logdev_list_length;
- internal_logdev_list = kmalloc(logdev_data_length +
- sizeof(struct report_log_lun), GFP_KERNEL);
+ internal_logdev_list = kmalloc(logdev_data_length, GFP_KERNEL);
if (!internal_logdev_list) {
kfree(*logdev_list);
*logdev_list = NULL;
@@ -1273,11 +1272,8 @@ static int pqi_get_device_lists(struct pqi_ctrl_info *ctrl_info,
}
memcpy(internal_logdev_list, logdev_data, logdev_data_length);
- memset((u8 *)internal_logdev_list + logdev_data_length, 0,
- sizeof(struct report_log_lun));
- put_unaligned_be32(logdev_list_length +
- sizeof(struct report_log_lun),
- &internal_logdev_list->header.list_length);
+ put_unaligned_be32(logdev_list_length,
+ &internal_logdev_list->header.list_length);
kfree(*logdev_list);
*logdev_list = internal_logdev_list;
--
2.34.1