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

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

On 2/6/23 15:58, Don.Brace@microchip.com wrote:
> 
> ________________________________
> From: Gustavo A. R. Silva <gustavoars@kernel.org>
> Sent: Wednesday, September 21, 2022 11:28 PM
> To: Kevin Barnett <kevin.barnett@microsemi.com>; Don Brace - C33706 <Don.Brace@microchip.com>; storagedev <storagedev@microchip.com>; James E.J. Bottomley <jejb@linux.ibm.com>; Martin K. Petersen <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org <linux-scsi@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>; Gustavo A. R. Silva <gustavoars@kernel.org>; linux-hardening@vger.kernel.org <linux-hardening@vger.kernel.org>
> 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@kernel.org. 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@kernel.org>
> 
> 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@microchip.com>
> 
> 
> ---
> 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
> 
>