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