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

[Christian Borntraeger <borntraeger@de.ibm.com>]

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