Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/BaseUefiCpuLib: Use toolchain-specific rodata section name

["Marvin Häuser" <mhaeuser@posteo.de>]

On 10/08/2021 06:40, Andrew Fish via groups.io wrote:
>
>
>> On Aug 9, 2021, at 7:43 PM, Ni, Ray <ray.ni@intel.com 
>> <mailto:ray.ni@intel.com>> wrote:
>>
>> Acked-by: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>>
>> I will depend on tool owner to review the tool configuration change 
>> making sure that the correct section name is chosen for different C 
>> compilers.
>>
>
> Ray,
>
> I made a detailed response about Mach-O with Xcode/clang and I don’t 
> think patch works. Not sure if it breaks anything, but it puts things 
> in the .data PE/COFF section.

The latter part is true only for Xcode-based toolchains, as far as I am 
aware now, and this is in fact the way it works right now.

>
> I’m also worried it is broken for any toolchain that generates ELF and 
> use GenFw. I don’t think the GenFw tool creates a PE/COFF .rodata 
> section [1] so if things work they will end up in the .data section, 
> or things might break? Some one who knows that tool better than me 
> should take a detailed look.

You are correct, but the NASM section name semantically translates to an 
*ELF* .rodata section for usage during linking. The GNU linker scripts 
merge it into .text later [1], yielding no PE/COFF .rodata section as 
observed.

>
> I’m guessing it likely does the correct thing for toolchains that 
> generate PE/COFF directly?

Behaviour should be fixed for PE-based toolchains, preserved correct for 
ELF-based toolchains (but inconsistent with PE-based, as we merge 
.rodata into .text here), and as correct or broken for Xcode-based 
toolchains as it was before.

>
> My vote is to not add this feature until we can prove it works 
> properly on all the toolchains. For Xcode it may be easier to just 
> dump this stuff in the .text section (see my other mail for more 
> background).

If you can help with designing a solution, I'm more than happy to to 
submit a V2. Just I don't know Xcode, and I don't run macOS. :)

> It looks like we might have to modify GenFw if we want to create a 
> .rodata section?

For now, this patch un-breaks PE-based toolchains. They are broken in 
the way described in the BZ, but not in a security-critical fashion. 
Rather we have the worst of both worlds, the additional size of another 
aligned section, and the downgraded permission as we would observe with 
a merge into e.g. .text. I have no strong opinion on which route to 
pick, i.e. dedicated .r(o)data or merging into .text, but I would like 
it to be consistent in the end, especially across toolchains. Ideally, 
in my opinion, the platform maintainer can choose whether they want the 
extra protection (NX .rodata), or the extra space (.text merge).

>
> It might be possible to cheat and use this concept to force code into 
> the text section for ELF and Mach-O, but I’m not sure if that hits the 
> correct security bar. But the last thing we want is to claim something 
> is in a read only section when it is in a read write section.

I can check again later, but ELF should be fine, really.

Best regards,
Marvin


[1] 
https://github.com/tianocore/edk2/blob/d02dbb53cd78de799e6afaa237e98771fb5148db/BaseTools/Scripts/GccBase.lds#L25

>
> [1] git grep CreateSectionHeader
> BaseTools/Source/C/GenFw/Elf32Convert.c:602:*CreateSectionHeader*(".text", 
> mTextOffset, mDataOffset - mTextOffset,
> BaseTools/Source/C/GenFw/Elf32Convert.c:612:*CreateSectionHeader*(".data", 
> mDataOffset, mHiiRsrcOffset - mDataOffset,
> BaseTools/Source/C/GenFw/Elf32Convert.c:622:*CreateSectionHeader*(".rsrc", 
> mHiiRsrcOffset, mRelocOffset - mHiiRsrcOffset,
> BaseTools/Source/C/GenFw/Elf32Convert.c:1107:*CreateSectionHeader*(".reloc", 
> mRelocOffset, mCoffOffset - mRelocOffset,
> BaseTools/Source/C/GenFw/Elf64Convert.c:929:*CreateSectionHeader*(".text", 
> mTextOffset, mDataOffset - mTextOffset,
> BaseTools/Source/C/GenFw/Elf64Convert.c:939:*CreateSectionHeader*(".data", 
> mDataOffset, mHiiRsrcOffset - mDataOffset,
> BaseTools/Source/C/GenFw/Elf64Convert.c:949:*CreateSectionHeader*(".rsrc", 
> mHiiRsrcOffset, mRelocOffset - mHiiRsrcOffset,
> BaseTools/Source/C/GenFw/Elf64Convert.c:1641:*CreateSectionHeader*(".reloc", 
> mRelocOffset, mCoffOffset - mRelocOffset,
> BaseTools/Source/C/GenFw/ElfConvert.c:125:*CreateSectionHeader*(
> BaseTools/Source/C/GenFw/ElfConvert.h:74:*CreateSectionHeader*(
>
> Thanks,
>
> Andrew Fish
>
>> Thanks,
>> Ray
>>
>>> -----Original Message-----
>>> From: Marvin Häuser <mhaeuser@posteo.de <mailto:mhaeuser@posteo.de>>
>>> Sent: Monday, August 9, 2021 5:51 PM
>>> To:devel@edk2.groups.io <mailto:devel@edk2.groups.io>
>>> Cc: Dong, Eric <eric.dong@intel.com <mailto:eric.dong@intel.com>>; 
>>> Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Kumar, Rahul1 
>>> <rahul1.kumar@intel.com <mailto:rahul1.kumar@intel.com>>; Vitaly 
>>> Cheptsov
>>> <vit9696@protonmail.com <mailto:vit9696@protonmail.com>>
>>> Subject: [PATCH v2 2/2] UefiCpuPkg/BaseUefiCpuLib: Use 
>>> toolchain-specific rodata section name
>>>
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3318 
>>> <https://bugzilla.tianocore.org/show_bug.cgi?id=3318>
>>>
>>> Correctly define the read-only data sections with the
>>> toolchain-specific section name. This hardens image permission
>>> security and may save image space.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com <mailto:eric.dong@intel.com>>
>>> Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>>> Cc: Rahul Kumar <rahul1.kumar@intel.com <mailto:rahul1.kumar@intel.com>>
>>> Cc: Vitaly Cheptsov <vit9696@protonmail.com 
>>> <mailto:vit9696@protonmail.com>>
>>> Signed-off-by: Marvin Häuser <mhaeuser@posteo.de 
>>> <mailto:mhaeuser@posteo.de>>
>>> ---
>>> UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm | 2 +-
>>> UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm  | 2 +-
>>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm
>>> b/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm
>>> index 5e27cc325012..cfb8bf4a5ae0 100644
>>> --- a/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm
>>> +++ b/UefiCpuPkg/Library/BaseUefiCpuLib/Ia32/InitializeFpu.nasm
>>> @@ -6,7 +6,7 @@
>>> ;*
>>>
>>> ;------------------------------------------------------------------------------
>>>
>>>
>>>
>>> -    SECTION .rodata
>>>
>>> +    SECTION RODATA_SECTION_NAME
>>>
>>>
>>>
>>> ;
>>>
>>> ; Float control word initial value:
>>>
>>> diff --git a/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm
>>> b/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm
>>> index 8485b4713548..3c976a21e391 100644
>>> --- a/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm
>>> +++ b/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm
>>> @@ -6,7 +6,7 @@
>>> ;*
>>>
>>> ;------------------------------------------------------------------------------
>>>
>>>
>>>
>>> -    SECTION .rodata
>>>
>>> +    SECTION RODATA_SECTION_NAME
>>>
>>> ;
>>>
>>> ; Float control word initial value:
>>>
>>> ; all exceptions masked, double-extended-precision, round-to-nearest
>>>
>>> --
>>> 2.31.1
>>
>>
>>
>
>