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

[Evgeniy Baskov]

On 2023-03-11 20:39, Ard Biesheuvel wrote:
> On Sat, 11 Mar 2023 at 16:09, Evgeniy Baskov <baskov@xxxxxxxxx> wrote:
> > On 2023-03-10 18:20, Ard Biesheuvel wrote:
> > > On Thu, 15 Dec 2022 at 13:42, Evgeniy Baskov <baskov@xxxxxxxxx> 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@xxxxxxxxxx>
> > >> Signed-off-by: Evgeniy Baskov <baskov@xxxxxxxxx>
> > >
> > > 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.