linux-kernel - Re: [PATCH] mtd: nftl: Remove VLA usage

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]

Date:   Sun, 29 Apr 2018 07:31:15 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Boris Brezillon <boris.brezillon@...tlin.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        David Woodhouse <dwmw2@...radead.org>,
        Brian Norris <computersforpeace@...il.com>,
        Marek Vasut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        Linux mtd <linux-mtd@...ts.infradead.org>
Subject: Re: [PATCH] mtd: nftl: Remove VLA usage

On Sun, Apr 29, 2018 at 2:16 AM, Boris Brezillon
<boris.brezillon@...tlin.com> wrote:
> Hi Kees,
>
> On Mon, 23 Apr 2018 13:35:00 -0700
> Kees Cook <keescook@...omium.org> wrote:
>
>> On the quest to remove all VLAs from the kernel[1] this changes the
>> check_free_sectors() routine to use the same stack buffer for both
>> data byte checks (SECTORSIZE) and oob byte checks (oobsize). Since
>> these regions aren't needed at the same time, they don't need to be
>> consecutively allocated. Additionally, while it's possible for oobsize
>> to be large, it is unlikely to be larger than the actual SECTORSIZE. As
>> such, remove the VLA, adjust offsets and add a sanity check to make sure
>> we never get a pathological oobsize.
>>
>> [1] https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qPXydAacU1RqZWA@mail.gmail.com
>>
>> Signed-off-by: Kees Cook <keescook@...omium.org>
>> ---
>>  drivers/mtd/inftlmount.c | 11 ++++++++---
>>  drivers/mtd/nftlmount.c  | 11 ++++++++---
>>  2 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/inftlmount.c b/drivers/mtd/inftlmount.c
>> index aab4f68bd36f..9cdae7f0fc2e 100644
>> --- a/drivers/mtd/inftlmount.c
>> +++ b/drivers/mtd/inftlmount.c
>> @@ -334,7 +334,7 @@ static int memcmpb(void *a, int c, int n)
>>  static int check_free_sectors(struct INFTLrecord *inftl, unsigned int address,
>>       int len, int check_oob)
>>  {
>> -     u8 buf[SECTORSIZE + inftl->mbd.mtd->oobsize];
>> +     u8 buf[SECTORSIZE];
>
> Could we instead move to dynamic allocation. I mean, SECTORSIZE is 512
> bytes, so only with this function we consume 1/16 of the stack. Not to
> mention that some MTD drivers might want to do DMA on buffer passed by
> the MTD user, and if the buffer is on the stack they'll have to use a
> bounce buffer instead.

Sure! I can rework it to do that. Is GFP_KERNEL okay for that, or does
it need something else?

-Kees

-- 
Kees Cook
Pixel Security