[PATCH v2] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section

[PATCH v2] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section

[Thomas Huth]

According to the "ELF-64 Object File Format" specification:

"The first word in the entry, namesz, identifies the length, in
 bytes, of a name identifying the entry’s owner or originator. The name field
 contains a null-terminated string, with padding as necessary to ensure 8-
 byte alignment for the descriptor field. The length does not include the
 terminating null or the padding."

So we should not include the terminating NUL in the length field here.

Also there is a compiler warning with GCC 9.3 when compiling with
the -fsanitize=thread compiler flag:

 In function 'strncpy',
    inlined from 's390x_write_elf64_notes' at ../target/s390x/arch_dump.c:219:9:
 /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
  '__builtin_strncpy' specified bound 8 equals destination size
  [-Werror=stringop-truncation]

Since the name should always be NUL-terminated, let's use g_strlcpy() to
silence this warning. And while we're at it, also add an assert() to make
sure that the provided names always fit the size field (which is fine for
the current callers, the function is called once with "CORE" and once with
"LINUX" as a name).

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v2: Use g_strlcpy instead of strncpy

 target/s390x/arch_dump.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
index 50fa0ae4b6..f205123834 100644
--- a/target/s390x/arch_dump.c
+++ b/target/s390x/arch_dump.c
@@ -212,11 +212,13 @@ static int s390x_write_elf64_notes(const char *note_name,
     int note_size;
     int ret = -1;
 
+    assert(strlen(note_name) < sizeof(note.name));
+
     for (nf = funcs; nf->note_contents_func; nf++) {
         memset(&note, 0, sizeof(note));
-        note.hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
+        note.hdr.n_namesz = cpu_to_be32(strlen(note_name));
         note.hdr.n_descsz = cpu_to_be32(nf->contents_size);
-        strncpy(note.name, note_name, sizeof(note.name));
+        g_strlcpy(note.name, note_name, sizeof(note.name));
         (*nf->note_contents_func)(&note, cpu, id);
 
         note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
-- 
2.27.0

Re: [PATCH v2] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section

[Christian Borntraeger]

On 04.02.21 17:41, Thomas Huth wrote:
> According to the "ELF-64 Object File Format" specification:
> 
> "The first word in the entry, namesz, identifies the length, in
>  bytes, of a name identifying the entry’s owner or originator. The name field
>  contains a null-terminated string, with padding as necessary to ensure 8-
>  byte alignment for the descriptor field. The length does not include the
>  terminating null or the padding."
> 
> So we should not include the terminating NUL in the length field here.
> 
> Also there is a compiler warning with GCC 9.3 when compiling with
> the -fsanitize=thread compiler flag:
> 
>  In function 'strncpy',
>     inlined from 's390x_write_elf64_notes' at ../target/s390x/arch_dump.c:219:9:
>  /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
>   '__builtin_strncpy' specified bound 8 equals destination size
>   [-Werror=stringop-truncation]
> 
> Since the name should always be NUL-terminated, let's use g_strlcpy() to
> silence this warning. And while we're at it, also add an assert() to make
> sure that the provided names always fit the size field (which is fine for
> the current callers, the function is called once with "CORE" and once with
> "LINUX" as a name).
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v2: Use g_strlcpy instead of strncpy


With this patch I do get

WARNING: possibly corrupt Elf64_Nhdr: n_namesz: 0 n_descsz: 4 n_type: 88

when running crash on the elf file created by dump-guest-memory. Without the
patch everything is fine. 

> 
>  target/s390x/arch_dump.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/arch_dump.c b/target/s390x/arch_dump.c
> index 50fa0ae4b6..f205123834 100644
> --- a/target/s390x/arch_dump.c
> +++ b/target/s390x/arch_dump.c
> @@ -212,11 +212,13 @@ static int s390x_write_elf64_notes(const char *note_name,
>      int note_size;
>      int ret = -1;
>  
> +    assert(strlen(note_name) < sizeof(note.name));
> +
>      for (nf = funcs; nf->note_contents_func; nf++) {
>          memset(&note, 0, sizeof(note));
> -        note.hdr.n_namesz = cpu_to_be32(strlen(note_name) + 1);
> +        note.hdr.n_namesz = cpu_to_be32(strlen(note_name));
>          note.hdr.n_descsz = cpu_to_be32(nf->contents_size);
> -        strncpy(note.name, note_name, sizeof(note.name));
> +        g_strlcpy(note.name, note_name, sizeof(note.name));
>          (*nf->note_contents_func)(&note, cpu, id);
>  
>          note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
> 

Re: [PATCH v2] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section

[Thomas Huth]

On 04/02/2021 18.00, Christian Borntraeger wrote:
> On 04.02.21 17:41, Thomas Huth wrote:
>> According to the "ELF-64 Object File Format" specification:
>>
>> "The first word in the entry, namesz, identifies the length, in
>>   bytes, of a name identifying the entry’s owner or originator. The name field
>>   contains a null-terminated string, with padding as necessary to ensure 8-
>>   byte alignment for the descriptor field. The length does not include the
>>   terminating null or the padding."
>>
>> So we should not include the terminating NUL in the length field here.
>>
>> Also there is a compiler warning with GCC 9.3 when compiling with
>> the -fsanitize=thread compiler flag:
>>
>>   In function 'strncpy',
>>      inlined from 's390x_write_elf64_notes' at ../target/s390x/arch_dump.c:219:9:
>>   /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
>>    '__builtin_strncpy' specified bound 8 equals destination size
>>    [-Werror=stringop-truncation]
>>
>> Since the name should always be NUL-terminated, let's use g_strlcpy() to
>> silence this warning. And while we're at it, also add an assert() to make
>> sure that the provided names always fit the size field (which is fine for
>> the current callers, the function is called once with "CORE" and once with
>> "LINUX" as a name).
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   v2: Use g_strlcpy instead of strncpy
> 
> 
> With this patch I do get
> 
> WARNING: possibly corrupt Elf64_Nhdr: n_namesz: 0 n_descsz: 4 n_type: 88
> 
> when running crash on the elf file created by dump-guest-memory. Without the
> patch everything is fine.

Drat! Looking at the crash sources:

  https://github.com/crash-utility/crash/blob/master/s390x.c#L378

... it seems like crash is rather rounding up to the next 4 bytes boundary 
instead of the next 8 bytes boundary. Thus things go wrong now when QEMU 
writes writes the "CORE" notes section. In the old code we were using 4 + 1 
as a lengths, so crash correctly rounded this up to 8. But now with 4 as a 
length, this does not work right anymore :-(

Seems like I either misunderstood the "ELF-64 Object File Format" 
specification, or this is a bug in the crash utility (it should either add 1 
to n_namesz for the trailing NUL or pad to 8 instead of 4)? Anyway, it's 
maybe better to keep the "+ 1" in QEMU for now to avoid breaking things, I 
guess?

  Thomas

Re: [PATCH v2] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section

[Christian Borntraeger]

On 05.02.21 07:12, Thomas Huth wrote:
> On 04/02/2021 18.00, Christian Borntraeger wrote:
>> On 04.02.21 17:41, Thomas Huth wrote:
>>> According to the "ELF-64 Object File Format" specification:
>>>
>>> "The first word in the entry, namesz, identifies the length, in
>>>   bytes, of a name identifying the entry’s owner or originator. The name field
>>>   contains a null-terminated string, with padding as necessary to ensure 8-
>>>   byte alignment for the descriptor field. The length does not include the
>>>   terminating null or the padding."
>>>
>>> So we should not include the terminating NUL in the length field here.
>>>
>>> Also there is a compiler warning with GCC 9.3 when compiling with
>>> the -fsanitize=thread compiler flag:
>>>
>>>   In function 'strncpy',
>>>      inlined from 's390x_write_elf64_notes' at ../target/s390x/arch_dump.c:219:9:
>>>   /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
>>>    '__builtin_strncpy' specified bound 8 equals destination size
>>>    [-Werror=stringop-truncation]
>>>
>>> Since the name should always be NUL-terminated, let's use g_strlcpy() to
>>> silence this warning. And while we're at it, also add an assert() to make
>>> sure that the provided names always fit the size field (which is fine for
>>> the current callers, the function is called once with "CORE" and once with
>>> "LINUX" as a name).
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>   v2: Use g_strlcpy instead of strncpy
>>
>>
>> With this patch I do get
>>
>> WARNING: possibly corrupt Elf64_Nhdr: n_namesz: 0 n_descsz: 4 n_type: 88
>>
>> when running crash on the elf file created by dump-guest-memory. Without the
>> patch everything is fine.
> 
> Drat! Looking at the crash sources:
> 
>  https://github.com/crash-utility/crash/blob/master/s390x.c#L378
> 
> ... it seems like crash is rather rounding up to the next 4 bytes boundary instead of the next 8 bytes boundary. Thus things go wrong now when QEMU writes writes the "CORE" notes section. In the old code we were using 4 + 1 as a lengths, so crash correctly rounded this up to 8. But now with 4 as a length, this does not work right anymore :-(
> 
> Seems like I either misunderstood the "ELF-64 Object File Format" specification, or this is a bug in the crash utility (it should either add 1 to n_namesz for the trailing NUL or pad to 8 instead of 4)? Anyway, it's maybe better to keep the "+ 1" in QEMU for now to avoid breaking things, I guess?

I guess kdump and friends are also doing the +1 otherwise we would see the error with those ELF dumps.
But yes, as long as crash does not work we must not apply this patch.

Re: [PATCH v2] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section

[Christian Borntraeger]

On 05.02.21 08:08, Christian Borntraeger wrote:
> 
> 
> On 05.02.21 07:12, Thomas Huth wrote:
>> On 04/02/2021 18.00, Christian Borntraeger wrote:
>>> On 04.02.21 17:41, Thomas Huth wrote:
>>>> According to the "ELF-64 Object File Format" specification:
>>>>
>>>> "The first word in the entry, namesz, identifies the length, in
>>>>   bytes, of a name identifying the entry’s owner or originator. The name field
>>>>   contains a null-terminated string, with padding as necessary to ensure 8-
>>>>   byte alignment for the descriptor field. The length does not include the
>>>>   terminating null or the padding."
>>>>
>>>> So we should not include the terminating NUL in the length field here.
>>>>
>>>> Also there is a compiler warning with GCC 9.3 when compiling with
>>>> the -fsanitize=thread compiler flag:
>>>>
>>>>   In function 'strncpy',
>>>>      inlined from 's390x_write_elf64_notes' at ../target/s390x/arch_dump.c:219:9:
>>>>   /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
>>>>    '__builtin_strncpy' specified bound 8 equals destination size
>>>>    [-Werror=stringop-truncation]
>>>>
>>>> Since the name should always be NUL-terminated, let's use g_strlcpy() to
>>>> silence this warning. And while we're at it, also add an assert() to make
>>>> sure that the provided names always fit the size field (which is fine for
>>>> the current callers, the function is called once with "CORE" and once with
>>>> "LINUX" as a name).
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>   v2: Use g_strlcpy instead of strncpy
>>>
>>>
>>> With this patch I do get
>>>
>>> WARNING: possibly corrupt Elf64_Nhdr: n_namesz: 0 n_descsz: 4 n_type: 88
>>>
>>> when running crash on the elf file created by dump-guest-memory. Without the
>>> patch everything is fine.
>>
>> Drat! Looking at the crash sources:
>>
>>  https://github.com/crash-utility/crash/blob/master/s390x.c#L378
>>
>> ... it seems like crash is rather rounding up to the next 4 bytes boundary instead of the next 8 bytes boundary. Thus things go wrong now when QEMU writes writes the "CORE" notes section. In the old code we were using 4 + 1 as a lengths, so crash correctly rounded this up to 8. But now with 4 as a length, this does not work right anymore :-(
>>
>> Seems like I either misunderstood the "ELF-64 Object File Format" specification, or this is a bug in the crash utility (it should either add 1 to n_namesz for the trailing NUL or pad to 8 instead of 4)? Anyway, it's maybe better to keep the "+ 1" in QEMU for now to avoid breaking things, I guess?
> 
> I guess kdump and friends are also doing the +1 otherwise we would see the error with those ELF dumps.
> But yes, as long as crash does not work we must not apply this patch.

FWIW, readelf also complains:
Displaying notes found at file offset 0x000000b0 with length 0x000004d8:
  Owner                Data size 	Description
  CORE                 0x00000150	NT_PRSTATUS (prstatus structure)
  (NONE)               0x00000004	Unknown note type: (0x00000088)
   description data: 00 00 00 02 
readelf: dump.mem2: Warning: note with invalid namesz and/or descsz found at offset 0x170
readelf: dump.mem2: Warning:  type: 0x0, namesize: 0x434f5245, descsize: 0x00000000, alignment: 4

Re: [PATCH v2] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section

[Thomas Huth]

On 05/02/2021 08.08, Christian Borntraeger wrote:
> 
> 
> On 05.02.21 07:12, Thomas Huth wrote:
>> On 04/02/2021 18.00, Christian Borntraeger wrote:
>>> On 04.02.21 17:41, Thomas Huth wrote:
>>>> According to the "ELF-64 Object File Format" specification:
>>>>
>>>> "The first word in the entry, namesz, identifies the length, in
>>>>    bytes, of a name identifying the entry’s owner or originator. The name field
>>>>    contains a null-terminated string, with padding as necessary to ensure 8-
>>>>    byte alignment for the descriptor field. The length does not include the
>>>>    terminating null or the padding."
>>>>
>>>> So we should not include the terminating NUL in the length field here.
>>>>
>>>> Also there is a compiler warning with GCC 9.3 when compiling with
>>>> the -fsanitize=thread compiler flag:
>>>>
>>>>    In function 'strncpy',
>>>>       inlined from 's390x_write_elf64_notes' at ../target/s390x/arch_dump.c:219:9:
>>>>    /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
>>>>     '__builtin_strncpy' specified bound 8 equals destination size
>>>>     [-Werror=stringop-truncation]
>>>>
>>>> Since the name should always be NUL-terminated, let's use g_strlcpy() to
>>>> silence this warning. And while we're at it, also add an assert() to make
>>>> sure that the provided names always fit the size field (which is fine for
>>>> the current callers, the function is called once with "CORE" and once with
>>>> "LINUX" as a name).
>>>>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>    v2: Use g_strlcpy instead of strncpy
>>>
>>>
>>> With this patch I do get
>>>
>>> WARNING: possibly corrupt Elf64_Nhdr: n_namesz: 0 n_descsz: 4 n_type: 88
>>>
>>> when running crash on the elf file created by dump-guest-memory. Without the
>>> patch everything is fine.
>>
>> Drat! Looking at the crash sources:
>>
>>   https://github.com/crash-utility/crash/blob/master/s390x.c#L378
>>
>> ... it seems like crash is rather rounding up to the next 4 bytes boundary instead of the next 8 bytes boundary. Thus things go wrong now when QEMU writes writes the "CORE" notes section. In the old code we were using 4 + 1 as a lengths, so crash correctly rounded this up to 8. But now with 4 as a length, this does not work right anymore :-(
>>
>> Seems like I either misunderstood the "ELF-64 Object File Format" specification, or this is a bug in the crash utility (it should either add 1 to n_namesz for the trailing NUL or pad to 8 instead of 4)? Anyway, it's maybe better to keep the "+ 1" in QEMU for now to avoid breaking things, I guess?
> 
> I guess kdump and friends are also doing the +1 otherwise we would see the error with those ELF dumps.
> But yes, as long as crash does not work we must not apply this patch.

Looking at the other target/*/arch_dump.c files, it also seems like they 
include the NUL in the namesz field for their 64-bit flavours ... I've got 
the feeling that the "ELF-64 Object File Format" spec simply does not apply 
here, or I must have missed something else. Big sorry for the noise, I 
should have tested this more thoroughly before sending out the patch!
I'll respin with just the change to strncpy instead...

  Thomas

Re: [PATCH v2] target/s390x/arch_dump: Fixes for the name field in the PT_NOTE section

[Thomas Huth]

On 05/02/2021 09.18, Thomas Huth wrote:
> On 05/02/2021 08.08, Christian Borntraeger wrote:
>>
>>
>> On 05.02.21 07:12, Thomas Huth wrote:
>>> On 04/02/2021 18.00, Christian Borntraeger wrote:
>>>> On 04.02.21 17:41, Thomas Huth wrote:
>>>>> According to the "ELF-64 Object File Format" specification:
>>>>>
>>>>> "The first word in the entry, namesz, identifies the length, in
>>>>>    bytes, of a name identifying the entry’s owner or originator. The 
>>>>> name field
>>>>>    contains a null-terminated string, with padding as necessary to 
>>>>> ensure 8-
>>>>>    byte alignment for the descriptor field. The length does not include 
>>>>> the
>>>>>    terminating null or the padding."
>>>>>
>>>>> So we should not include the terminating NUL in the length field here.
>>>>>
>>>>> Also there is a compiler warning with GCC 9.3 when compiling with
>>>>> the -fsanitize=thread compiler flag:
>>>>>
>>>>>    In function 'strncpy',
>>>>>       inlined from 's390x_write_elf64_notes' at 
>>>>> ../target/s390x/arch_dump.c:219:9:
>>>>>    /usr/include/x86_64-linux-gnu/bits/string_fortified.h:106:10: error:
>>>>>     '__builtin_strncpy' specified bound 8 equals destination size
>>>>>     [-Werror=stringop-truncation]
>>>>>
>>>>> Since the name should always be NUL-terminated, let's use g_strlcpy() to
>>>>> silence this warning. And while we're at it, also add an assert() to make
>>>>> sure that the provided names always fit the size field (which is fine for
>>>>> the current callers, the function is called once with "CORE" and once with
>>>>> "LINUX" as a name).
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>    v2: Use g_strlcpy instead of strncpy
>>>>
>>>>
>>>> With this patch I do get
>>>>
>>>> WARNING: possibly corrupt Elf64_Nhdr: n_namesz: 0 n_descsz: 4 n_type: 88
>>>>
>>>> when running crash on the elf file created by dump-guest-memory. Without 
>>>> the
>>>> patch everything is fine.
>>>
>>> Drat! Looking at the crash sources:
>>>
>>>   https://github.com/crash-utility/crash/blob/master/s390x.c#L378
>>>
>>> ... it seems like crash is rather rounding up to the next 4 bytes 
>>> boundary instead of the next 8 bytes boundary. Thus things go wrong now 
>>> when QEMU writes writes the "CORE" notes section. In the old code we were 
>>> using 4 + 1 as a lengths, so crash correctly rounded this up to 8. But 
>>> now with 4 as a length, this does not work right anymore :-(
>>>
>>> Seems like I either misunderstood the "ELF-64 Object File Format" 
>>> specification, or this is a bug in the crash utility (it should either 
>>> add 1 to n_namesz for the trailing NUL or pad to 8 instead of 4)? Anyway, 
>>> it's maybe better to keep the "+ 1" in QEMU for now to avoid breaking 
>>> things, I guess?
>>
>> I guess kdump and friends are also doing the +1 otherwise we would see the 
>> error with those ELF dumps.
>> But yes, as long as crash does not work we must not apply this patch.
> 
> Looking at the other target/*/arch_dump.c files, it also seems like they 
> include the NUL in the namesz field for their 64-bit flavours ... I've got 
> the feeling that the "ELF-64 Object File Format" spec simply does not apply 
> here, or I must have missed something else.
After searching for a little bit longer, I guess this is the spec that 
applies nowadays:

  http://www.sco.com/developers/gabi/latest/ch5.pheader.html#note_section

Citing:

  "The first namesz bytes in name contain a null-terminated character 
representation of the entry's owner or originator."

So the NUL should be included in the namesz field, indeed.

  Thomas