PE/COFF image sections have two sizes, the "raw" size (i.e. the size section data occupies in a stored file) and "virtual" size (i.e. the size section data occupies in a loaded image). There is no strict relation between them, i.e. raw size can be lesser than, equal to, or greater than virtual size. As memory protection always works on loaded images, virtual size needs to be used to ensure adequate permissions. Yet, the current code uses the raw file size[1]. [1] https://github.com/tianocore/edk2/blob/2072c22a0d63c780b0cc6377f6d4ffb116ad6144/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c#L528
Moving this to CONFIRMED, per discussion in the 5/5 Infosec meeting.
Created attachment 1332 [details] suggested patch Simple one-line fix, not applied for almost two years. May I ask what the heck you are waiting for?
Sorry Gerd, not terribly interested in working on this code. Applying red tape could fool people into believing it’s adequate afterwards. :)
(In reply to Marvin Häuser from comment #3) > Sorry Gerd, not terribly interested in working on this code. Oh, that question was directed to bug assignee and SecurityPkg maintainer. Caring about bugs, especially security bugs, is their duty IMHO. Bug reporter also providing fixes is a nice-to-have, but certainly not required. > Applying red tape could fool people into believing it’s adequate afterwards. :) Yea, no doubt there is more, looking at the list of open security bugs there are multiple PE-related ones and I have my doubts that this is complete. I guess your recommendation is to swap the complete image loader instead?
Actually both interpretations are valid. While replacing the image loader also basically is mandatory - I got a “we don’t care about the details, just give us literally anything else” from multiple core contributors back then -, this sub-code in particular is its own design defect. Basically the notion of PE section permissions is ignored entirely and a heuristic is applied to divide the image into code (RX) and data (RW) segments, with no support for configurations like RNX whatsoever. I don’t remember the other issues with it, but I’m pretty sure there were more. But yes, my image loader replacement proposal did fix this implicitly and is now maintained in a downstream fork, fixing this BZ, most of my other BZs, and more at: https://github.com/acidanthera/audk Mainlining it was shelved after an uncontroversial no-op (w.r.t. the compiled result) patch with prerequisites was not merged after extensive discussion, one R-b, a ping by me, a ping by a colleague, and a real-world toolchain bug it was able to discover via the STATIC_ASSERTs (which also had its fix ignored at the time): https://edk2.groups.io/g/devel/topic/84909448 Please just unembargo and mark as WONTFIX.
As recommended by Marvin, unembargo and mark as WONTFIX.
