linux-hardening - Re: [PATCH][next] mtd: rawnand: 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200930213634.GA12855@embeddedor>
Date:   Wed, 30 Sep 2020 16:36:34 -0500
From:   "Gustavo A. R. Silva" <gustavoars@...nel.org>
To:     Jann Horn <jannh@...gle.com>
Cc:     Stefan Agner <stefan@...er.ch>, Lucas Stach <dev@...xeye.de>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Richard Weinberger <richard@....at>,
        Vignesh Raghavendra <vigneshr@...com>,
        Thierry Reding <thierry.reding@...il.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        linux-mtd@...ts.infradead.org, linux-tegra@...r.kernel.org,
        kernel list <linux-kernel@...r.kernel.org>,
        linux-hardening@...r.kernel.org
Subject: Re: [PATCH][next] mtd: rawnand: Replace one-element array with
 flexible-array member

On Wed, Sep 30, 2020 at 11:10:43PM +0200, Jann Horn wrote:
> On Wed, Sep 30, 2020 at 11:02 PM 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].
> 
> But this is not such a case, right? Isn't this a true fixed-size
> array? It sounds like you're just changing it because it
> pattern-matched on "array of length 1 at the end of a struct".

Yeah; I should have changed that 'dynamically' part of the text above
a bit. However, as I commented in the text below, in the case that more
CS IDs are needed (let's wait for the maintainers to comment on this...)
in the future, this change makes the code more maintainable, as for
the allocation part, the developer would only have to update the CS_N
macro to the number of CS IDs that are needed.

Thanks
--
Gustavo

> 
> > Refactor the code according to the use of a flexible-array member
> > instead of a one-element array. Also, make use of the struct_size()
> > helper to calculate the size of the allocation for _nand_. In order
> > to keep the code as maintainable as possible and to keep _cs_ as an
> > array, add a new macro CS_N to aid in the allocation size calculation,
> > in case there is a need for more Chip Select IDs in the future. In the
> > meantime, the macro is set to 1. This also avoids having to use a magic
> > number '1' as the last argument for struct_size().
> [...]
> > diff --git a/drivers/mtd/nand/raw/tegra_nand.c b/drivers/mtd/nand/raw/tegra_nand.c
> [...]
> > +/* Number of Chip Selects. Currently, only one. */
> > +#define CS_N                   1
> > +
> >  struct tegra_nand_controller {
> >         struct nand_controller controller;
> >         struct device *dev;
> > @@ -183,7 +186,7 @@ struct tegra_nand_chip {
> >         u32 config;
> >         u32 config_ecc;
> >         u32 bch_config;
> > -       int cs[1];
> > +       int cs[];
> >  };
> [...]