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 <mhaeu...@posteo.de>
Sent: Friday, August 27, 2021 2:33 AM
To: devel@edk2.groups.io; Ni, Ray <ray...@intel.com>
Cc: Andrew Fish <af...@apple.com>; l...@nuviainc.com; Kinney, Michael D 
<michael.d.kin...@intel.com>; Gao, Zhichao
<zhichao....@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A 
<hao.a...@intel.com>; Bi, Dandan
<dandan...@intel.com>; Dong, Eric <eric.d...@intel.com>; Bret Barkelew 
<bret.barke...@microsoft.com>; Vitaly Cheptsov
<vit9...@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








-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79863): https://edk2.groups.io/g/devel/message/79863
Mute This Topic: https://groups.io/mt/85147044/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to