netdev - Re: [PATCH] treewide: Replace zero-length arrays 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <202002111129.77DB1CCC7B@keescook>
Date:   Tue, 11 Feb 2020 11:32:04 -0800
From:   Kees Cook <keescook@...omium.org>
To:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>, linux-kernel@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>,
        linux-crypto@...r.kernel.org, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH] treewide: Replace zero-length arrays with flexible-array
 member

On Tue, Feb 11, 2020 at 01:20:36PM -0600, Gustavo A. R. Silva wrote:
> 
> 
> On 2/11/20 12:32, Greg KH wrote:
> > On Tue, Feb 11, 2020 at 11:41:26AM -0600, Gustavo A. R. Silva wrote:
> >> The current codebase makes use of the zero-length array language
> >> extension to the C90 standard, but the preferred mechanism to declare
> >> variable-length types such as these ones is a flexible array member[1][2],
> >> introduced in C99:
> >>
> >> struct foo {
> >>         int stuff;
> >>         struct boo array[];
> >> };
> >>
> >> By making use of the mechanism above, we will get a compiler warning
> >> in case the flexible array does not occur last in the structure, which
> >> will help us prevent some kind of undefined behavior bugs from being
> >> unadvertenly introduced[3] to the codebase from now on.
> >>
> >> All these instances of code were found with the help of the following
> >> Coccinelle script:
> >>
> >> @@
> >> identifier S, member, array;
> >> type T1, T2;
> >> @@
> >>
> >> struct S {
> >>   ...
> >>   T1 member;
> >>   T2 array[
> >> - 0
> >>   ];
> >> };
> >>
> >> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> >> [2] https://github.com/KSPP/linux/issues/21
> >> [3] commit 76497732932f ("cxgb3/l2t: Fix undefined behaviour")
> >>
> >> NOTE: I'll carry this in my -next tree for the v5.6 merge window.
> > 
> > Why not carve this up into per-subsystem patches so that we can apply
> > them to our 5.7-rc1 trees and then you submit the "remaining" that don't
> > somehow get merged at that timeframe for 5.7-rc2?
> > 
> 
> Yep, sounds good. I'll do that.

FWIW, I'd just like to point out that since this is a mechanical change
with no code generation differences (unlike the pre-C90 1-byte array
conversions), it's a way better use of everyone's time to just splat
this in all at once.

That said, it looks like Gustavo is up for it, but I'd like us to
generally consider these kinds of mechanical changes as being easier to
manage in a single patch. (Though getting Acks tends to be a bit
harder...)

-- 
Kees Cook