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

[Christian Borntraeger]

On 05.02.21 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.

FWIW, readelf also complains:
Displaying notes found at file offset 0x000000b0 with length 0x000004d8:
  Owner                Data size        Description
  CORE                 0x00000150       NT_PRSTATUS (prstatus structure)
  (NONE)               0x00000004       Unknown note type: (0x00000088)
   description data: 00 00 00 02 
readelf: dump.mem2: Warning: note with invalid namesz and/or descsz found at 
offset 0x170
readelf: dump.mem2: Warning:  type: 0x0, namesize: 0x434f5245, descsize: 
0x00000000, alignment: 4