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

[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.
Will fix these as well. I won't, however, change the overlong lines
warnings, as "fixing" these makes the code less readable.

>>>> [...]
>>>> 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.

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

>>> 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