[Crash-utility] Crash-utility Digest, Vol 178, Issue 21

[lijiang]

在 2020年08月07日 18:33, Mathias Krause 写道:
> Am 07.08.20 um 07:52 schrieb lijiang:
>>>> [...]
>>>> For the Program Header table, could it be optional? If present, the value of
>>>> e_phoff should be non-zero, otherwise its value is zero. Would it be better
>>>> to check if the value of e_phoff is valid?
>>>
>>> We already ensure the file has at least two program headers by testing
>>> e_phnum to be >= 2 before. So program headers aren't optional any more
>>> when reaching this code.
>>
>> Thank you for the explanation in detail.
>>
>>> Admitted, while it would be rather unusual to have the program headers
>>> starting at file offset 0, I don't think we should decline parsing such
>>
>> Only the ELF header has a fixed position in the file, and an ELF header always
>> resides at the beginning. If e_phoff is zero, there may be something wrong.
> 
> True that. But checking e_phoff just for 0 isn't sufficient either. If
> it overlaps with the ELF header (or any other headers or sections), the
> ELF file might be broken. Though, I don't want to write all the sanity
> checks that would be required, as this needs parsing, interpreting and
> sanity checking a lot more values which seems way too complicated for
> this simple use case. I mean, we don't even check if the first program
> header actually is a PT_NOTE one -- simply assume it and skip it right
> away. So adding partial sanity checks here doesn't make much sense,
> IMHO. Adding a warning if e_phoff == 0 is fine with me, though.
> 
>>> files just because. IMHO, it's rather more sensible to test for the
>>> minimal number of program headers -- as we already do -- and otherwise
>>> take e_phoff as-is while still ensuring it's within the file's size
>>> limit. We could add a warning if e_phoff is zero, though.
>>>
>>>>
>>>>> +		if (elf32->e_phoff != sizeof(Elf32_Ehdr)) {
>>>>> +			if (CRASHDEBUG(1))
>>>>> +				error(WARNING, "%s: first PHdr not following "
>>>>> +					"EHdr (PHdr offset = %u)", file,
>>>
>>> Just noticed the missing '\n' here. Will fix in v2, if needed.
>>>
>> Yes.  Thanks.
> 
> Ok, will do.
> 
>> BTW: There are some code style issues such as the code indent, etc. I found out
>> its errors with the script tool(checkpatch.pl).
> 
> Yeah, I probably inherited them from the original source. crash is,
> apparently, very inconsistent about its tab and space usage. I tried to
> fix them for the lines I touched, but, apparently, missed a few cases.
OK, thanks.

> Will fix these as well. I won't, however, change the overlong lines
> warnings, as "fixing" these makes the code less readable.
> 
That's true.  Agree with you. :-)

>>>>> [...]
>>>>> diff --git a/netdump.h b/netdump.h
>>>>> index 7fa04f7c3a0f..844279bc4a00 100644
>>>>> --- a/netdump.h
>>>>> +++ b/netdump.h
>>>>> @@ -25,6 +25,8 @@
>>>>>          sizeof(Elf64_Ehdr)+sizeof(Elf64_Phdr)+sizeof(Elf64_Phdr)
>>>>>  #define MIN_NETDUMP_ELF_HEADER_SIZE \
>>>>>          MAX(MIN_NETDUMP_ELF32_HEADER_SIZE, MIN_NETDUMP_ELF64_HEADER_SIZE)
>>>>> +#define SAFE_NETDUMP_ELF_HEADER_SIZE \
>>>>> +	(MIN_NETDUMP_ELF_HEADER_SIZE+128)
>>>>>  
>>
>> Could it be better to define the macro like this? That can avoid using directly
>> the magic number.
>>
>>  #define MIN_NETDUMP_ELF32_HEADER_SIZE \
>> -        sizeof(Elf32_Ehdr)+sizeof(Elf32_Phdr)+sizeof(Elf32_Phdr)
>> +        sizeof(Elf32_Ehdr)+sizeof(Elf32_Phdr)+sizeof(Elf32_Phdr)+sizeof(Elf32_Shdr)
>>  #define MIN_NETDUMP_ELF64_HEADER_SIZE \
>> -        sizeof(Elf64_Ehdr)+sizeof(Elf64_Phdr)+sizeof(Elf64_Phdr)
>> +        sizeof(Elf64_Ehdr)+sizeof(Elf64_Phdr)+sizeof(Elf64_Phdr)+sizeof(Elf64_Shdr)
>>  #define MIN_NETDUMP_ELF_HEADER_SIZE \
>>          MAX(MIN_NETDUMP_ELF32_HEADER_SIZE, MIN_NETDUMP_ELF64_HEADER_SIZE)
> 
> Unfortunately that'll only work if there's just a single section header
> inbetween the ELF and the program headers -- as there was in my example.
> If there are more or others, it'll break again.
> 
> It also adds more implicit (and wrong!) knowledge that isn't obvious to
> the reader of that macros. As it reads above, one could come up with the
> following question:
> 
> Q: Why would we care about the section header following the two program
>    headers?
> A: We don't. It's actually an optional section header *preceding* the
>    program headers we want to skip over. Program headers are also likely
>    more than two, but we just care about the second one, assuming the
>    first on is the PT_NOTE one.
> 
> So no, I don't think the above is the better solution.
> 
If we have to use the size of 128, would you mind adding a new macro for it? For example:

+#define NETDUMP_ELF_SHDR_SIZE (128)
...
+#define SAFE_NETDUMP_ELF_HEADER_SIZE \
+       (MIN_NETDUMP_ELF_HEADER_SIZE+NETDUMP_ELF_SHDR_SIZE)

That can avoid using the Magic Number.

>> Actually, it is better not to assume its size, but to read out the ELF header firstly,
>> and then decide how to access the Phdr and Shdr Header based on the e_phnum, e_shnum,
>> e_phentsize, e_shentsize, e_phoff and e_shoff,etc. But, that will make the logic of
>> code become complicated.
> 
> I agree. That's why I simplified it all to allow the program headers to
> be up to 128 bytes away from their usual location. If that's not even
> the case, the error will tell the user and we can either adjust the
> number or rewrite that whole code to not assume a single buffer. But I'd
> rather postpone the latter till it actually hits a user.
> 
OK, let's leave it there and improve it in the future.

Thanks.
Lianbo
>>>> Can you please describe more details why the size of padding area is 128 bytes?
>>>> Are there any particular reasons?
>>>
>>> The reasoning behind the 128 bytes limit is simple: to not complicate
>>> things too much. ;)
>>> The rest of the code in netdump.c assumes the header is contiguous, i.e.
>>> the ELF header and the program headers can be read and put into a single
>>> (small) buffer. Assuming the "padding" after the ELF header to the first
>>> program header isn't all that big in the exceptional case made me go for
>>> this arbitrary (but effective) limit.
>>>
>>> The padding I've observed is as follows:
>>>
>>> $ readelf -h vmw.core
>>> ELF Header:
>>>   Magic:   7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00
>>>   Class:                             ELF64
>>>   Data:                              2's complement, little endian
>>>   Version:                           1 (current)
>>>   OS/ABI:                            UNIX - System V
>>>   ABI Version:                       0
>>>   Type:                              CORE (Core file)
>>>   Machine:                           Advanced Micro Devices X86-64
>>>   Version:                           0x1
>>>   Entry point address:               0x0
>>>   Start of program headers:          128 (bytes into file)
>>>   Start of section headers:          64 (bytes into file)
>>>   Flags:                             0x0
>>>   Size of this header:               64 (bytes)
>>>   Size of program headers:           56 (bytes)
>>>   Number of program headers:         8
>>>   Size of section headers:           64 (bytes)
>>>   Number of section headers:         1
>>>   Section header string table index: 0
>>>
>>> So here the section headers have be generated in front of the program
>>
>> It may be related to the method of generating ELF Header files.
> 
> Yes. Section headers seems to have been generated first. But that's
> legit. There's no fixed ordering required.
> 
>>> headers. Thereby the "padding bytes" are actually the section headers,
>>> occupying 64 bytes, making the program headers start at offset 128
>>> instead of the "usual" 64.
>>>
>> The section header table may be optional, sometimes.
> 
> Yes, sure.
> 
> Thanks,
> Mathias
>