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


Marvin Häuser <mhaeuser@...>
 

On 10/08/2021 06:40, Andrew Fish via groups.io wrote:


On Aug 9, 2021, at 7:43 PM, Ni, Ray <ray.ni@... <mailto:ray.ni@...>> wrote:

Acked-by: Ray Ni <ray.ni@... <mailto:ray.ni@...>>

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@... <mailto:mhaeuser@...>>
Sent: Monday, August 9, 2021 5:51 PM
To:devel@edk2.groups.io <mailto:devel@edk2.groups.io>
Cc: Dong, Eric <eric.dong@... <mailto:eric.dong@...>>; Ni, Ray <ray.ni@... <mailto:ray.ni@...>>; Kumar, Rahul1 <rahul1.kumar@... <mailto:rahul1.kumar@...>>; Vitaly Cheptsov
<vit9696@... <mailto:vit9696@...>>
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@... <mailto:eric.dong@...>>
Cc: Ray Ni <ray.ni@... <mailto:ray.ni@...>>
Cc: Rahul Kumar <rahul1.kumar@... <mailto:rahul1.kumar@...>>
Cc: Vitaly Cheptsov <vit9696@... <mailto:vit9696@...>>
Signed-off-by: Marvin Häuser <mhaeuser@... <mailto:mhaeuser@...>>
---
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

Join devel@edk2.groups.io to automatically receive all group messages.