Re: [PATCH 1/1] riscv: efi: Set NX compat flag in PE/COFF header

[Heinrich Schuchardt]

On 09.10.24 09:34, Alexandre Ghiti wrote:
> Hi Heinrich,
>
> On 01/10/2024 17:24, Heinrich Schuchardt wrote:
> > On 01.10.24 15:51, Alexandre Ghiti wrote:
> > > Hi Heinrich,
> > >
> > > On 29/09/2024 16:02, Heinrich Schuchardt wrote:
> > > > The IMAGE_DLLCHARACTERISTICS_NX_COMPAT informs the firmware that the
> > > > EFI binary does not rely on pages that are both executable and
> > > > writable.
> > > >
> > > > The flag is used by some distro versions of GRUB to decide if the EFI
> > > > binary may be executed.
> > > >
> > > > As the Linux kernel neither has RWX sections nor needs RWX pages for
> > > > relocation we should set the flag.
> > > >
> > > > Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
> > > > Cc: <stable@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@xxxxxxxxxxxxx>
> > > > ---
> > > >   arch/riscv/kernel/efi-header.S | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi- 
> > > > header.S
> > > > index 515b2dfbca75..c5f17c2710b5 100644
> > > > --- a/arch/riscv/kernel/efi-header.S
> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > @@ -64,7 +64,7 @@ extra_header_fields:
> > > >       .long    efi_header_end - _start            // SizeOfHeaders
> > > >       .long    0                    // CheckSum
> > > >       .short    IMAGE_SUBSYSTEM_EFI_APPLICATION        // Subsystem
> > > > -    .short    0                    // DllCharacteristics
> > > > +    .short    IMAGE_DLL_CHARACTERISTICS_NX_COMPAT    // 
> > > > DllCharacteristics
> > > >       .quad    0                    // SizeOfStackReserve
> > > >       .quad    0                    // SizeOfStackCommit
> > > >       .quad    0                    // SizeOfHeapReserve
> > >
> > >
> > > I don't understand if this fixes something or not: what could go 
> > > wrong if we don't do this?
> > >
> > > Thanks,
> > >
> > > Alex
> >
> >
> > Hello Alexandre,
> >
> > https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/ 
> > uefi-ca-memory-mitigation-requirements
> > describes Microsoft's effort to improve security by avoiding memory 
> > pages that are both executable and writable.
> >
> > IMAGE_DLL_CHARACTERISTICS_NX_COMPAT is an assertion by the EFI binary 
> > that it does not use RWX pages. It may use the 
> > EFI_MEMORY_ATTRIBUTE_PROTOCOL to set whether a page is writable or 
> > executable (but not both).
> >
> > When using secure boot, compliant firmware will not allow loading a 
> > binary if the flag is not set.
>
>
> Great, so that's a necessary fix, it will get merged in the next rc or so:
>
> Fixes: cb7d2dd5612a ("RISC-V: Add PE/COFF header for EFI stub")

Thanks for reviewing.

At the time of commit cb7d2dd5612a (2020-10-02) the requirement did not 
exist. I guess a Fixes: tag is not applicable under these circumstances.

Best regards

Heinrich