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

[Gustavo A. R. Silva]

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>