| Message ID | 20230308202209.2980947-1-ardb@kernel.org |
|---|---|
| Headers | show |
| Series | efi: x86: Use strict W^X mappings in PE/COFF header | expand |
On Wed, 8 Mar 2023 at 21:22, Ard Biesheuvel <ardb@kernel.org> wrote: > > We are currently limited in the number of PE/COFF sections we can > describe in the PE header, due to lack of space. This is caused by the > presence of the setup header at offset 0x1f1, leaving only the space > before it for PE metadata. > > However, now that we no longer copy the setup_header from this part of > the image for use by the EFI stub, we no longer have to describe it as > part of the loadable image. This means we can put the PE header *after* > the setup header, and use as much space as we like. It also means we > don't have to describe this part of the image in PE/COFF, and simply > treat it as part of the header. This means we can drop the ".setup" > section as well. > Better idea: let's just rip out the ancient real mode boot code. It's 20+ years old and only prints an error message in case the kernel is booted in a way that has not been supported for all that time. Comments anyone? > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > arch/x86/boot/header.S | 26 +++----------------- > arch/x86/boot/setup.ld | 1 + > arch/x86/boot/tools/build.c | 11 +++------ > 3 files changed, 9 insertions(+), 29 deletions(-) > > diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S > index 9338c68e7413d6e6..aba499404d8b870e 100644 > --- a/arch/x86/boot/header.S > +++ b/arch/x86/boot/header.S > @@ -85,7 +85,7 @@ bs_die: > # Offset to the PE header. > # > .long LINUX_PE_MAGIC > - .long pe_header > + .long pe_header - bootsect_start > #endif /* CONFIG_EFI_STUB */ > > .section ".bsdata", "a" > @@ -96,6 +96,8 @@ bugger_off_msg: > .byte 0 > > #ifdef CONFIG_EFI_STUB > + .section ".peheader", "a" > + .align 8 > pe_header: > .long PE_MAGIC > > @@ -161,7 +163,7 @@ extra_header_fields: > # > .long 0 # SizeOfImage > > - .long 0x200 # SizeOfHeaders > + .long 0x800 # SizeOfHeaders > .long 0 # CheckSum > .word IMAGE_SUBSYSTEM_EFI_APPLICATION # Subsystem (EFI application) > #ifdef CONFIG_EFI_DXE_MEM_ATTRIBUTES > @@ -192,26 +194,6 @@ extra_header_fields: > > # Section table > section_table: > - # > - # The offset & size fields are filled in by build.c. > - # > - .ascii ".setup" > - .byte 0 > - .byte 0 > - .long 0 > - .long 0x0 # startup_{32,64} > - .long 0 # Size of initialized data > - # on disk > - .long 0x0 # startup_{32,64} > - .long 0 # PointerToRelocations > - .long 0 # PointerToLineNumbers > - .word 0 # NumberOfRelocations > - .word 0 # NumberOfLineNumbers > - .long IMAGE_SCN_CNT_CODE | \ > - IMAGE_SCN_MEM_READ | \ > - IMAGE_SCN_MEM_EXECUTE | \ > - IMAGE_SCN_ALIGN_16BYTES # Characteristics > - > # > # The EFI application loader requires a relocation section > # because EFI applications must be relocatable. The .reloc > diff --git a/arch/x86/boot/setup.ld b/arch/x86/boot/setup.ld > index 49546c247ae25e97..5981287bbcb7f439 100644 > --- a/arch/x86/boot/setup.ld > +++ b/arch/x86/boot/setup.ld > @@ -16,6 +16,7 @@ SECTIONS > . = 495; > .header : { *(.header) } > .entrytext : { *(.entrytext) } > + .peheader : { *(.peheader) } > .inittext : { *(.inittext) } > .initdata : { *(.initdata) } > __end_init = .; > diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c > index e6fd09789482ed04..883e6359221cd588 100644 > --- a/arch/x86/boot/tools/build.c > +++ b/arch/x86/boot/tools/build.c > @@ -296,16 +296,13 @@ static void update_pecoff_section_header(char *section_name, uint32_t offset, ui > update_pecoff_section_header_fields(section_name, offset, size, size, offset); > } > > -static void update_pecoff_setup_and_reloc(unsigned int size) > +static void update_pecoff_reloc(unsigned int size) > { > - uint32_t setup_offset = SECTOR_SIZE; > uint32_t reloc_offset = size - PECOFF_RELOC_RESERVE - PECOFF_COMPAT_RESERVE; > #ifdef CONFIG_EFI_MIXED > uint32_t compat_offset = reloc_offset + PECOFF_RELOC_RESERVE; > #endif > - uint32_t setup_size = reloc_offset - setup_offset; > > - update_pecoff_section_header(".setup", setup_offset, setup_size); > update_pecoff_section_header(".reloc", reloc_offset, PECOFF_RELOC_RESERVE); > > /* > @@ -353,7 +350,7 @@ static unsigned int update_pecoff_sections(unsigned int text_start, unsigned int > * Size of code: Subtract the size of the first sector (512 bytes) > * which includes the header. > */ > - put_unaligned_le32(file_sz - SECTOR_SIZE + bss_sz, &hdr->text_size); > + put_unaligned_le32(text_sz + bss_sz, &hdr->text_size); > > /* Size of image */ > put_unaligned_le32(init_sz, &hdr->image_size); > @@ -407,7 +404,7 @@ static void efi_stub_entry_update(void) > > #else > > -static inline void update_pecoff_setup_and_reloc(unsigned int size) {} > +static inline void update_pecoff_reloc(unsigned int size) {} > static inline void update_pecoff_text(unsigned int text_start, > unsigned int file_sz, > unsigned int init_sz) {} > @@ -542,7 +539,7 @@ int main(int argc, char **argv) > #ifdef CONFIG_EFI_STUB > /* PE specification require 512-byte minimum section file alignment */ > kern_size = round_up(kern_file_size + 4, SECTOR_SIZE); > - update_pecoff_setup_and_reloc(setup_size); > + update_pecoff_reloc(setup_size); > #else > /* Number of 16-byte paragraphs, including space for a 4-byte CRC */ > kern_size = round_up(kern_file_size + 4, PARAGRAPH_SIZE); > -- > 2.39.2 >
On 2023-03-08 23:22, Ard Biesheuvel wrote: > This is a follow-up to work proposed by Evgeny to tighten memory > permissions used by the EFI stub and subsequently by the decompressor > on > x86. > > Instead of going out of our way to make more space in the first 500 > bytes of the image, and relying on non-1:1 mapped sections (which is > risky in the context of bespoke PE loaders), these patches reorganize > the header so the PE header comes after the x86 setup header, and can > be > extended at will. > > I pushed a branch at [1] that combines this with v4 of Evgeny's series > (after some minor surgery, e.g., to reorder the text and rodata > sections > so they are contiguous) > > We might split off the rodata section as well, and give it > read/non-exec > permissions, but I'd like to discuss the approach first, and perhaps > get > some testing data points. > > Cc: Evgeniy Baskov <baskov@ispras.ru> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Alexey Khoroshilov <khoroshilov@ispras.ru> > Cc: Peter Jones <pjones@redhat.com> > Cc: "Limonciello, Mario" <mario.limonciello@amd.com> > > [0] > https://lore.kernel.org/linux-efi/cover.1671098103.git.baskov@ispras.ru/ > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-x86-nx-v4 > > Ard Biesheuvel (4): > efi: x86: Use private copy of struct setup_header > efi: x86: Move PE header after setup header > efi: x86: Drop alignment section header flags > efi: x86: Split PE/COFF .text section into .text and .data > > arch/x86/boot/Makefile | 2 +- > arch/x86/boot/header.S | 52 +++++++++----------- > arch/x86/boot/setup.ld | 1 + > arch/x86/boot/tools/build.c | 38 +++++++++----- > drivers/firmware/efi/libstub/x86-stub.c | 43 +++------------- > 5 files changed, 59 insertions(+), 77 deletions(-) I've quickly looked through these patches but I'll do more testing tomorrow. This approach seems to be better than mine if it will work. I've tried the similar thing but I did not think of creating the local copy of the bootparams and the attempt to map them did not work since the PE loader I am trying to get kernel booting with does not accept sections before the PE header. But since the bootparams is inside the padding and is not used, it should be fine. But this will still need more changes to work properly with stricter PE loaders like the one that I've mentioned in my patch series [1]. The image should also have 4K aligned section virtual addresses and sizes (even on .reloc and .compat AFAIK), otherwise UEFI will ignore memory attributes (or refuse to load the kernel). Another desired thing is having adjacent section with no padding in between them, since [1] does have a mode that requires sections them to be adjacent. (SizeOfHeaders/header_size should also be set to the size of setup since it is also checked to be adjacent to the first section.) I did not do the one-to-one mapping of file and virtual addresses since it would require almost 4K paddings for the auxiliary sections. [1] https://github.com/acidanthera/audk/tree/secure_pe Thanks, Evgeniy Baskov
On Thu, 9 Mar 2023 at 18:59, Evgeniy Baskov <baskov@ispras.ru> wrote: > > On 2023-03-08 23:22, Ard Biesheuvel wrote: > > This is a follow-up to work proposed by Evgeny to tighten memory > > permissions used by the EFI stub and subsequently by the decompressor > > on > > x86. > > > > Instead of going out of our way to make more space in the first 500 > > bytes of the image, and relying on non-1:1 mapped sections (which is > > risky in the context of bespoke PE loaders), these patches reorganize > > the header so the PE header comes after the x86 setup header, and can > > be > > extended at will. > > > > I pushed a branch at [1] that combines this with v4 of Evgeny's series > > (after some minor surgery, e.g., to reorder the text and rodata > > sections > > so they are contiguous) > > > > We might split off the rodata section as well, and give it > > read/non-exec > > permissions, but I'd like to discuss the approach first, and perhaps > > get > > some testing data points. > > > > Cc: Evgeniy Baskov <baskov@ispras.ru> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Alexey Khoroshilov <khoroshilov@ispras.ru> > > Cc: Peter Jones <pjones@redhat.com> > > Cc: "Limonciello, Mario" <mario.limonciello@amd.com> > > > > [0] > > https://lore.kernel.org/linux-efi/cover.1671098103.git.baskov@ispras.ru/ > > [1] > > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-x86-nx-v4 > > > > Ard Biesheuvel (4): > > efi: x86: Use private copy of struct setup_header > > efi: x86: Move PE header after setup header > > efi: x86: Drop alignment section header flags > > efi: x86: Split PE/COFF .text section into .text and .data > > > > arch/x86/boot/Makefile | 2 +- > > arch/x86/boot/header.S | 52 +++++++++----------- > > arch/x86/boot/setup.ld | 1 + > > arch/x86/boot/tools/build.c | 38 +++++++++----- > > drivers/firmware/efi/libstub/x86-stub.c | 43 +++------------- > > 5 files changed, 59 insertions(+), 77 deletions(-) > > I've quickly looked through these patches but I'll do more testing > tomorrow. > > This approach seems to be better than mine if it will work. I've tried > the similar thing but I did not think of creating the local copy of the > bootparams and the attempt to map them did not work since the PE loader > I am trying to get kernel booting with does not accept sections before > the PE header. But since the bootparams is inside the padding and is > not used, it should be fine. > > But this will still need more changes to work properly with stricter PE > loaders like the one that I've mentioned in my patch series [1]. > > The image should also have 4K aligned section virtual addresses and > sizes > (even on .reloc and .compat AFAIK), otherwise UEFI will ignore memory > attributes (or refuse to load the kernel). EDK2 works fine as is, i.e. with only .text and .data aligned to 4k virtually, and the data size of .data aligned to 512 bytes. ProtectUefiImageCommon - 0x3C8600C0 - 0x0000000038777000 - 0x0000000002BC6000 SetUefiImageMemoryAttributes - 0x0000000038777000 - 0x0000000000004000 (0x0000000000004008) SetUefiImageMemoryAttributes - 0x000000003877B000 - 0x0000000000BEE000 (0x0000000000020008) SetUefiImageMemoryAttributes - 0x0000000039369000 - 0x0000000001FD4000 (0x0000000000004008) > Another desired thing is > having > adjacent section with no padding in between them, since [1] does have a > mode that requires sections them to be adjacent. Does that have any basis in the PE/COFF spec? > (SizeOfHeaders/header_size > should also be set to the size of setup since it is also checked to be > adjacent to the first section.) > Does that have any basis in the PE/COFF spec? > I did not do the one-to-one mapping of file and virtual addresses since > it > would require almost 4K paddings for the auxiliary sections. > > [1] https://github.com/acidanthera/audk/tree/secure_pe > I've backpedaled a little bit from this approach (see my other comment). If we just rip out the real mode stub, we can keep the PE header before the setup header, and simply describe whatever comes as .text.
On 2023-03-09 21:09, Ard Biesheuvel wrote: > On Thu, 9 Mar 2023 at 18:59, Evgeniy Baskov <baskov@ispras.ru> wrote: >> >> On 2023-03-08 23:22, Ard Biesheuvel wrote: >> > This is a follow-up to work proposed by Evgeny to tighten memory >> > permissions used by the EFI stub and subsequently by the decompressor >> > on >> > x86. >> > >> > Instead of going out of our way to make more space in the first 500 >> > bytes of the image, and relying on non-1:1 mapped sections (which is >> > risky in the context of bespoke PE loaders), these patches reorganize >> > the header so the PE header comes after the x86 setup header, and can >> > be >> > extended at will. >> > >> > I pushed a branch at [1] that combines this with v4 of Evgeny's series >> > (after some minor surgery, e.g., to reorder the text and rodata >> > sections >> > so they are contiguous) >> > >> > We might split off the rodata section as well, and give it >> > read/non-exec >> > permissions, but I'd like to discuss the approach first, and perhaps >> > get >> > some testing data points. >> > >> > Cc: Evgeniy Baskov <baskov@ispras.ru> >> > Cc: Borislav Petkov <bp@alien8.de> >> > Cc: Alexey Khoroshilov <khoroshilov@ispras.ru> >> > Cc: Peter Jones <pjones@redhat.com> >> > Cc: "Limonciello, Mario" <mario.limonciello@amd.com> >> > >> > [0] >> > https://lore.kernel.org/linux-efi/cover.1671098103.git.baskov@ispras.ru/ >> > [1] >> > https://git.kernel.org/pub/scm/linux/kernel/git/ardb/linux.git/log/?h=efi-x86-nx-v4 >> > >> > Ard Biesheuvel (4): >> > efi: x86: Use private copy of struct setup_header >> > efi: x86: Move PE header after setup header >> > efi: x86: Drop alignment section header flags >> > efi: x86: Split PE/COFF .text section into .text and .data >> > >> > arch/x86/boot/Makefile | 2 +- >> > arch/x86/boot/header.S | 52 +++++++++----------- >> > arch/x86/boot/setup.ld | 1 + >> > arch/x86/boot/tools/build.c | 38 +++++++++----- >> > drivers/firmware/efi/libstub/x86-stub.c | 43 +++------------- >> > 5 files changed, 59 insertions(+), 77 deletions(-) >> >> I've quickly looked through these patches but I'll do more testing >> tomorrow. >> >> This approach seems to be better than mine if it will work. I've tried >> the similar thing but I did not think of creating the local copy of >> the >> bootparams and the attempt to map them did not work since the PE >> loader >> I am trying to get kernel booting with does not accept sections before >> the PE header. But since the bootparams is inside the padding and is >> not used, it should be fine. >> >> But this will still need more changes to work properly with stricter >> PE >> loaders like the one that I've mentioned in my patch series [1]. >> >> The image should also have 4K aligned section virtual addresses and >> sizes >> (even on .reloc and .compat AFAIK), otherwise UEFI will ignore memory >> attributes (or refuse to load the kernel). > > EDK2 works fine as is, i.e. with only .text and .data aligned to 4k > virtually, and the data size of .data aligned to 512 bytes. > > ProtectUefiImageCommon - 0x3C8600C0 > - 0x0000000038777000 - 0x0000000002BC6000 > SetUefiImageMemoryAttributes - 0x0000000038777000 - 0x0000000000004000 > (0x0000000000004008) > SetUefiImageMemoryAttributes - 0x000000003877B000 - 0x0000000000BEE000 > (0x0000000000020008) > SetUefiImageMemoryAttributes - 0x0000000039369000 - 0x0000000001FD4000 > (0x0000000000004008) > Nice to know that. I think .reloc and .compat can be kept small, since protection for compressed kernel image is getting applied manually anyways (patch "efi/x86: Explicitly set sections memory attributes"). But anyways we can align text/data on 4K by rounding setup size (or the headers size if setup gets ripped out): diff --cc arch/x86/boot/tools/build.c index b449c82feaad,b449c82feaad..535646f283e3 --- a/arch/x86/boot/tools/build.c +++ b/arch/x86/boot/tools/build.c @@@ -502,9 -502,9 +505,11 @@@ static unsigned int read_setup(char *pa file_size += reserve_pecoff_compat_section(file_size); file_size += reserve_pecoff_reloc_section(file_size); -- /* Pad unused space with zeros */ -- ++#ifdef CONFIG_EFI_STUB ++ setup_size = round_up(file_size, 0x1000); ++#else setup_size = round_up(file_size, SECTOR_SIZE); ++#endif if (setup_size < SETUP_SECT_MIN * SECTOR_SIZE) setup_size = SETUP_SECT_MIN * SECTOR_SIZE; >> Another desired thing is >> having >> adjacent section with no padding in between them, since [1] does have >> a >> mode that requires sections them to be adjacent. > > Does that have any basis in the PE/COFF spec? No, it is not, I think this mode is rather for the internal firmware images. So this would just be nice to have and nothing strongly required. > >> (SizeOfHeaders/header_size >> should also be set to the size of setup since it is also checked to be >> adjacent to the first section.) >> > > Does that have any basis in the PE/COFF spec? This is neither. > >> I did not do the one-to-one mapping of file and virtual addresses >> since >> it >> would require almost 4K paddings for the auxiliary sections. >> >> [1] https://github.com/acidanthera/audk/tree/secure_pe >> > > I've backpedaled a little bit from this approach (see my other > comment). > > If we just rip out the real mode stub, we can keep the PE header > before the setup header, and simply describe whatever comes as .text. That sounds promising. I think the safest way is to make this a compile time option though, at least as the initial change, so it will not break any obscure boot loaders. But since modern kernel configurations are likely won't even fit into the real mode address space, this option can probably be made mutually exclusive with EFISTUB or CONFIG_EFI_DXE_MEM_ATTRIBUTES. Thanks, Evgeniy Baskov