Re: [PATCH 1/3][next] scsi: smartpqi: Replace one-element array with flexible-array member

"Gustavo A. R. Silva" <gustavo@xxxxxxxxxxxxxx> · Tue, 7 Feb 2023 15:53:42 -0600

On 2/7/23 15:08, Don.Brace@xxxxxxxxxxxxx wrote:

________________________________
From: Gustavo A. R. Silva <gustavo@xxxxxxxxxxxxxx>
Sent: Monday, February 6, 2023 4:28 PM
To: Don Brace - C33706 <Don.Brace@xxxxxxxxxxxxx>; gustavoars@xxxxxxxxxx <gustavoars@xxxxxxxxxx>; kevin.barnett@xxxxxxxxxxxxx <kevin.barnett@xxxxxxxxxxxxx>; storagedev <storagedev@xxxxxxxxxxxxx>; jejb@xxxxxxxxxxxxx <jejb@xxxxxxxxxxxxx>; martin.petersen@xxxxxxxxxx <martin.petersen@xxxxxxxxxx>
Cc: linux-scsi@xxxxxxxxxxxxxxx <linux-scsi@xxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx <linux-kernel@xxxxxxxxxxxxxxx>; linux-hardening@xxxxxxxxxxxxxxx <linux-hardening@xxxxxxxxxxxxxxx>; Kees Cook <keescook@xxxxxxxxxxxx>
Subject: Re: [PATCH 1/3][next] scsi: smartpqi: Replace one-element array with flexible-array member


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[];
   };

This HUNK is OK.
Thanks for your patch.

Acked-by: Don Brace <don.brace@xxxxxxxxxxxxx>
Don

   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) {

The driver author has provided his input on the change to the above comment:

I think that'd I'd prefer that we just amplify the existing comment to something like this:

         /*
          * Tack the controller itself onto the end of the logical device list
                * by adding a list entry that is all zeros.
          */

OK. Great. :)

Thanks
--
Gustavo


Thank-you for your patch
Don Brace <don.brace@xxxxxxxxxxxxx>








