On 3 May 2016 at 16:01, Laszlo Ersek <[email protected]> wrote: > On 05/03/16 14:45, Ard Biesheuvel wrote: >> This reduces the amount of memory mapped as both writable and executable >> to the absolute minimum, by mapping all of memory non-executable by >> default, and using PermissionsPeCoffExtraActionLib to only map those >> regions executable that require it for execution. If possible, these >> regions are remapped read-only at the same time, but in some cases >> (runtime drivers, TE images, images with < 4 KB section alignment) we >> cannot avoid having to map the entire PE/COFF image RWX. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <[email protected]> >> --- >> ArmVirtPkg/ArmVirtQemu.dsc | 9 >> ++++++++- >> ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 3 +++ >> 2 files changed, 11 insertions(+), 1 deletion(-) > > (1) Could you include a comprehensive table in the commit message that > describes what handling affects what driver type, and where each > treatment is established? >
Sure > For example, you mention TE images as exempt. According to > "ArmVirtQemu.fdf", this means SEC, PEI_CORE, PEIM. However, below you > link the library into PeiMain.inf (i.e., the PEI_CORE) as well. Why is > that correct? > PeiCore and DxeCore both contain PE/COFF loaders, which is why they need this library resolution. This is independent of whether these modules are TE themselves. > Also, why must e.g. runtime drivers be mapped fully RWX? > Runtime drivers are relocated a second time when SetVirtualAddressMap() is called, and the boot time MMU configuration may still be live at this point. Since these relocations point into the .text section as well (e.g., to relocate const structures containing function pointers), they cannot be mapped read-only. This is a bit disappointing, actually, and I wonder if there is a better solution. In fact, i was hoping for some discussion on this topic in general, not necessarily on the patches. > (2) Shouldn't ArmVirtQemuKernel.dsc be modified as well? (Maybe not > identically, since it runs entirely from DRAM, but still?) > The fact that it runs from DRAM is a problem. In fact, this patch will break if applies as is. The reason is that remapping all of RAM noexec will affect the code that is currently being executed. Here, ArmVirtQemu has a big advantage since NOR flash is R-X by nature. > (3) I think it's worth keeping in mind that on X64 and Ia32, some > drivers generate code dynamically. For example, in the implementation of > EFI_MP_SERVICES_PROTOCOL. I don't think it should block this series, but > perhaps make a note somewhere (code? commit message?) that such drivers > will have to massage their mappings individually on aarch64. > This is exactly the kind of feedback i was hoping for (hence the RFC in the subject line). There may be other cases where executable code is copied (and now that I think of it, we have such code in AArch64 as well). When it is arch-specific code, that can be handled ad hoc, but if there are generic ways of putting code on the heap, it would require some standardized protocol to manage this. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
