| Message ID | 20190123153458.35215-3-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Support HP Envy X2 | expand |
On Wed, Jan 23, 2019 at 04:34:58PM +0100, Alexander Graf wrote: > There is UEFI firmware popping up in the wild now that implements stricter > permission checks using NX and write protect page table entry bits. > > This means that firmware now may fail to load binaries if its individual > sections are not page aligned, as otherwise it can not ensure permission > boundaries. > > So let's bump all efi section alignments up to 4k (EFI page size). That way > we will stay compatible going forward. > > Unfortunately our internals can't deal very well with a mismatch of alignment > between the virtual and file offsets, so we have to also pad our target > binary a bit. > > Signed-off-by: Alexander Graf <agraf@suse.de> > --- > include/grub/efi/pe32.h | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h > index 7d44732d2..52ff208c0 100644 > --- a/include/grub/efi/pe32.h > +++ b/include/grub/efi/pe32.h > @@ -50,8 +50,13 @@ > /* According to the spec, the minimal alignment is 512 bytes... > But some examples (such as EFI drivers in the Intel > Sample Implementation) use 32 bytes (0x20) instead, and it seems > - to be working. For now, GRUB uses 512 bytes for safety. */ > -#define GRUB_PE32_SECTION_ALIGNMENT 0x200 > + to be working. > + > + However, there is firmware showing up in the field now with > + page alignment constraints to guarantee that page protection > + bits take effect. Because we can not easily distinguish between > + in-memory and in-file layout, let's bump all alignment to 4k. */ s/4k/GRUB_EFI_PAGE_SIZE/ > +#define GRUB_PE32_SECTION_ALIGNMENT 0x1000 I think that this should be: #define GRUB_PE32_SECTION_ALIGNMENT GRUB_EFI_PAGE_SIZE And I still miss two patches. One replacing GRUB_PE32_SECTION_ALIGNMENT with GRUB_PE32_FILE_ALIGNMENT in EFI32_HEADER_SIZE and EFI64_HEADER_SIZE macros. And another for some code in the util/mkimage.c... reloc_addr = ALIGN_UP (header_size + core_size, GRUB_PE32_FILE_ALIGNMENT); pe_size = ALIGN_UP (reloc_addr + layout.reloc_size, GRUB_PE32_FILE_ALIGNMENT); ...plus... o->file_alignment = grub_host_to_target32 (GRUB_PE32_FILE_ALIGNMENT); Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On 24.01.19 15:08, Daniel Kiper wrote: > On Wed, Jan 23, 2019 at 04:34:58PM +0100, Alexander Graf wrote: >> There is UEFI firmware popping up in the wild now that implements stricter >> permission checks using NX and write protect page table entry bits. >> >> This means that firmware now may fail to load binaries if its individual >> sections are not page aligned, as otherwise it can not ensure permission >> boundaries. >> >> So let's bump all efi section alignments up to 4k (EFI page size). That way >> we will stay compatible going forward. >> >> Unfortunately our internals can't deal very well with a mismatch of alignment >> between the virtual and file offsets, so we have to also pad our target >> binary a bit. >> >> Signed-off-by: Alexander Graf <agraf@suse.de> >> --- >> include/grub/efi/pe32.h | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h >> index 7d44732d2..52ff208c0 100644 >> --- a/include/grub/efi/pe32.h >> +++ b/include/grub/efi/pe32.h >> @@ -50,8 +50,13 @@ >> /* According to the spec, the minimal alignment is 512 bytes... >> But some examples (such as EFI drivers in the Intel >> Sample Implementation) use 32 bytes (0x20) instead, and it seems >> - to be working. For now, GRUB uses 512 bytes for safety. */ >> -#define GRUB_PE32_SECTION_ALIGNMENT 0x200 >> + to be working. >> + >> + However, there is firmware showing up in the field now with >> + page alignment constraints to guarantee that page protection >> + bits take effect. Because we can not easily distinguish between >> + in-memory and in-file layout, let's bump all alignment to 4k. */ > > s/4k/GRUB_EFI_PAGE_SIZE/ Are you sure you want an #include <efi/memory.h> in this file? I omitted it on purpose ;). > >> +#define GRUB_PE32_SECTION_ALIGNMENT 0x1000 > > I think that this should be: > > #define GRUB_PE32_SECTION_ALIGNMENT GRUB_EFI_PAGE_SIZE > > And I still miss two patches. One replacing GRUB_PE32_SECTION_ALIGNMENT > with GRUB_PE32_FILE_ALIGNMENT in EFI32_HEADER_SIZE and EFI64_HEADER_SIZE > macros. > > And another for some code in the util/mkimage.c... > > reloc_addr = ALIGN_UP (header_size + core_size, > GRUB_PE32_FILE_ALIGNMENT); > > pe_size = ALIGN_UP (reloc_addr + layout.reloc_size, > GRUB_PE32_FILE_ALIGNMENT); > > ...plus... > > o->file_alignment = grub_host_to_target32 (GRUB_PE32_FILE_ALIGNMENT); As mentioned above, these *have* to be identical today, because otherwise all other assumptions in the code just fall apart. I see little point in playing that they may be unrelated. Alex _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Thu, Jan 24, 2019 at 03:10:58PM +0100, Alexander Graf wrote: > On 24.01.19 15:08, Daniel Kiper wrote: > > On Wed, Jan 23, 2019 at 04:34:58PM +0100, Alexander Graf wrote: > >> There is UEFI firmware popping up in the wild now that implements stricter > >> permission checks using NX and write protect page table entry bits. > >> > >> This means that firmware now may fail to load binaries if its individual > >> sections are not page aligned, as otherwise it can not ensure permission > >> boundaries. > >> > >> So let's bump all efi section alignments up to 4k (EFI page size). That way > >> we will stay compatible going forward. > >> > >> Unfortunately our internals can't deal very well with a mismatch of alignment > >> between the virtual and file offsets, so we have to also pad our target > >> binary a bit. > >> > >> Signed-off-by: Alexander Graf <agraf@suse.de> > >> --- > >> include/grub/efi/pe32.h | 9 +++++++-- > >> 1 file changed, 7 insertions(+), 2 deletions(-) > >> > >> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h > >> index 7d44732d2..52ff208c0 100644 > >> --- a/include/grub/efi/pe32.h > >> +++ b/include/grub/efi/pe32.h > >> @@ -50,8 +50,13 @@ > >> /* According to the spec, the minimal alignment is 512 bytes... > >> But some examples (such as EFI drivers in the Intel > >> Sample Implementation) use 32 bytes (0x20) instead, and it seems > >> - to be working. For now, GRUB uses 512 bytes for safety. */ > >> -#define GRUB_PE32_SECTION_ALIGNMENT 0x200 > >> + to be working. > >> + > >> + However, there is firmware showing up in the field now with > >> + page alignment constraints to guarantee that page protection > >> + bits take effect. Because we can not easily distinguish between > >> + in-memory and in-file layout, let's bump all alignment to 4k. */ > > > > s/4k/GRUB_EFI_PAGE_SIZE/ > > Are you sure you want an > > #include <efi/memory.h> > > in this file? I omitted it on purpose ;). OK, probably this is not perfect but if nothing falls a part I would include it in this file. If something breaks I would explain in the comment why we hardcode this value here. > >> +#define GRUB_PE32_SECTION_ALIGNMENT 0x1000 > > > > I think that this should be: > > > > #define GRUB_PE32_SECTION_ALIGNMENT GRUB_EFI_PAGE_SIZE > > > > And I still miss two patches. One replacing GRUB_PE32_SECTION_ALIGNMENT > > with GRUB_PE32_FILE_ALIGNMENT in EFI32_HEADER_SIZE and EFI64_HEADER_SIZE > > macros. > > > > And another for some code in the util/mkimage.c... > > > > reloc_addr = ALIGN_UP (header_size + core_size, > > GRUB_PE32_FILE_ALIGNMENT); > > > > pe_size = ALIGN_UP (reloc_addr + layout.reloc_size, > > GRUB_PE32_FILE_ALIGNMENT); > > > > ...plus... > > > > o->file_alignment = grub_host_to_target32 (GRUB_PE32_FILE_ALIGNMENT); > > As mentioned above, these *have* to be identical today, because > otherwise all other assumptions in the code just fall apart. I see > little point in playing that they may be unrelated. GRUB_PE32_FILE_ALIGNMENT is equal to GRUB_PE32_SECTION_ALIGNMENT (well, I think that it also makes sense to explain before GRUB_PE32_FILE_ALIGNMENT definition why we should keep it equal to GRUB_PE32_SECTION_ALIGNMENT) hence nothing breaks. However, IMO after these changes the code looks more logical and is less confusing. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h index 7d44732d2..52ff208c0 100644 --- a/include/grub/efi/pe32.h +++ b/include/grub/efi/pe32.h @@ -50,8 +50,13 @@ /* According to the spec, the minimal alignment is 512 bytes... But some examples (such as EFI drivers in the Intel Sample Implementation) use 32 bytes (0x20) instead, and it seems - to be working. For now, GRUB uses 512 bytes for safety. */ -#define GRUB_PE32_SECTION_ALIGNMENT 0x200 + to be working. + + However, there is firmware showing up in the field now with + page alignment constraints to guarantee that page protection + bits take effect. Because we can not easily distinguish between + in-memory and in-file layout, let's bump all alignment to 4k. */ +#define GRUB_PE32_SECTION_ALIGNMENT 0x1000 #define GRUB_PE32_FILE_ALIGNMENT GRUB_PE32_SECTION_ALIGNMENT struct grub_pe32_coff_header
There is UEFI firmware popping up in the wild now that implements stricter permission checks using NX and write protect page table entry bits. This means that firmware now may fail to load binaries if its individual sections are not page aligned, as otherwise it can not ensure permission boundaries. So let's bump all efi section alignments up to 4k (EFI page size). That way we will stay compatible going forward. Unfortunately our internals can't deal very well with a mismatch of alignment between the virtual and file offsets, so we have to also pad our target binary a bit. Signed-off-by: Alexander Graf <agraf@suse.de> --- include/grub/efi/pe32.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) -- 2.12.3 _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel