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

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 5180 bytes --]



> On Aug 9, 2021, at 7:43 PM, Ni, Ray <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. 

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. 

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

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). It looks like we might have to modify GenFw if we want to create a .rodata section? 

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. 

[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
>> 
>> 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>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Vitaly Cheptsov <vit9696@protonmail.com>
>> Signed-off-by: Marvin Häuser <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
> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 20742 bytes --]