Re: [edk2-devel] [RFC] Expose HII package list via C variables

["Marvin Häuser" <mhaeuser@posteo.de>]

On 26/08/2021 20:38, Ni, Ray wrote:
> Marvin,
> If we can keep the secure parsing code inside the PeCoff library, all the backward combability issues because of removing the HII section support don't exist.

Hey Ray,

As I said, the updated proposal no longer has any impact on 
backwards-compatibility. For the new file format, there is still a big, 
big amount of time to reconsider later. Samer said the usage of this 
feature is declining. For now the comments from the downstreams really 
helped me getting an idea of what I am dealing with.

> The PeCoff library might be very complex but the complexity[1] is hidden inside and abstracted by the PeCoff library API.

Well, bugs hidden behind a nice API are still bugs, it's not just about 
the callers. From the top of my head I can name you this:

"ResourceDirectory", the object the loop is based on [1], is overwritten 
within loop paths that may enter another iteration [2]. With a 
well-formed file this will still work, and with a malformed file it at 
least will not cause dangerous behaviour, but it is still obvious that 
this is not the way this is meant to work.

This is obviously an easy fix and not a bud bug at all, but my point is 
there more code there is, the more slips through review. There are 
general parsing safety issues too.

Another interesting thing, not related to HII but somewhat related to 
the "file format assumptions" point is that there now is an ELF loader 
in PEI phase [3], but despite an attempt at abstraction with the 
LoadFile PPI, there is a clear "PEIM = PE/COFF" assumption in PeiCore 
[4]. Is this a real-world bug? I don't know, I cannot tell because the 
code is too complex. :)

> The complexity[2] of resolving combability issues after dropping HII section support disappears.
>
> The complexity[1] is a fixed value but [2] is hard to calculate because there are so many platforms that might use this feature.

Yes, hence I am happy about all the comments I could gather so far!

Best regards,
Marvin


[1]
https://github.com/tianocore/edk2/blob/6c8dd15c4ae42501438a525ec41299f365f223cb/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1549

[2]
https://github.com/tianocore/edk2/blob/6c8dd15c4ae42501438a525ec41299f365f223cb/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1576
https://github.com/tianocore/edk2/blob/6c8dd15c4ae42501438a525ec41299f365f223cb/MdePkg/Library/BasePeCoffLib/BasePeCoff.c#L1593

[3]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/UefiPayloadPkg/PayloadLoaderPeim/PayloadLoaderPeim.c#L159-L161

[4]
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Core/Pei/Image/Image.c#L910
https://github.com/tianocore/edk2/blob/7b4a99be8a39c12d3a7fc4b8db9f0eab4ac688d5/MdeModulePkg/Core/Pei/Image/Image.c#L799-L848

>
>> -----Original Message-----
>> From: Marvin Häuser <mhaeuser@posteo.de>
>> Sent: Friday, August 27, 2021 2:33 AM
>> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
>> Cc: Andrew Fish <afish@apple.com>; leif@nuviainc.com; Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Zhichao
>> <zhichao.gao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan
>> <dandan.bi@intel.com>; Dong, Eric <eric.dong@intel.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Vitaly Cheptsov
>> <vit9696@protonmail.com>
>> Subject: Re: [edk2-devel] [RFC] Expose HII package list via C variables
>>
>> On 26/08/2021 20:19, Ni, Ray wrote:
>>>> 3. Saves error-prone parsing work
>>> This might be the key reason? Is it theoretically possible to write a secure parsing code?
>> Hey Ray,
>>
>> Yes it is, but the most secure parsing code is that which doesn't exist.
>> I'm confident in the one I wrote for the new PE/COFF library, but if
>> variables suit the need, I really would like to use them, even if it is
>> only by a toggle. The new proposal (please check the chain with Mike)
>> was adapted to preserve the old behaviour fully for any platform that
>> wants it, permanently. Amaranth will very likely drop support for the
>> PE/COFF section, either nicely with an upstream PCD, or not-so-nicely
>> with maintaining patches to remove the functionality downstream.
>>
>> In the same chain I outlined I'm sketching a new terse file format and
>> I'd prefer to avoid any unnecessary data or parsing burden. The current
>> library I am sketching privately is *much* smaller than both the current
>> and the new PE/COFF library. The overall format in an UEFI context (no
>> dynamic linking etc.) is at least as powerful as PE/COFF and pretty much
>> always smaller than TE. I'd like to keep it that way. :)
>>
>> Best regards,
>> Marvin
>>
>>> Thanks,
>>> Ray
>>>
>>>
>>> 
>>>
>>>