Hi Christophe,

On Tue, May 29, 2018 at 10:56 AM, Christophe LEROY
[off-list ref] wrote:

Le 29/05/2018 =C3=A0 09:47, Geert Uytterhoeven a =C3=A9crit :

quoted

On Tue, May 29, 2018 at 8:03 AM, Christophe Leroy
[off-list ref] wrote:

quoted

   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

quoted

quoted

length [-Werror=3Dstringop-truncation]
   strncpy((char *)&n, text, 8);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Christophe Leroy <redacted>

Thanks for your patch!

quoted

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

quoted

quoted

         new_part->header.checksum =3D nvram_checksum(&new_part->header=

);

quoted

quoted

         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