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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:   Tue, 7 Feb 2023 15:53:42 -0600
From:   "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To:     Don.Brace@...rochip.com, gustavoars@...nel.org,
        kevin.barnett@...rosemi.com, storagedev@...rochip.com,
        jejb@...ux.ibm.com, martin.petersen@...cle.com
Cc:     linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-hardening@...r.kernel.org, keescook@...omium.org
Subject: Re: [PATCH 1/3][next] scsi: smartpqi: Replace one-element array with
 flexible-array member



On 2/7/23 15:08, Don.Brace@...rochip.com wrote:
> 
> ________________________________
> From: Gustavo A. R. Silva <gustavo@...eddedor.com>
> Sent: Monday, February 6, 2023 4:28 PM
> To: Don Brace - C33706 <Don.Brace@...rochip.com>; gustavoars@...nel.org <gustavoars@...nel.org>; kevin.barnett@...rosemi.com <kevin.barnett@...rosemi.com>; storagedev <storagedev@...rochip.com>; jejb@...ux.ibm.com <jejb@...ux.ibm.com>; martin.petersen@...cle.com <martin.petersen@...cle.com>
> Cc: linux-scsi@...r.kernel.org <linux-scsi@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.kernel.org>; linux-hardening@...r.kernel.org <linux-hardening@...r.kernel.org>; Kees Cook <keescook@...omium.org>
> 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@...rochip.com>
> 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@...rochip.com>
> 
>