[edk2-devel] [PATCH v2 1/2] BaseTools: Define the read-only data section name per toolchain
Marvin Häuser
mhaeuser at posteo.de
Thu Aug 12 20:25:12 UTC 2021
On 12/08/2021 09:26, Marvin Häuser wrote:
>>> The value of p is constant, so it can be placed in a constant data
>>> section. p points to a global variable, so if the compiler does not
>>> manage to somehow turn this into relative addressing (let's assume
>>> it does not), it needs to generate a relocation. This means the
>>> compiler cannot put it in __TEXT,__const, so it has to put it in
>>> __DATA,__const (of course it could put it in other __DATA sections,
>>> but let's assume the compiler agrees this should be read-only). The
>>> very same issue will arise and no matter the choice of the compiler,
>>> this will end up in .data. Do you agree? Or do we have some
>>> guarantee that Apple Clang cannot emit __DATA,__const?
>>
>> I don’t see your bigger point. The compiler is free to implement as
>> it sees fit. Which section some code ends up in is more of an
>> implementation detail for the compiler, and we can’t really depend on
>> that?
>
> Your point, rightfully, was that things that we request to be
> read-only (may) end up being read-write. My issue is that, if the
> compiler requests this pointer to be read-only (it may not, but also
> it may), our PE executable does not honour it either. __DATA,__const
> is a section for constant data, and we put it into a read-write
> section. The bigger point is, whenever the compile stack wants
> something read-only (be that NASM or be that Apple Clang, anything
> really), we should actually ensure it is read-only. I can do that for
> only NASM by forcing the __TEXT,__const section name (at the cost of
> prohibiting relocs), but I do not know how to do it for Apple Clang.
> At worst we could take a hacked-ish solution, where all Mach-O
> segments are converted to PE/COFF sections - with the exception of the
> __DATA,__const section, which, if aligned on a segment alignment
> boundary, can be inserted between the two other parts .data is split
> into. I read in the XNU source that the ARM protection code does
> something roughly like this [1], but I'm really far from well-versed
> in the deep details of macOS.
>
> Sorry for this not being "integrated" in above text, but I found two
> more things while looking for citation 1.
>
> 1) Mach-O sections can be renamed, including the preceding segment
> name [2]. According to the very next line, the example actually
> creates a new segment. Does it allow merging into another, existing
> segment? What if I did something like:
>
> -Wl,-rename_section,__DATA,__const,__TEXT,__const2
>
> or even
>
> -Wl,-rename_section,__DATA,__const,__TEXT,__const
(I cut the upper part of the quote to make some room...)
Both worked as expected.
>
> i.e. can it merge two sections together? if __DATA,__const had data
> with relocs, would the renaming trigger the "no relocs" error of
> __TEXT, or does that happen before section renaming? Any chance it can
> be turned off?
As feared, the error triggers. However I found a seemingly
not-so-well-known flag for Apple ld64 to disable it: "-read_only_relocs
suppress". In fact, it is already used for IA32 builds, but not for X64
builds? [1]
I also noticed the extent of the XCODE5 "exceptions", e.g. duplicated
files [2]. The files seem to be out-of-sync, but interestingly the
XCODE5 version is the one that is more current and used in other
toolchain builds as well.
So, maybe we can do the following?
1) Pass "-read_only_relocs suppress" for X64 and clean up any
XCODE5-specific workarounds for text relocations.
2) Use neither "__TEXT,__const" nor "__DATA,__const" for
NASM_RODATA_SECTION_NAME, but "__DATA_CONST,__const". Give it RNX
permissions.
3) Case-distinct between "generate standalone .rodata" and "merge
.rodata into .text".
3.1) case 1: Rename "__TEXT,__const" and "__TEXT,__cstring" to
"__DATA_CONST,__text_const" and "__DATA_CONST,__cstring" respectively.
3.2) case 2: Rename "__DATA_CONST,__const" to "__TEXT,__data_const".
Rationales:
1) There is no such concept in PE/COFF, we can discard it and align with
PE/COFF and ELF toolchains. Allows ".rodata" merge into ".text".
2) There is no pre-existing segment for read-only data, so we create our
own with a naming convention taken from XNU.
3.1) Both former sections do not contain executable code, so we properly
enforce the separation introduced in 2). Choose unique names to not
reduce the object file separation.
3.2) Merge ".rodata" into ".text" in the same way ELF toolchains do.
Saves space and we get to keep read-only.
I can test both cases work fine soon.
>
> 1.1) Actually, for the standalone .rodata section, we can just rename
> it to __DATA_CONST,__const, as I have seen elsewhere in XNU. No
> hacked-ish solution needed. :)
>
> 2) We actually can force the compiler to put data in the constant data
> segment [3]. This is of course not used in EDK II, and probably
> neither portable nor necessary, but an interesting detail. I really
> think we should honour it either way.
>
> I will likely try to get my hands on some sort of Apple development
> environment soon, but I cannot promise much right now. I think it
> really is better if I can test through all toolchains myself. If you
> release Apple Clang for Linux, I also won't complain of course. :)
I found a port of Apple ld64 for Linux. I get it's not "the real deal",
but all observed behaviour matches what I've seen so far from Apple
Xcode. Actually boots OVMF. :)
A side note, seems like even latest mtoc gives .data RWX no matter what?
Ouch...
Best regards,
Marvin
[1]
https://github.com/tianocore/edk2/blob/master/BaseTools/Conf/tools_def.template#L2980-L2982
[2]
https://github.com/tianocore/edk2/blob/0a6b303dcedb7af238ad485d545802befb797b3a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
https://github.com/tianocore/edk2/blob/0a6b303dcedb7af238ad485d545802befb797b3a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>
>>
>>>
>>>>>> We should double check what is happening for ELF on x86, ARM,
>>>>>> RiskV, etc. and do the same thing. I assume all the tools that
>>>>>> generate PE/COFF directly are good with .rodata?
>>>>> They are not, that is the whole point of the patch in its current
>>>>> shape. .rodata is valid for ELF and Mach-O, PE/COFF needs .rdata.
>>>>>
>>>>>> I think it is likely as simple as dumping the EFL object file in
>>>>>> objdump or gdb for the given toolchain (like my Xcode example).
>>>>>>
>>>>>> TL;DR It looks to me like nasm does some SECTION translations
>>>>>> under the hood to make code portable, and I’d like to make sure
>>>>>> we capture those in the new NASM_RODATA_SECTION_NAME. If some one
>>>>>> is doing a security review having NASM_RODATA_SECTION_NAME is
>>>>>> going to imply that a .rodata section is being used by that
>>>>>> specific toolchain, and I think that is much worse than the
>>>>>> current “magic” behavior in nasm. We are much better off
>>>>>> explaining what is really happening, since it is not very obvious.
>>>>> I feel like I'm too tired to get the point. Do you mean you want
>>>>> comments whenever this section name is used? Or comments in
>>>>> tools_def?
>>>>>
>>>> I think I’d settle for a more descriptive commit comment that
>>>> better defines what the define means like I mentioned in the other
>>>> mail.
>>>
>>> Hmm no, we can do that too, but in that case I really want comments
>>> in the code. tools_def is not really documented at all, maybe it is
>>> time to introduce an example comment so at least new things get
>>> commented? Maybe just the start of a macro list. Relying on "git
>>> blame" to figure out simple things is rather awful.
>>>
>>> One more thing from another thread: Yes, the new macro should refer
>>> to object file section naming. I want this patch to get object file
>>> sections proper and sound. From there on we can fix the linking
>>> stage to emit proper and sound executables in a later patch.
>>>
>>
>> OK then please refactor the commit message to make it clear that this
>> patch is to get the correct section in the object files, and work is
>> still need to get this into executable images.
>
> Sure.
>
>>
>> For Xcode you can make it __DATA__,__const since that is the closest
>> thing to read only data and I think that is your intent.
>
> I would like to do that, but only if we can ensure __DATA,__const is
> merged into .text, or is a separate RNX section.
>
>>
>> GenFW is part of EDKII BaseTools and mtoc is part of the open source
>> CCTOOLS project and both those tools would need to be modified to
>> create a .rodata section in PE/COFF.
>
> Yes, that should not be a big problem. Remaining issues for me:
> 1) How to merge __DATA,__const into .text, or how to emit a standalone
> .rodata section, for Xcode-based toolchains? (Some ideas above, will
> ping Vitaly soon as well)
> 2) How to submit modified mtoc? Any chance it could be maintained in
> EDK II like GenFw? (Would be nice if you could provide some insight)
> 3) How to merge .rdata into .text for MSVC? (I will try to research
> this soon-ish, but no promises)
> 4) How to design a toggle for the platform maintainer to choose
> between .text merge and standalone .rodata?
>
> Please note that I'm not asking you to research any of those questions
> (but 2) would be nice :) ), this is merely a summary of open points
> till the second stage (correct executables, not just correct object
> sections) can be properly approached.
>
> Thanks for your time and insight!
>
> Best regards,
> Marvin
>
>
> [1]
> https://github.com/apple/darwin-xnu/blob/a1babec6b135d1f35b2590a1990af3c5c5393479/osfmk/arm/arm_vm_init.c#L318-L324
>
> [2]
> https://github.com/apple/darwin-xnu/blob/2ff845c2e033bd0ff64b5b6aa6063a1f8f65aa32/makedefs/MakeInc.def#L578
>
> [3]
> https://github.com/apple/darwin-xnu/blob/8f02f2a044b9bb1ad951987ef5bab20ec9486310/libsa/lastkerneldataconst.c#L48
>
>>
>> Thanks,
>>
>> Andrew Fish
>>
>>> Best regards,
>>> Marvin
>>>
>>>>
>>>> Thanks,
>>>>
>>>> Andrew Fish
>>>>
>>>>> Best regards,
>>>>> Marvin
>>>>>
>>>>>> [1]
>>>>>> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm#L14
>>>>>> <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm#L14>
>>>>>> <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm#L14
>>>>>> <https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/BaseUefiCpuLib/X64/InitializeFpu.nasm#L14>>
>>>>>>
>>>>>>
>>>>>> [2] $otool -V -s __TEXT
>>>>>> __constBuild/OvmfX64/DEBUG_XCODE5/X64/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib/OUTPUT/X64/InitializeFpu.obj
>>>>>> Build//OvmfX64/DEBUG_XCODE5/X64/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib/OUTPUT/X64/InitializeFpu.obj:
>>>>>>
>>>>>> Contents of (__TEXT,__const) section
>>>>>> 0000001d 7f 03 80 1f 00 00
>>>>>>
>>>>>> $ otool -l
>>>>>> Build//OvmfX64/DEBUG_XCODE5/X64/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib/OUTPUT/X64/InitializeFpu.obj
>>>>>> Build/OvmfX64/DEBUG_XCODE5/X64/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib/OUTPUT/X64/InitializeFpu.obj:
>>>>>>
>>>>>> Load command 0
>>>>>> cmd LC_SEGMENT_64
>>>>>> cmdsize 232
>>>>>> segname
>>>>>> vmaddr 0x0000000000000000
>>>>>> vmsize 0x0000000000000026
>>>>>> fileoff 288
>>>>>> filesize 38
>>>>>> maxprot 0x00000007
>>>>>> initprot 0x00000007
>>>>>> nsects 2
>>>>>> flags 0x0
>>>>>> Section
>>>>>> sectname __text
>>>>>> segname __TEXT
>>>>>> addr 0x0000000000000000
>>>>>> size 0x000000000000001d
>>>>>> offset 288
>>>>>> align 2^0 (1)
>>>>>> reloff 328
>>>>>> nreloc 2
>>>>>> flags 0x80000500
>>>>>> reserved1 0
>>>>>> reserved2 0
>>>>>> Section
>>>>>> sectname __const
>>>>>> segname __TEXT
>>>>>> addr 0x000000000000001d
>>>>>> size 0x0000000000000006
>>>>>> offset 320
>>>>>> align 2^0 (1)
>>>>>> reloff 0
>>>>>> nreloc 0
>>>>>> flags 0x00000000
>>>>>> reserved1 0
>>>>>> reserved2 0
>>>>>> Load command 1
>>>>>> cmd LC_SYMTAB
>>>>>> cmdsize 24
>>>>>> symoff 344
>>>>>> nsyms 3
>>>>>> stroff 392
>>>>>> strsize 63
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Andrew Fish
>>>>>>
>>>>>>
>>>>>>> Thanks for your notes and insight!
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Marvin
>>>>>>>
>>>>>>>
>>>>>>> [1]
>>>>>>> "For compatibility with other Unix platforms, the following
>>>>>>> standard names are also supported:
>>>>>>> [...]
>>>>>>> .rodata = __DATA,__const data
>>>>>>> [...]
>>>>>>> If the .rodata section contains no relocations, it is instead
>>>>>>> put into the __TEXT,__const section unless this section has
>>>>>>> already been specified explicitly."
>>>>>>> https://www.nasm.us/xdoc/2.13.01/html/nasmdoc7.html
>>>>>>> <https://www.nasm.us/xdoc/2.13.01/html/nasmdoc7.html>
>>>>>>> <https://www.nasm.us/xdoc/2.13.01/html/nasmdoc7.html
>>>>>>> <https://www.nasm.us/xdoc/2.13.01/html/nasmdoc7.html>>
>>>>>>>
>>>>>>>> [1] otool -lh DxeCore.dll
>>>>>>>> ...
>>>>>>>> Load command 1
>>>>>>>> cmd LC_SEGMENT_64
>>>>>>>> cmdsize 312
>>>>>>>> segname __DATA
>>>>>>>> vmaddr 0x000000000002b000
>>>>>>>> vmsize 0x0000000000147000
>>>>>>>> fileoff 180224
>>>>>>>> filesize 8192
>>>>>>>> maxprot 0x00000003
>>>>>>>> initprot 0x00000003
>>>>>>>> nsects 3
>>>>>>>> flags 0x0
>>>>>>>> Section
>>>>>>>> sectname __const
>>>>>>>> segname __DATA
>>>>>>>> addr 0x000000000002b000
>>>>>>>> size 0x0000000000000718
>>>>>>>> offset 180224
>>>>>>>> align 2^4 (16)
>>>>>>>> reloff 0
>>>>>>>> nreloc 0
>>>>>>>> flags 0x00000000
>>>>>>>> reserved1 0
>>>>>>>> reserved2 0
>>>>>>>> Section
>>>>>>>> sectname __data
>>>>>>>> segname __DATA
>>>>>>>> addr 0x000000000002b720
>>>>>>>> size 0x00000000000014f0
>>>>>>>> offset 182048
>>>>>>>> align 2^4 (16)
>>>>>>>> reloff 0
>>>>>>>> nreloc 0
>>>>>>>> flags 0x00000000
>>>>>>>> reserved1 0
>>>>>>>> reserved2 0
>>>>>>>> Section
>>>>>>>> sectname __bss
>>>>>>>> segname __DATA
>>>>>>>> addr 0x000000000002cc10
>>>>>>>> size 0x0000000000144e11
>>>>>>>> offset 0
>>>>>>>> align 2^4 (16)
>>>>>>>> reloff 0
>>>>>>>> nreloc 0
>>>>>>>> flags 0x00000001
>>>>>>>> reserved1 0
>>>>>>>> reserved2 0
>>>>>>>> …
>>>>>>>>
>>>>>>>> [2]
>>>>>>>> https://opensource.apple.com/source/cctools/cctools-698/efitools/mtoc.c.auto.html
>>>>>>>> <https://opensource.apple.com/source/cctools/cctools-698/efitools/mtoc.c.auto.html>
>>>>>>>> <https://opensource.apple.com/source/cctools/cctools-698/efitools/mtoc.c.auto.html
>>>>>>>> <https://opensource.apple.com/source/cctools/cctools-698/efitools/mtoc.c.auto.html>>
>>>>>>>>
>>>>>>>>
>>>>>>>> [3] otool more output…
>>>>>>>> Load command 0
>>>>>>>> cmd LC_SEGMENT_64
>>>>>>>> cmdsize 392
>>>>>>>> segname __TEXT
>>>>>>>> vmaddr 0x0000000000000240
>>>>>>>> vmsize 0x00000000000296c0
>>>>>>>> fileoff 1184
>>>>>>>> filesize 169664
>>>>>>>> maxprot 0x00000005
>>>>>>>> initprot 0x00000005
>>>>>>>> nsects 4
>>>>>>>> flags 0x0
>>>>>>>> Section
>>>>>>>> sectname __text
>>>>>>>> segname __TEXT
>>>>>>>> addr 0x0000000000000240
>>>>>>>> size 0x000000000002489f
>>>>>>>> offset 1184
>>>>>>>> align 2^3 (8)
>>>>>>>> reloff 0
>>>>>>>> nreloc 0
>>>>>>>> flags 0x80000400
>>>>>>>> reserved1 0
>>>>>>>> reserved2 0
>>>>>>>> Section
>>>>>>>> sectname __cstring
>>>>>>>> segname __TEXT
>>>>>>>> addr 0x0000000000024ae0
>>>>>>>> size 0x000000000000496d
>>>>>>>> offset 150848
>>>>>>>> align 2^4 (16)
>>>>>>>> reloff 0
>>>>>>>> nreloc 0
>>>>>>>> flags 0x00000002
>>>>>>>> reserved1 0
>>>>>>>> reserved2 0
>>>>>>>> Section
>>>>>>>> sectname __ustring
>>>>>>>> segname __TEXT
>>>>>>>> addr 0x000000000002944e
>>>>>>>> size 0x0000000000000048
>>>>>>>> offset 169646
>>>>>>>> align 2^1 (2)
>>>>>>>> reloff 0
>>>>>>>> nreloc 0
>>>>>>>> flags 0x00000000
>>>>>>>> reserved1 0
>>>>>>>> reserved2 0
>>>>>>>> Section
>>>>>>>> sectname __const
>>>>>>>> segname __TEXT
>>>>>>>> addr 0x00000000000294a0
>>>>>>>> size 0x0000000000000448
>>>>>>>> offset 169728
>>>>>>>> align 2^4 (16)
>>>>>>>> reloff 0
>>>>>>>> nreloc 0
>>>>>>>> flags 0x00000000
>>>>>>>> reserved1 0
>>>>>>>> reserved2 0
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> Andrew Fish
>>>>>>>>
>>>>>>>>>
>>>>>>>>> DEBUG_XCODE5_IA32_CC_FLAGS = -arch i386 -c -g -Os
>>>>>>>>> -Wall -Werror -include AutoGen.h -funsigned-char
>>>>>>>>> -fno-stack-protector -fno-builtin -fshort-wchar -fasm-blocks
>>>>>>>>> -mdynamic-no-pic -mno-implicit-float -mms-bitfields
>>>>>>>>> -msoft-float -Wno-unused-parameter -Wno-missing-braces
>>>>>>>>> -Wno-missing-field-initializers -Wno-tautological-compare
>>>>>>>>> -Wno-sign-compare -Wno-varargs
>>>>>>>>> -ftrap-function=undefined_behavior_has_been_optimized_away_by_clang
>>>>>>>>> $(PLATFORM_FLAGS)
>>>>>>>>>
>>>>>>>>> @@ -3003,7 +3003,7 @@ RELEASE_XCODE5_X64_DLINK_FLAGS =
>>>>>>>>> -arch x86_64 -u _$(IMAGE_ENTRY_POINT) -e _
>>>>>>>>> DEBUG_XCODE5_X64_ASM_FLAGS = -arch x86_64 -g
>>>>>>>>>
>>>>>>>>> NOOPT_XCODE5_X64_ASM_FLAGS = -arch x86_64 -g
>>>>>>>>>
>>>>>>>>> RELEASE_XCODE5_X64_ASM_FLAGS = -arch x86_64
>>>>>>>>>
>>>>>>>>> - *_XCODE5_X64_NASM_FLAGS = -f macho64
>>>>>>>>>
>>>>>>>>> + *_XCODE5_X64_NASM_FLAGS = -f macho64
>>>>>>>>> -DRODATA_SECTION_NAME=.rodata
>>>>>>>>>
>>>>>>>>> *_XCODE5_*_PP_FLAGS = -E -x assembler-with-cpp
>>>>>>>>> -include AutoGen.h
>>>>>>>>>
>>>>>>>>> *_XCODE5_*_VFRPP_FLAGS = -x c -E -P -DVFRCOMPILE -include
>>>>>>>>> $(MODULE_NAME)StrDefs.h
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> 2.31.1
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#79205): https://edk2.groups.io/g/devel/message/79205
Mute This Topic: https://groups.io/mt/84764902/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list