在 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
>
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
