Re: [PATCH v4 2/2] mkimage: Align efi sections on 4k boundary

[Alexander Graf]

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 <address@hidden>
>> ---
>>  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