[PATCH v2] powerpc/64: Fix build failure with GCC 8.1

[PATCH v2] powerpc/64: Fix build failure with GCC 8.1

[Christophe Leroy]

  CC      arch/powerpc/kernel/nvram_64.o
arch/powerpc/kernel/nvram_64.c: In function 'nvram_create_partition':
arch/powerpc/kernel/nvram_64.c:1042:2: error: 'strncpy' specified bound 12 equals destination size [-Werror=stringop-truncation]
  strncpy(new_part->header.name, name, 12);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  CC      arch/powerpc/kernel/trace/ftrace.o
In function 'make_field',
    inlined from 'ps3_repository_read_boot_dat_address' at arch/powerpc/platforms/ps3/repository.c:900:9:
arch/powerpc/platforms/ps3/repository.c:106:2: error: 'strncpy' output truncated before terminating nul copying 8 bytes from a string of the same length [-Werror=stringop-truncation]
  strncpy((char *)&n, text, 8);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v2:
 - Using strnlen(src, sizeof(dest)) instead of min(strlen(src), sizeof(dest))
 - Changed nvram one to memcpy() to still fit the entire fied (thanks to benh)

 arch/powerpc/kernel/nvram_64.c          | 2 +-
 arch/powerpc/platforms/ps3/repository.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index ba681dac7b46..cf7772cdc3fd 100644
--- 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)));
 	new_part->header.checksum = nvram_checksum(&new_part->header);
 
 	rc = nvram_write_header(new_part);
diff --git a/arch/powerpc/platforms/ps3/repository.c b/arch/powerpc/platforms/ps3/repository.c
index 50dbaf24b1ee..e49c887787c4 100644
--- a/arch/powerpc/platforms/ps3/repository.c
+++ b/arch/powerpc/platforms/ps3/repository.c
@@ -101,9 +101,9 @@ static u64 make_first_field(const char *text, u64 index)
 
 static u64 make_field(const char *text, u64 index)
 {
-	u64 n;
+	u64 n = 0;
 
-	strncpy((char *)&n, text, 8);
+	memcpy((char *)&n, text, strnlen(text, sizeof(n)));
 	return n + index;
 }
 
-- 
2.13.3

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

[Geert Uytterhoeven]

Hi Christophe,

CC Geoff

On Tue, May 29, 2018 at 8:03 AM, Christophe Leroy
<christophe.leroy@c-s.fr> wrote:
>   CC      arch/powerpc/kernel/nvram_64.o
> arch/powerpc/kernel/nvram_64.c: In function 'nvram_create_partition':
> arch/powerpc/kernel/nvram_64.c:1042:2: error: 'strncpy' specified bound 12 equals destination size [-Werror=stringop-truncation]
>   strncpy(new_part->header.name, name, 12);
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>   CC      arch/powerpc/kernel/trace/ftrace.o
> In function 'make_field',
>     inlined from 'ps3_repository_read_boot_dat_address' at arch/powerpc/platforms/ps3/repository.c:900:9:
> arch/powerpc/platforms/ps3/repository.c:106:2: error: 'strncpy' output truncated before terminating nul copying 8 bytes from a string of the same length [-Werror=stringop-truncation]
>   strncpy((char *)&n, text, 8);
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>

Thanks for your patch!

> --- 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.

>         new_part->header.checksum = nvram_checksum(&new_part->header);
>
>         rc = nvram_write_header(new_part);
> diff --git a/arch/powerpc/platforms/ps3/repository.c b/arch/powerpc/platforms/ps3/repository.c
> index 50dbaf24b1ee..e49c887787c4 100644
> --- a/arch/powerpc/platforms/ps3/repository.c
> +++ b/arch/powerpc/platforms/ps3/repository.c
> @@ -101,9 +101,9 @@ static u64 make_first_field(const char *text, u64 index)
>
>  static u64 make_field(const char *text, u64 index)
>  {
> -       u64 n;
> +       u64 n = 0;
>
> -       strncpy((char *)&n, text, 8);
> +       memcpy((char *)&n, text, strnlen(text, sizeof(n)));

This changes behavior: strncpy() fills the remainder of the buffer with
zeroes.  I don't remember the details of the PS3 repository structure,
but given this writes to a fixed size u64 buffer, I'd expect the PS3
hypervisor code to (1) rely on the zero padding, and (2) not need a zero
terminator if there are 8 characters in the buffer, so probably the
original code is correct, and the "fixed" code isn't.

Has this been tested on a PS3?

>         return n + index;
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Christophe LEROY]

Le 29/05/2018 à 09:47, Geert Uytterhoeven a écrit :
> Hi Christophe,
> 
> CC Geoff
> 
> On Tue, May 29, 2018 at 8:03 AM, Christophe Leroy
> <christophe.leroy@c-s.fr> wrote:
>>    CC      arch/powerpc/kernel/nvram_64.o
>> arch/powerpc/kernel/nvram_64.c: In function 'nvram_create_partition':
>> arch/powerpc/kernel/nvram_64.c:1042:2: error: 'strncpy' specified bound 12 equals destination size [-Werror=stringop-truncation]
>>    strncpy(new_part->header.name, name, 12);
>>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>>    CC      arch/powerpc/kernel/trace/ftrace.o
>> In function 'make_field',
>>      inlined from 'ps3_repository_read_boot_dat_address' at arch/powerpc/platforms/ps3/repository.c:900:9:
>> arch/powerpc/platforms/ps3/repository.c:106:2: error: 'strncpy' output truncated before terminating nul copying 8 bytes from a string of the same length [-Werror=stringop-truncation]
>>    strncpy((char *)&n, text, 8);
>>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> 
> Thanks for your patch!
> 
>> --- 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.

> 
>>          new_part->header.checksum = nvram_checksum(&new_part->header);
>>
>>          rc = nvram_write_header(new_part);
>> diff --git a/arch/powerpc/platforms/ps3/repository.c b/arch/powerpc/platforms/ps3/repository.c
>> index 50dbaf24b1ee..e49c887787c4 100644
>> --- a/arch/powerpc/platforms/ps3/repository.c
>> +++ b/arch/powerpc/platforms/ps3/repository.c
>> @@ -101,9 +101,9 @@ static u64 make_first_field(const char *text, u64 index)
>>
>>   static u64 make_field(const char *text, u64 index)
>>   {
>> -       u64 n;
>> +       u64 n = 0;
>>
>> -       strncpy((char *)&n, text, 8);
>> +       memcpy((char *)&n, text, strnlen(text, sizeof(n)));
> 
> This changes behavior: strncpy() fills the remainder of the buffer with
> zeroes.  I don't remember the details of the PS3 repository structure,
> but given this writes to a fixed size u64 buffer, I'd expect the PS3
> hypervisor code to (1) rely on the zero padding, and (2) not need a zero
> terminator if there are 8 characters in the buffer, so probably the
> original code is correct, and the "fixed" code isn't.

Here I have set n to 0 prior to the copy, so the buffer IS zero padded.

Christophe

> 
> Has this been tested on a PS3?
> 
>>          return n + index;
>>   }
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

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

[Geert Uytterhoeven]

Hi Christophe,

On Tue, May 29, 2018 at 10:56 AM, Christophe LEROY
<christophe.leroy@c-s.fr> wrote:
> Le 29/05/2018 =C3=A0 09:47, Geert Uytterhoeven a =C3=A9crit :
>> On Tue, May 29, 2018 at 8:03 AM, Christophe Leroy
>> <christophe.leroy@c-s.fr> wrote:
>>>
>>>    CC      arch/powerpc/kernel/nvram_64.o
>>> arch/powerpc/kernel/nvram_64.c: In function 'nvram_create_partition':
>>> arch/powerpc/kernel/nvram_64.c:1042:2: error: 'strncpy' specified bound
>>> 12 equals destination size [-Werror=3Dstringop-truncation]
>>>    strncpy(new_part->header.name, name, 12);
>>>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>>    CC      arch/powerpc/kernel/trace/ftrace.o
>>> In function 'make_field',
>>>      inlined from 'ps3_repository_read_boot_dat_address' at
>>> arch/powerpc/platforms/ps3/repository.c:900:9:
>>> arch/powerpc/platforms/ps3/repository.c:106:2: error: 'strncpy' output
>>> truncated before terminating nul copying 8 bytes from a string of the s=
ame
>>> length [-Werror=3Dstringop-truncation]
>>>    strncpy((char *)&n, text, 8);
>>>    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>
>>
>> Thanks for your patch!
>>
>>> --- 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 =3D free_part->index;
>>>          new_part->header.signature =3D sig;
>>>          new_part->header.length =3D 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().

>>>          new_part->header.checksum =3D nvram_checksum(&new_part->header=
);
>>>
>>>          rc =3D nvram_write_header(new_part);
>>> diff --git a/arch/powerpc/platforms/ps3/repository.c
>>> b/arch/powerpc/platforms/ps3/repository.c
>>> index 50dbaf24b1ee..e49c887787c4 100644
>>> --- a/arch/powerpc/platforms/ps3/repository.c
>>> +++ b/arch/powerpc/platforms/ps3/repository.c
>>> @@ -101,9 +101,9 @@ static u64 make_first_field(const char *text, u64
>>> index)
>>>
>>>   static u64 make_field(const char *text, u64 index)
>>>   {
>>> -       u64 n;
>>> +       u64 n =3D 0;
>>>
>>> -       strncpy((char *)&n, text, 8);
>>> +       memcpy((char *)&n, text, strnlen(text, sizeof(n)));
>>
>>
>> This changes behavior: strncpy() fills the remainder of the buffer with
>> zeroes.  I don't remember the details of the PS3 repository structure,
>> but given this writes to a fixed size u64 buffer, I'd expect the PS3
>> hypervisor code to (1) rely on the zero padding, and (2) not need a zero
>> terminator if there are 8 characters in the buffer, so probably the
>> original code is correct, and the "fixed" code isn't.
>
> Here I have set n to 0 prior to the copy, so the buffer IS zero padded.

Sorry, I missed that part.

Gr{oetje,eeting}s,

                        Geert

--=20
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k=
.org

In personal conversations with technical people, I call myself a hacker. Bu=
t
when I'm talking to journalists I just say "programmer" or something like t=
hat.
                                -- Linus Torvalds

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

[Christophe LEROY]

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@c-s.fr> wrote:
>> Le 29/05/2018 à 09:47, Geert Uytterhoeven a écrit :
>>> On Tue, May 29, 2018 at 8:03 AM, Christophe Leroy
>>> <christophe.leroy@c-s.fr> wrote:
>>>>
>>>>     CC      arch/powerpc/kernel/nvram_64.o
>>>> arch/powerpc/kernel/nvram_64.c: In function 'nvram_create_partition':
>>>> arch/powerpc/kernel/nvram_64.c:1042:2: error: 'strncpy' specified bound
>>>> 12 equals destination size [-Werror=stringop-truncation]
>>>>     strncpy(new_part->header.name, name, 12);
>>>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>>     CC      arch/powerpc/kernel/trace/ftrace.o
>>>> In function 'make_field',
>>>>       inlined from 'ps3_repository_read_boot_dat_address' at
>>>> arch/powerpc/platforms/ps3/repository.c:900:9:
>>>> arch/powerpc/platforms/ps3/repository.c:106:2: error: 'strncpy' output
>>>> truncated before terminating nul copying 8 bytes from a string of the same
>>>> length [-Werror=stringop-truncation]
>>>>     strncpy((char *)&n, text, 8);
>>>>     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>>>
>>>
>>> Thanks for your patch!
>>>
>>>> --- 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().

Right but then, what is the best solution to elimate that warning ?

Would it be better to enclose those two lines in:

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wstringop-truncation"
...
#pragma GCC diagnostic pop


Christophe

> 
>>>>           new_part->header.checksum = nvram_checksum(&new_part->header);
>>>>
>>>>           rc = nvram_write_header(new_part);
>>>> diff --git a/arch/powerpc/platforms/ps3/repository.c
>>>> b/arch/powerpc/platforms/ps3/repository.c
>>>> index 50dbaf24b1ee..e49c887787c4 100644
>>>> --- a/arch/powerpc/platforms/ps3/repository.c
>>>> +++ b/arch/powerpc/platforms/ps3/repository.c
>>>> @@ -101,9 +101,9 @@ static u64 make_first_field(const char *text, u64
>>>> index)
>>>>
>>>>    static u64 make_field(const char *text, u64 index)
>>>>    {
>>>> -       u64 n;
>>>> +       u64 n = 0;
>>>>
>>>> -       strncpy((char *)&n, text, 8);
>>>> +       memcpy((char *)&n, text, strnlen(text, sizeof(n)));
>>>
>>>
>>> This changes behavior: strncpy() fills the remainder of the buffer with
>>> zeroes.  I don't remember the details of the PS3 repository structure,
>>> but given this writes to a fixed size u64 buffer, I'd expect the PS3
>>> hypervisor code to (1) rely on the zero padding, and (2) not need a zero
>>> terminator if there are 8 characters in the buffer, so probably the
>>> original code is correct, and the "fixed" code isn't.
>>
>> Here I have set n to 0 prior to the copy, so the buffer IS zero padded.
> 
> Sorry, I missed that part.
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

RE: [PATCH v2] powerpc/64: Fix build failure with GCC 8.1

[David Laight]

RnJvbTogQ2hyaXN0b3BoZSBMRVJPWQ0KPiBTZW50OiAyOSBNYXkgMjAxOCAxMDozNw0KLi4uDQo+
ID4+Pj4gLSAgICAgICBzdHJuY3B5KG5ld19wYXJ0LT5oZWFkZXIubmFtZSwgbmFtZSwgMTIpOw0K
PiA+Pj4+ICsgICAgICAgbWVtY3B5KG5ld19wYXJ0LT5oZWFkZXIubmFtZSwgbmFtZSwgc3Rybmxl
bihuYW1lLA0KPiA+Pj4+IHNpemVvZihuZXdfcGFydC0+aGVhZGVyLm5hbWUpKSk7DQo+ID4+Pg0K
PiA+Pj4NCj4gPj4+IFRoZSBjb21tZW50IGZvciBudnJhbV9oZWFkZXIubGdudGggc2F5czoNCj4g
Pj4+DQo+ID4+PiAgICAgICAgICAgLyogVGVybWluYXRpbmcgbnVsbCByZXF1aXJlZCBvbmx5IGZv
ciBuYW1lcyA8IDEyIGNoYXJzLiAqLw0KPiA+Pj4NCj4gPj4+IFRoaXMgd2lsbCBub3QgdGVybWlu
YXRlIHRoZSBzdHJpbmcgd2l0aCBhIHplcm8gKHRoZSBzdHJ1Y3QgaXMNCj4gPj4+IGFsbG9jYXRl
ZCB3aXRoIGttYWxsb2MpLg0KPiA+Pj4gU28gdGhlIG9yaWdpbmFsIGNvZGUgaXMgY29ycmVjdCwg
dGhlIG5ldyBvbmUgaXNuJ3QuDQo+ID4+DQo+ID4+IFJpZ2h0LCB0aGVuIEkgaGF2ZSB0byBmaXJz
dCB6ZXJvaXplIHRoZSBkZXN0aW5hdGlvbi4NCj4gPg0KPiA+IFVzaW5nIGt6YWxsb2MoKSBpbnN0
ZWFkIG9mIGttYWxsb2MoKSB3aWxsIGRvLg0KPiA+DQo+ID4gU3RpbGwsIHBhcGVyaW5nIGFyb3Vu
ZCB0aGVzZSB3YXJuaW5ncyBzZWVtcyB0byBvYnNjdXJlIHRoaW5ncywgSU1ITy4NCj4gPiBBbmQg
aXQgaW5jcmVhc2VzIGNvZGUgc2l6ZSwgYXMgeW91IGhhZCB0byBhZGQgYSBjYWxsIHRvIHN0cm5s
ZW4oKS4NCj4gDQo+IFJpZ2h0IGJ1dCB0aGVuLCB3aGF0IGlzIHRoZSBiZXN0IHNvbHV0aW9uIHRv
IGVsaW1hdGUgdGhhdCB3YXJuaW5nID8NCg0KVGltZSB0byBhZGQgdGhlIElfcmVhbGx5X21lYW5f
c3RybmN5KCkgZnVuY3Rpb24uDQoNCglEYXZpZA0KDQo=

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

[Michael Ellerman]

Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 29/05/2018 =C3=A0 11:05, Geert Uytterhoeven a =C3=A9crit=C2=A0:
>> Hi Christophe,
>> On Tue, May 29, 2018 at 10:56 AM, Christophe LEROY
>> <christophe.leroy@c-s.fr> wrote:
>>> Le 29/05/2018 =C3=A0 09:47, Geert Uytterhoeven a =C3=A9crit :
>>>> 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 =3D free_part->index;
>>>>>           new_part->header.signature =3D sig;
>>>>>           new_part->header.length =3D 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.
>>=20
>> Using kzalloc() instead of kmalloc() will do.
>>=20
>> 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

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

[Christophe LEROY]

Le 31/05/2018 à 07:54, Michael Ellerman a écrit :
> Christophe LEROY <christophe.leroy@c-s.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@c-s.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().

Thanks. You take it as is and add the kzalloc() or you expect a v3 from 
me with the kzalloc()

Christophe

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

[Michael Ellerman]

Christophe LEROY <christophe.leroy@c-s.fr> writes:
> Le 31/05/2018 =C3=A0 07:54, Michael Ellerman a =C3=A9crit=C2=A0:
>> Christophe LEROY <christophe.leroy@c-s.fr> writes:
>>> Le 29/05/2018 =C3=A0 11:05, Geert Uytterhoeven a =C3=A9crit=C2=A0:
>>>> Hi Christophe,
>>>> On Tue, May 29, 2018 at 10:56 AM, Christophe LEROY
>>>> <christophe.leroy@c-s.fr> wrote:
>>>>> Le 29/05/2018 =C3=A0 09:47, Geert Uytterhoeven a =C3=A9crit :
>>>>>> 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 ch=
ar
>>>>>>> *name, int sig,
>>>>>>>            new_part->index =3D free_part->index;
>>>>>>>            new_part->header.signature =3D sig;
>>>>>>>            new_part->header.length =3D 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().
>>=20
>>=20
>> 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.
>>=20
>> The only place where we should need to worry about the 12 byte buffer is
>> in nvram_write_header().
>>=20
>> Anyway that's a bigger change, so I'll take this for now with kzalloc().
>
> Thanks. You take it as is and add the kzalloc() or you expect a v3 from=20
> me with the kzalloc()

Sorry that wasn't clear was it. I'll add the kzalloc().

cheers

Re: [v2] powerpc/64: Fix build failure with GCC 8.1

[Michael Ellerman]

On Tue, 2018-05-29 at 06:03:53 UTC, Christophe Leroy wrote:
> CC      arch/powerpc/kernel/nvram_64.o
> arch/powerpc/kernel/nvram_64.c: In function 'nvram_create_partition':
> arch/powerpc/kernel/nvram_64.c:1042:2: error: 'strncpy' specified bound 12 equals destination size [-Werror=stringop-truncation]
>   strncpy(new_part->header.name, name, 12);
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
>   CC      arch/powerpc/kernel/trace/ftrace.o
> In function 'make_field',
>     inlined from 'ps3_repository_read_boot_dat_address' at arch/powerpc/platforms/ps3/repository.c:900:9:
> arch/powerpc/platforms/ps3/repository.c:106:2: error: 'strncpy' output truncated before terminating nul copying 8 bytes from a string of the same length [-Werror=stringop-truncation]
>   strncpy((char *)&n, text, 8);
>   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/c95998811807d897ca112ea62d6671

cheers