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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a3308767-eb30-446b-8c70-32b36a3075e4@canonical.com>
Date: Wed, 9 Oct 2024 09:47:24 +0200
From: Heinrich Schuchardt <heinrich.schuchardt@...onical.com>
To: Alexandre Ghiti <alex@...ti.fr>
Cc: Ard Biesheuvel <ardb@...nel.org>,
 Emil Renner Berthing <emil.renner.berthing@...onical.com>,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
 stable@...r.kernel.org, Paul Walmsley <paul.walmsley@...ive.com>,
 Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>
Subject: Re: [PATCH 1/1] riscv: efi: Set NX compat flag in PE/COFF header

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@...nel.org>
>>>> Cc: <stable@...r.kernel.org>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@...onical.com>
>>>> ---
>>>>   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