[PATCH] efi_loader: Allow overlapped extra data for PE hashing

[Heinrich Schuchardt]

On 6/27/22 05:43, Su, Bao Cheng wrote:
> On Fri, 2022-06-24 at 11:44 +0200, Jan Kiszka wrote:
>> On 24.06.22 10:53, Heinrich Schuchardt wrote:
>>> On 6/24/22 07:32, Su, Bao Cheng wrote:
>>>> During PE hashing, when holes exists between sections, the extra
>>>> data
>>>> calculated could be a dupulicated region of the last section.
>>>>
>>>> Such PE image with holes existing between sections may contain
>>>> the
>>>> symbol table for the kernel, for example.
>>>>
>>>> The Authenticode_PE spec does not rule how to deal with such
>>>> scenario,
>>>> however, other tools such as pesign and sbsign both have the
>>>> overlapped
>>>
>>> Thanks for analyzing differences in hashing.
>>>
>>> Above you mention holes between sections. Here you talk about
>>> overlapping sections. These two cases are obviously distinct.
>>>
>>> Please, provide an accurate description.
>>
>> Yeah, I also gave that feedback internally already as it left me a
>> bit
>> confused.
>>
>>>
>>> Examples (in text form) would be helpful.
>>
>> There is apparently no good PE dump tooling available, so I try to
>> describe our scenario verbally:

You could try https://github.com/xypron/efi_analyzer.

>>
>> We are generating a unified kernel image, similar to what systemd
>> does,
>> for ARM and ARM64 [1]. The stub has .text and .data sections, and
>> then
>> follows the symbol table (some versions of binutils allow to suppress
>> it, other not, sigh). When appending the actual payload to that
>> (kernel
>> image, command line, initrd, dtbs), those sections are added right
>> after
>> the symbol table, creating an unhashed gap between the last stub
>> section
>> and the first appended one. That unified linux.efi is then signed and
>> should be verifiable and bootable (as it is with EDK2).
>>
>
> I will try to give a more straightforward description, considering
> below PE image:
>
> ## PE Header:
>    @0x00000000
>     ...
> ### Section Header 1:
>    ...
>    @0x00000108 : 0x00008000             - SizeOfRawData
>    @0x0000010C : 0x00001000             - PointerToRawData
>    ...
> ### Section Header 2:
>    ...
>    @0x00000130 : 0x00001C00             - SizeOfRawData
>    @0x00000134 : 0x00009000             - PointerToRawData
>    ...
> ### Section Header 3:
>    ...
>    @0x00000158 : 0x00001200             - SizeOfRawData
>    @0x0000015C : 0x0000B200             - PointerToRawData
>    ...
>
>  From the section headers, the end offset of section 2 is 0x1C00 +
> 0x9000 = 0xAC00, however, the start offset of the section 3 is 0xB200,
> there is a `hole` here of size 0x600 bytes. In our case Jan has
> explained this is the symbol table.
>
> According to PE hasing spec, when finished the parsing of sections, the
> bytes_hashed should be calculated and compared to the (total PE size -
> auth size), and if the bytes_hashed is lesser, it means there are extra
> data need be hashed as well.
>
> According to spec, the offset of the extra data is set to bytes_hashed,
> this does not cause overlapping for a normal PE image without holes
> between sections, because the bytes_hashed is equal to the tail of the
> last section. However, for our case the extra data is the overlapped
> with the last section or sections, because the bytes_hashed is lesser
> than the tail of the last section due to the `hole`.
>
> U-Boot currently considers this part of data as overlapped and excludes
> them from the hashing, however other tools or BLs such as
> pesign/sbsign/EDK2 do not rule out the overlapped data, the hash result

"Overlap" means that bytes of the image belong to two sections.

An example of overlap would be:

section 1: 0x1000 - 0x2000
section 2: 0x1800 - 0x2800

"Gap" means that bytes between two sections don't belong to any section:

section1: 0x1000 - 0x2000
section2: 0x2800 - 0x3800

> stays consistent among these tools, although the last part is hashed
> twice indeed.

We will have to update U-Boot's unit tests to contain an example with a
gap. How do you create these files with a gap?

Best regards

Heinrich

>
> Baocheng
>
>> HTH,
>> Jan
>>
>> [1]
>> https://github.com/siemens/efibootguard/blob/master/docs/UNIFIED-KERNEL.md
>>
>