linux-kernel - Re: [PATCH v2] powerpc/64: Fix build failure with GCC 8.1

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: <87efhsz7xz.fsf@concordia.ellerman.id.au>
Date:   Thu, 31 May 2018 15:54:16 +1000
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Christophe LEROY <christophe.leroy@....fr>,
        Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        Geoff Levand <geoff@...radead.org>
Subject: Re: [PATCH v2] powerpc/64: Fix build failure with GCC 8.1

Christophe LEROY <christophe.leroy@....fr> writes:
> Le 29/05/2018 à 11:05, Geert Uytterhoeven a écrit :
>> Hi Christophe,
>> On Tue, May 29, 2018 at 10:56 AM, Christophe LEROY
>> <christophe.leroy@....fr> wrote:
>>> Le 29/05/2018 à 09:47, Geert Uytterhoeven a écrit :
>>>> On Tue, May 29, 2018 at 8:03 AM, Christophe Leroy
>>>>> --- a/arch/powerpc/kernel/nvram_64.c
>>>>> +++ b/arch/powerpc/kernel/nvram_64.c
>>>>> @@ -1039,7 +1039,7 @@ loff_t __init nvram_create_partition(const char
>>>>> *name, int sig,
>>>>>           new_part->index = free_part->index;
>>>>>           new_part->header.signature = sig;
>>>>>           new_part->header.length = size;
>>>>> -       strncpy(new_part->header.name, name, 12);
>>>>> +       memcpy(new_part->header.name, name, strnlen(name,
>>>>> sizeof(new_part->header.name)));
>>>>
>>>>
>>>> The comment for nvram_header.lgnth says:
>>>>
>>>>           /* Terminating null required only for names < 12 chars. */
>>>>
>>>> This will not terminate the string with a zero (the struct is
>>>> allocated with kmalloc).
>>>> So the original code is correct, the new one isn't.
>>>
>>> Right, then I have to first zeroize the destination.
>> 
>> Using kzalloc() instead of kmalloc() will do.
>> 
>> Still, papering around these warnings seems to obscure things, IMHO.
>> And it increases code size, as you had to add a call to strnlen().


The right fix is to not try and mirror the on-device structure in the
kernel struct. We should just use a proper NULL terminated string, which
would avoid the need to explicitly do strncmp(.., .., 12) in the code
and be less bug prone in general.

The only place where we should need to worry about the 12 byte buffer is
in nvram_write_header().

Anyway that's a bigger change, so I'll take this for now with kzalloc().

cheers