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

[Christian Borntraeger]

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.