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

[PATCH] 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, we can simply decrease
the size of the strncpy by one here 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>
---
 The ELF-64 spec can be found here, for example:
 https://uclibc.org/docs/elf-64-gen.pdf

 Here's a CI run with the compiler warning:
 https://gitlab.com/huth/qemu/-/jobs/1003508341#L1248

 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..20c3a09707 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));
+        strncpy(note.name, note_name, sizeof(note.name) - 1);
         (*nf->note_contents_func)(&note, cpu, id);
 
         note_size = sizeof(note) - sizeof(note.contents) + nf->contents_size;
-- 
2.27.0

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

[Christian Borntraeger]

On 03.02.21 10:44, 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, we can simply decrease
> the size of the strncpy by one here 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>
> ---
>  The ELF-64 spec can be found here, for example:
>  https://uclibc.org/docs/elf-64-gen.pdf
> 
>  Here's a CI run with the compiler warning:
>  https://gitlab.com/huth/qemu/-/jobs/1003508341#L1248
> 
>  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..20c3a09707 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));
> +        strncpy(note.name, note_name, sizeof(note.name) - 1);

This kind of feels wrong. With 8 bytes of note.name, we should be able to store "Test123" + the final \0.
Now we tell strncpy to stop at 7. Which means that for Test123 we would NOT copy the final \0.

So I actually think that the sanitize warning is wrong (as long as the assertion holds true).

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

[Thomas Huth]

On 04/02/2021 14.01, Christian Borntraeger wrote:
> 
> 
> On 03.02.21 10:44, 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, we can simply decrease
>> the size of the strncpy by one here 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>
>> ---
>>   The ELF-64 spec can be found here, for example:
>>   https://uclibc.org/docs/elf-64-gen.pdf
>>
>>   Here's a CI run with the compiler warning:
>>   https://gitlab.com/huth/qemu/-/jobs/1003508341#L1248
>>
>>   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..20c3a09707 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));
>> +        strncpy(note.name, note_name, sizeof(note.name) - 1);
> 
> This kind of feels wrong. With 8 bytes of note.name, we should be able to store "Test123" + the final \0.
> Now we tell strncpy to stop at 7. Which means that for Test123 we would NOT copy the final \0.

Yes, but there is a memset(&note, 0, sizeof(note)) at the beginning of the 
for-loop, so the 8th byte should always be set to 0 anyway. But if you 
prefer, I can also simply use g_strlcpy() instead...?

  Thomas

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

[Christian Borntraeger]

On 04.02.21 14:09, Thomas Huth wrote:
> On 04/02/2021 14.01, Christian Borntraeger wrote:
>>
>>
>> On 03.02.21 10:44, 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, we can simply decrease
>>> the size of the strncpy by one here 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>
>>> ---
>>>   The ELF-64 spec can be found here, for example:
>>>   https://uclibc.org/docs/elf-64-gen.pdf
>>>
>>>   Here's a CI run with the compiler warning:
>>>   https://gitlab.com/huth/qemu/-/jobs/1003508341#L1248
>>>
>>>   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..20c3a09707 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));
>>> +        strncpy(note.name, note_name, sizeof(note.name) - 1);
>>
>> This kind of feels wrong. With 8 bytes of note.name, we should be able to store "Test123" + the final \0.
>> Now we tell strncpy to stop at 7. Which means that for Test123 we would NOT copy the final \0.
> 
> Yes, but there is a memset(&note, 0, sizeof(note)) at the beginning of the for-loop, so the 8th byte should always be set to 0 anyway. But if you prefer, I can also simply use g_strlcpy() instead...?

This might be better as gcc also warns for the "it fits perfectly case" when we do not copy the \0.
While the following case should not trigger with todays QEMU code it shows the issue.


$ gcc -W -Wall test.c 
test.c: In function ‘main’:
test.c:7:2: warning: ‘strncpy’ output truncated before terminating nul copying 7 bytes from a string of the same length [-Wstringop-truncation]
    7 |  strncpy(buf, "Test123", 7);
      |  ^~~~~~~~~~~~~~~~~~~~~~~~~~
$ cat test.c
#include <string.h>

int main()
{

	char buf[8];
	strncpy(buf, "Test123", 7);
}