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 <th...@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;
>