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