[v4,21/26] efi/x86: Explicitly set sections memory attributes

  Explicitly change sections memory attributes in efi_pe_entry in case
of incorrect EFI implementations and to reduce access rights to
compressed kernel blob. By default it is set executable due to
restriction in maximum number of sections that can fit before zero
page.

Tested-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
---
 drivers/firmware/efi/libstub/x86-stub.c | 54 +++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
  

On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@ispras.ru> wrote:
>
> Explicitly change sections memory attributes in efi_pe_entry in case
> of incorrect EFI implementations and to reduce access rights to
> compressed kernel blob. By default it is set executable due to
> restriction in maximum number of sections that can fit before zero
> page.
>
> Tested-by: Peter Jones <pjones@redhat.com>
> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>

I don't think we need this patch. Firmware that cares about W^X will
map the PE image with R-X for text/rodata and RW- for data/bss, which
is sufficient, and firmware that doesn't is a lost cause anyway.


> ---
>  drivers/firmware/efi/libstub/x86-stub.c | 54 +++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> index 1f0a2e7075c3..60697fcd8950 100644
> --- a/drivers/firmware/efi/libstub/x86-stub.c
> +++ b/drivers/firmware/efi/libstub/x86-stub.c
> @@ -27,6 +27,12 @@ const efi_dxe_services_table_t *efi_dxe_table;
>  u32 image_offset __section(".data");
>  static efi_loaded_image_t *image __section(".data");
>
> +extern char _head[], _ehead[];
> +extern char _compressed[], _ecompressed[];
> +extern char _text[], _etext[];
> +extern char _rodata[], _erodata[];
> +extern char _data[];
> +
>  static efi_status_t
>  preserve_pci_rom_image(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom)
>  {
> @@ -343,6 +349,52 @@ void __noreturn efi_exit(efi_handle_t handle, efi_status_t status)
>                 asm("hlt");
>  }
>
> +
> +/*
> + * Manually setup memory protection attributes for each ELF section
> + * since we cannot do it properly by using PE sections.
> + */
> +static void setup_sections_memory_protection(unsigned long image_base)
> +{
> +#ifdef CONFIG_EFI_DXE_MEM_ATTRIBUTES
> +       efi_dxe_table = get_efi_config_table(EFI_DXE_SERVICES_TABLE_GUID);
> +
> +       if (!efi_dxe_table ||
> +           efi_dxe_table->hdr.signature != EFI_DXE_SERVICES_TABLE_SIGNATURE) {
> +               efi_warn("Unable to locate EFI DXE services table\n");
> +               efi_dxe_table = NULL;
> +               return;
> +       }
> +
> +       /* .setup [image_base, _head] */
> +       efi_adjust_memory_range_protection(image_base,
> +                                          (unsigned long)_head - image_base,
> +                                          EFI_MEMORY_RO | EFI_MEMORY_XP);
> +       /* .head.text [_head, _ehead] */
> +       efi_adjust_memory_range_protection((unsigned long)_head,
> +                                          (unsigned long)_ehead - (unsigned long)_head,
> +                                          EFI_MEMORY_RO);
> +       /* .rodata..compressed [_compressed, _ecompressed] */
> +       efi_adjust_memory_range_protection((unsigned long)_compressed,
> +                                          (unsigned long)_ecompressed - (unsigned long)_compressed,
> +                                          EFI_MEMORY_RO | EFI_MEMORY_XP);
> +       /* .text [_text, _etext] */
> +       efi_adjust_memory_range_protection((unsigned long)_text,
> +                                          (unsigned long)_etext - (unsigned long)_text,
> +                                          EFI_MEMORY_RO);
> +       /* .rodata [_rodata, _erodata] */
> +       efi_adjust_memory_range_protection((unsigned long)_rodata,
> +                                          (unsigned long)_erodata - (unsigned long)_rodata,
> +                                          EFI_MEMORY_RO | EFI_MEMORY_XP);
> +       /* .data, .bss [_data, _end] */
> +       efi_adjust_memory_range_protection((unsigned long)_data,
> +                                          (unsigned long)_end - (unsigned long)_data,
> +                                          EFI_MEMORY_XP);
> +#else
> +       (void)image_base;
> +#endif
> +}
> +
>  void __noreturn efi_stub_entry(efi_handle_t handle,
>                                efi_system_table_t *sys_table_arg,
>                                struct boot_params *boot_params);
> @@ -687,6 +739,8 @@ asmlinkage unsigned long efi_main(efi_handle_t handle,
>                 efi_dxe_table = NULL;
>         }
>
> +       setup_sections_memory_protection(bzimage_addr - image_offset);
> +
>  #ifdef CONFIG_CMDLINE_BOOL
>         status = efi_parse_options(CONFIG_CMDLINE);
>         if (status != EFI_SUCCESS) {
> --
> 2.37.4
>
  

On 2023-03-10 18:20, Ard Biesheuvel wrote:
> On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> 
>> Explicitly change sections memory attributes in efi_pe_entry in case
>> of incorrect EFI implementations and to reduce access rights to
>> compressed kernel blob. By default it is set executable due to
>> restriction in maximum number of sections that can fit before zero
>> page.
>> 
>> Tested-by: Peter Jones <pjones@redhat.com>
>> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
> 
> I don't think we need this patch. Firmware that cares about W^X will
> map the PE image with R-X for text/rodata and RW- for data/bss, which
> is sufficient, and firmware that doesn't is a lost cause anyway.

This patch were here mainly here to make .rodata non-executable and for
the UEFI handover protocol, for which attributes are usually not getting
applied.

Since the UEFI handover protocol is deprecated, I'll exclude patches 
from
v5 and maybe submit it separately modified to apply attributes only when
booting via this protocol.

> 
> 
>> ---
>>  drivers/firmware/efi/libstub/x86-stub.c | 54 
>> +++++++++++++++++++++++++
>>  1 file changed, 54 insertions(+)
>> 
>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c 
>> b/drivers/firmware/efi/libstub/x86-stub.c
>> index 1f0a2e7075c3..60697fcd8950 100644
>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>> @@ -27,6 +27,12 @@ const efi_dxe_services_table_t *efi_dxe_table;
>>  u32 image_offset __section(".data");
>>  static efi_loaded_image_t *image __section(".data");
>> 
>> +extern char _head[], _ehead[];
>> +extern char _compressed[], _ecompressed[];
>> +extern char _text[], _etext[];
>> +extern char _rodata[], _erodata[];
>> +extern char _data[];
>> +
>>  static efi_status_t
>>  preserve_pci_rom_image(efi_pci_io_protocol_t *pci, struct 
>> pci_setup_rom **__rom)
>>  {
>> @@ -343,6 +349,52 @@ void __noreturn efi_exit(efi_handle_t handle, 
>> efi_status_t status)
>>                 asm("hlt");
>>  }
>> 
>> +
>> +/*
>> + * Manually setup memory protection attributes for each ELF section
>> + * since we cannot do it properly by using PE sections.
>> + */
>> +static void setup_sections_memory_protection(unsigned long 
>> image_base)
>> +{
>> +#ifdef CONFIG_EFI_DXE_MEM_ATTRIBUTES
>> +       efi_dxe_table = 
>> get_efi_config_table(EFI_DXE_SERVICES_TABLE_GUID);
>> +
>> +       if (!efi_dxe_table ||
>> +           efi_dxe_table->hdr.signature != 
>> EFI_DXE_SERVICES_TABLE_SIGNATURE) {
>> +               efi_warn("Unable to locate EFI DXE services table\n");
>> +               efi_dxe_table = NULL;
>> +               return;
>> +       }
>> +
>> +       /* .setup [image_base, _head] */
>> +       efi_adjust_memory_range_protection(image_base,
>> +                                          (unsigned long)_head - 
>> image_base,
>> +                                          EFI_MEMORY_RO | 
>> EFI_MEMORY_XP);
>> +       /* .head.text [_head, _ehead] */
>> +       efi_adjust_memory_range_protection((unsigned long)_head,
>> +                                          (unsigned long)_ehead - 
>> (unsigned long)_head,
>> +                                          EFI_MEMORY_RO);
>> +       /* .rodata..compressed [_compressed, _ecompressed] */
>> +       efi_adjust_memory_range_protection((unsigned long)_compressed,
>> +                                          (unsigned long)_ecompressed 
>> - (unsigned long)_compressed,
>> +                                          EFI_MEMORY_RO | 
>> EFI_MEMORY_XP);
>> +       /* .text [_text, _etext] */
>> +       efi_adjust_memory_range_protection((unsigned long)_text,
>> +                                          (unsigned long)_etext - 
>> (unsigned long)_text,
>> +                                          EFI_MEMORY_RO);
>> +       /* .rodata [_rodata, _erodata] */
>> +       efi_adjust_memory_range_protection((unsigned long)_rodata,
>> +                                          (unsigned long)_erodata - 
>> (unsigned long)_rodata,
>> +                                          EFI_MEMORY_RO | 
>> EFI_MEMORY_XP);
>> +       /* .data, .bss [_data, _end] */
>> +       efi_adjust_memory_range_protection((unsigned long)_data,
>> +                                          (unsigned long)_end - 
>> (unsigned long)_data,
>> +                                          EFI_MEMORY_XP);
>> +#else
>> +       (void)image_base;
>> +#endif
>> +}
>> +
>>  void __noreturn efi_stub_entry(efi_handle_t handle,
>>                                efi_system_table_t *sys_table_arg,
>>                                struct boot_params *boot_params);
>> @@ -687,6 +739,8 @@ asmlinkage unsigned long efi_main(efi_handle_t 
>> handle,
>>                 efi_dxe_table = NULL;
>>         }
>> 
>> +       setup_sections_memory_protection(bzimage_addr - image_offset);
>> +
>>  #ifdef CONFIG_CMDLINE_BOOL
>>         status = efi_parse_options(CONFIG_CMDLINE);
>>         if (status != EFI_SUCCESS) {
>> --
>> 2.37.4
>>
  

On Sat, 11 Mar 2023 at 16:09, Evgeniy Baskov <baskov@ispras.ru> wrote:
>
> On 2023-03-10 18:20, Ard Biesheuvel wrote:
> > On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@ispras.ru> wrote:
> >>
> >> Explicitly change sections memory attributes in efi_pe_entry in case
> >> of incorrect EFI implementations and to reduce access rights to
> >> compressed kernel blob. By default it is set executable due to
> >> restriction in maximum number of sections that can fit before zero
> >> page.
> >>
> >> Tested-by: Peter Jones <pjones@redhat.com>
> >> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
> >
> > I don't think we need this patch. Firmware that cares about W^X will
> > map the PE image with R-X for text/rodata and RW- for data/bss, which
> > is sufficient, and firmware that doesn't is a lost cause anyway.
>
> This patch were here mainly here to make .rodata non-executable and for
> the UEFI handover protocol, for which attributes are usually not getting
> applied.
>
> Since the UEFI handover protocol is deprecated, I'll exclude patches
> from
> v5 and maybe submit it separately modified to apply attributes only when
> booting via this protocol.
>

I think the issue here is that loaders that use the UEFI handover
protocol use their own implementations of LoadImage/StartImage as
well, and some of those tend to do little more than copy the image
into memory and jump to the EFI handover protocol entry point, without
even accounting for the image size in memory or clearing the bss.

To be honest, even though I understand the reason these had to be
implemented, I'm a bit reluctant to cater for the needs of such
loaders, given that these are all downstream distro forks of GRUB
(with shim) with varying levels of adherence to the PE/COFF spec.

I'm happy to revisit this later if others feel this is important, but
for the moment, I'd prefer it if we could focus on making the x86
image work better with compliant loaders, which is what this series is
primarily about.
  

On 2023-03-11 20:39, Ard Biesheuvel wrote:
> On Sat, 11 Mar 2023 at 16:09, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> 
>> On 2023-03-10 18:20, Ard Biesheuvel wrote:
>> > On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@ispras.ru> wrote:
>> >>
>> >> Explicitly change sections memory attributes in efi_pe_entry in case
>> >> of incorrect EFI implementations and to reduce access rights to
>> >> compressed kernel blob. By default it is set executable due to
>> >> restriction in maximum number of sections that can fit before zero
>> >> page.
>> >>
>> >> Tested-by: Peter Jones <pjones@redhat.com>
>> >> Signed-off-by: Evgeniy Baskov <baskov@ispras.ru>
>> >
>> > I don't think we need this patch. Firmware that cares about W^X will
>> > map the PE image with R-X for text/rodata and RW- for data/bss, which
>> > is sufficient, and firmware that doesn't is a lost cause anyway.
>> 
>> This patch were here mainly here to make .rodata non-executable and 
>> for
>> the UEFI handover protocol, for which attributes are usually not 
>> getting
>> applied.
>> 
>> Since the UEFI handover protocol is deprecated, I'll exclude patches
>> from
>> v5 and maybe submit it separately modified to apply attributes only 
>> when
>> booting via this protocol.
>> 
> 
> I think the issue here is that loaders that use the UEFI handover
> protocol use their own implementations of LoadImage/StartImage as
> well, and some of those tend to do little more than copy the image
> into memory and jump to the EFI handover protocol entry point, without
> even accounting for the image size in memory or clearing the bss.
> 

AFAIK this patch does not break loaders that load PE image as a flat
binary, since it only operates on ELF sections that are mapped 1-to-1.
But that's just the note for a future.

> To be honest, even though I understand the reason these had to be
> implemented, I'm a bit reluctant to cater for the needs of such
> loaders, given that these are all downstream distro forks of GRUB
> (with shim) with varying levels of adherence to the PE/COFF spec.
> 
> I'm happy to revisit this later if others feel this is important, but
> for the moment, I'd prefer it if we could focus on making the x86
> image work better with compliant loaders, which is what this series is
> primarily about.

That's very reasonable. I'll put this patch aside for now then.