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