netdev - Re: [PATCH][next] ath11k: 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>] [<thread-prev] [day] [month] [year] [list]
Message-Id: <20210928105453.1656EC4360D@smtp.codeaurora.org>
Date:   Tue, 28 Sep 2021 10:54:53 +0000 (UTC)
From:   Kalle Valo <kvalo@...eaurora.org>
To:     "Gustavo A. R. Silva" <gustavoars@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, ath11k@...ts.infradead.org,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] ath11k: Replace one-element array with
 flexible-array
 member

"Gustavo A. R. Silva" <gustavoars@...nel.org> wrote:

> There is a regular need in the kernel to provide a way to declare having a
> dynamically sized set of trailing elements in a structure. Kernel code
> should always use "flexible array members"[1] for these cases. The older
> style of one-element or zero-length arrays should no longer be used[2].
> 
> Refactor the code a bit according to the use of a flexible-array member in
> struct scan_chan_list_params instead of a one-element array, and use the
> struct_size() helper.
> 
> Also, save 25 (too many) bytes that were being allocated:
> 
> $ pahole -C channel_param drivers/net/wireless/ath/ath11k/reg.o
> struct channel_param {
>         u8                         chan_id;              /*     0     1 */
>         u8                         pwr;                  /*     1     1 */
>         u32                        mhz;                  /*     2     4 */
> 
>         /* Bitfield combined with next fields */
> 
>         u32                        half_rate:1;          /*     4:16  4 */
>         u32                        quarter_rate:1;       /*     4:17  4 */
>         u32                        dfs_set:1;            /*     4:18  4 */
>         u32                        dfs_set_cfreq2:1;     /*     4:19  4 */
>         u32                        is_chan_passive:1;    /*     4:20  4 */
>         u32                        allow_ht:1;           /*     4:21  4 */
>         u32                        allow_vht:1;          /*     4:22  4 */
>         u32                        allow_he:1;           /*     4:23  4 */
>         u32                        set_agile:1;          /*     4:24  4 */
>         u32                        psc_channel:1;        /*     4:25  4 */
> 
>         /* XXX 6 bits hole, try to pack */
> 
>         u32                        phy_mode;             /*     8     4 */
>         u32                        cfreq1;               /*    12     4 */
>         u32                        cfreq2;               /*    16     4 */
>         char                       maxpower;             /*    20     1 */
>         char                       minpower;             /*    21     1 */
>         char                       maxregpower;          /*    22     1 */
>         u8                         antennamax;           /*    23     1 */
>         u8                         reg_class_id;         /*    24     1 */
> 
>         /* size: 25, cachelines: 1, members: 21 */
>         /* sum members: 23 */
>         /* sum bitfield members: 10 bits, bit holes: 1, sum bit holes: 6 bits */
>         /* last cacheline: 25 bytes */
> } __attribute__((__packed__));
> 
> as previously, sizeof(struct scan_chan_list_params) was 32 bytes:
> 
> $ pahole -C scan_chan_list_params drivers/net/wireless/ath/ath11k/reg.o
> struct scan_chan_list_params {
>         u32                        pdev_id;              /*     0     4 */
>         u16                        nallchans;            /*     4     2 */
>         struct channel_param       ch_param[1];          /*     6    25 */
> 
>         /* size: 32, cachelines: 1, members: 3 */
>         /* padding: 1 */
>         /* last cacheline: 32 bytes */
> };
> 
> and now with the flexible array transformation it is just 8 bytes:
> 
> $ pahole -C scan_chan_list_params drivers/net/wireless/ath/ath11k/reg.o
> struct scan_chan_list_params {
>         u32                        pdev_id;              /*     0     4 */
>         u16                        nallchans;            /*     4     2 */
>         struct channel_param       ch_param[];           /*     6     0 */
> 
>         /* size: 8, cachelines: 1, members: 3 */
>         /* padding: 2 */
>         /* last cacheline: 8 bytes */
> };
> 
> This helps with the ongoing efforts to globally enable -Warray-bounds and
> get us closer to being able to tighten the FORTIFY_SOURCE routines on
> memcpy().
> 
> This issue was found with the help of Coccinelle and audited and fixed,
> manually.
> 
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://www.kernel.org/doc/html/v5.10/process/deprecated.html#zero-length-and-one-element-arrays
> 
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://github.com/KSPP/linux/issues/109
> Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> Signed-off-by: Kalle Valo <kvalo@...eaurora.org>

Patch applied to ath-next branch of ath.git, thanks.

b2549465cdea ath11k: Replace one-element array with flexible-array member

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210823172159.GA25800@embeddedor/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches