On Sun, 12 Mar 2023 at 13:02, Evgeniy Baskov <baskov@xxxxxxxxx> wrote: > > On 2023-03-11 20:31, Ard Biesheuvel wrote: > > On Sat, 11 Mar 2023 at 16:02, Evgeniy Baskov <baskov@xxxxxxxxx> wrote: > >> > >> On 2023-03-10 18:17, Ard Biesheuvel wrote: > >> > On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@xxxxxxxxx> wrote: > >> >> > >> >> Currently kernel image is not fully compliant PE image, so it may > >> >> fail to boot with stricter implementations of UEFI PE loaders. > >> >> > >> >> Set minimal alignments and sizes specified by PE documentation [1] > >> >> referenced by UEFI specification [2]. Align PE header to 8 bytes. > >> >> > >> >> Generate PE sections dynamically. This simplifies code, since with > >> >> current implementation all of the sections needs to be defined in > >> >> header.S, where most section header fields do not hold valid values, > >> >> except for their names. Before the change, it also held flags, > >> >> but now flags depend on kernel configuration and it is simpler > >> >> to set them from build.c too. > >> >> > >> >> Setup sections protection. Since we cannot fit every needed section, > >> >> set a part of protection flags dynamically during initialization. > >> >> This step is omitted if CONFIG_EFI_DXE_MEM_ATTRIBUTES is not set. > >> >> > >> >> [1] > >> >> https://download.microsoft.com/download/9/c/5/9c5b2167-8017-4bae-9fde-d599bac8184a/pecoff_v83.docx > >> >> [2] > >> >> https://uefi.org/sites/default/files/resources/UEFI_Spec_2_9_2021_03_18.pdf > >> >> > >> >> Tested-by: Peter Jones <pjones@xxxxxxxxxx> > >> >> Signed-off-by: Evgeniy Baskov <baskov@xxxxxxxxx> > >> > > >> > I would prefer it if we didn't rewrite the build tool this way. > >> > > >> > Having the sections in header.S in the order they appear in the binary > >> > is rather useful, and I don't think we should manipulate the section > >> > flags based on whether CONFIG_DXE_MEM_ATTRIBUTES is set. I also don't > >> > think we need more than .text / .,data (as discussed in the other > >> > thread on linux-efi@) > >> > > >> > Furthermore, I had a look at the audk PE loader [0], and I think it is > >> > being overly pedantic. > >> > > >> > The PE/COFF spec does not require that all sections are virtually > >> > contiguous, and it does not require that the file content is > >> > completely covered by either the header or by a section. > >> > > >> > So what I would prefer to do is the following: > >> > > >> > Sections: > >> > Idx Name Size VMA Type > >> > 0 .reloc 00000200 0000000000002000 DATA > >> > 1 .compat 00000200 0000000000003000 DATA > >> > 2 .text 00bee000 0000000000004000 TEXT > >> > 3 .data 00002200 0000000000bf2000 DATA > >> > > >> > using 4k section alignment and 512 byte file alignment, and a header > >> > size of 0x200 as before (This requires my patch that allows the setup > >> > header to remain unmapped when running the stub [1]) > >> > > >> > The reloc and compat payloads are placed at the end of the setup data > >> > as before, but increased in size to 512 bytes each, and then mapped > >> > non-1:1 into the RVA space. > >> > > >> > This works happily with both the existing PE loader as well as the > >> > audk one, but with the pedantic flags disabled. > >> > > >> > >> This makes sense. I'll change this patch to use this layout and > >> to keep sections in headers.S before sending v5. (and I guess I'll > >> make the compressed kernel a part of .text). I have a few questions > >> though: > >> > >> This layout assumes having the local copy of the bootparams as > >> in your RFC patches, right? > >> > > > > Indeed. Otherwise, the setup header may not have been copied to memory > > by the loader. > > > >> Can I keep the .rodata -- 5th section fits in the section table > >> without much work? > >> > > > > You could, but at least the current PE/COFF loader in EDK2 will map it > > read/write, as it only distinguishes between executable sections and > > non-executable sections. > > > > At least it will slightly improve security for some implementations > (e.g. audk, while being overly strict support RO sections) > Yeah, but more common loaders will put the compressed data in a writable region. I'd prefer to have a simple and common baseline where we always just use R-X for all text and rodata, and RW- for everything else.
