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

[Thomas Huth <thuth@redhat.com>]

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