[ObjCopy] Respect requirements of LC_ENCRYPTION_INFO commands #120995
Conversation
|
@llvm/pr-subscribers-llvm-binary-utilities Author: Daniel Rodríguez Troitiño (drodriguez) ChangesLLD (and other Mach-O linkers) when preparing an encryptable binary make space to leave all the load commands in an non-encrypted page (see 1) When using objcopy of a small encryptable binary, the code was not respecting this fact, and the encryptable segments were not kept beyond the first page. This was obvious for small or empty binaries. The changes introduced here keep track if a Full diff: https://github.com/llvm/llvm-project/pull/120995.diff 5 Files Affected:
diff --git a/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp b/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
index 93bc6631e64c86..d4eb6a9b9fc0b5 100644
--- a/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
+++ b/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp
@@ -116,6 +116,11 @@ uint64_t MachOLayoutBuilder::layoutSegments() {
const bool IsObjectFile =
O.Header.FileType == MachO::HeaderFileType::MH_OBJECT;
uint64_t Offset = IsObjectFile ? (HeaderSize + O.Header.SizeOfCmds) : 0;
+ if (O.EncryptionInfoCommandIndex) {
+ // If we are emitting an encryptable binary, our load commands must have a
+ // separate (non-encrypted) page to themselves.
+ Offset = alignToPowerOf2(HeaderSize + O.Header.SizeOfCmds, PageSize);
+ }
for (LoadCommand &LC : O.LoadCommands) {
auto &MLC = LC.MachOLoadCommand;
StringRef Segname;
diff --git a/llvm/lib/ObjCopy/MachO/MachOObject.cpp b/llvm/lib/ObjCopy/MachO/MachOObject.cpp
index d593d6788e112f..fe4b1cf4b12ae5 100644
--- a/llvm/lib/ObjCopy/MachO/MachOObject.cpp
+++ b/llvm/lib/ObjCopy/MachO/MachOObject.cpp
@@ -85,6 +85,10 @@ void Object::updateLoadCommandIndexes() {
case MachO::LC_DYLD_EXPORTS_TRIE:
ExportsTrieCommandIndex = Index;
break;
+ case MachO::LC_ENCRYPTION_INFO:
+ case MachO::LC_ENCRYPTION_INFO_64:
+ EncryptionInfoCommandIndex = Index;
+ break;
}
}
}
diff --git a/llvm/lib/ObjCopy/MachO/MachOObject.h b/llvm/lib/ObjCopy/MachO/MachOObject.h
index b3303fd291c82c..6b5c5c8d57bfbb 100644
--- a/llvm/lib/ObjCopy/MachO/MachOObject.h
+++ b/llvm/lib/ObjCopy/MachO/MachOObject.h
@@ -340,6 +340,8 @@ struct Object {
/// The index of the LC_SEGMENT or LC_SEGMENT_64 load command
/// corresponding to the __TEXT segment.
std::optional<size_t> TextSegmentCommandIndex;
+ /// The index of the LC_ENCRYPTION_INFO or LC_ENCRYPTION_INFO_64 load command if present.
+ std::optional<size_t> EncryptionInfoCommandIndex;
BumpPtrAllocator Alloc;
StringSaver NewSectionsContents;
diff --git a/llvm/lib/ObjCopy/MachO/MachOReader.cpp b/llvm/lib/ObjCopy/MachO/MachOReader.cpp
index 2b344f36d8e78b..ef0e0262f93958 100644
--- a/llvm/lib/ObjCopy/MachO/MachOReader.cpp
+++ b/llvm/lib/ObjCopy/MachO/MachOReader.cpp
@@ -184,6 +184,10 @@ Error MachOReader::readLoadCommands(Object &O) const {
case MachO::LC_DYLD_CHAINED_FIXUPS:
O.ChainedFixupsCommandIndex = O.LoadCommands.size();
break;
+ case MachO::LC_ENCRYPTION_INFO:
+ case MachO::LC_ENCRYPTION_INFO_64:
+ O.EncryptionInfoCommandIndex = O.LoadCommands.size();
+ break;
}
#define HANDLE_LOAD_COMMAND(LCName, LCValue, LCStruct) \
case MachO::LCName: \
diff --git a/llvm/test/tools/llvm-objcopy/MachO/strip-with-encryption-info.test b/llvm/test/tools/llvm-objcopy/MachO/strip-with-encryption-info.test
new file mode 100644
index 00000000000000..19b06b1ec02c81
--- /dev/null
+++ b/llvm/test/tools/llvm-objcopy/MachO/strip-with-encryption-info.test
@@ -0,0 +1,217 @@
+# RUN: rm -rf %t && mkdir %t
+# RUN: yaml2obj %s -o %t/original
+# RUN: llvm-strip --strip-all %t/original -o %t/stripped
+# RUN: llvm-readobj --macho-segment %t/stripped | FileCheck %s
+
+# CHECK-LABEL: Name: __PAGEZERO
+# CHECK: fileoff: 16384
+
+# CHECK-LABEL: Name: __TEXT
+# CHECK: fileoff: 16384
+
+# The YAML below is the following code
+# int main(int argc, char **argv) { return 0; }
+# Compiled on macOS against the macOS SDK and passing `-Wl,-encryptable`
+# Contents are removed, since they are not important for the test. We need a
+# small text segment (smaller than a page).
+--- !mach-o
+FileHeader:
+ magic: 0xFEEDFACF
+ cputype: 0x100000C
+ cpusubtype: 0x0
+ filetype: 0x2
+ ncmds: 15
+ sizeofcmds: 696
+ flags: 0x200085
+ reserved: 0x0
+LoadCommands:
+ - cmd: LC_SEGMENT_64
+ cmdsize: 72
+ segname: __PAGEZERO
+ vmaddr: 0
+ vmsize: 4294967296
+ fileoff: 0
+ filesize: 0
+ maxprot: 0
+ initprot: 0
+ nsects: 0
+ flags: 0
+ - cmd: LC_SEGMENT_64
+ cmdsize: 232
+ segname: __TEXT
+ vmaddr: 4294967296
+ vmsize: 32768
+ fileoff: 0
+ filesize: 32768
+ maxprot: 5
+ initprot: 5
+ nsects: 2
+ flags: 0
+ Sections:
+ - sectname: __text
+ segname: __TEXT
+ addr: 0x100004000
+ size: 32
+ offset: 0x4000
+ align: 2
+ reloff: 0x0
+ nreloc: 0
+ flags: 0x80000400
+ reserved1: 0x0
+ reserved2: 0x0
+ reserved3: 0x0
+ - sectname: __unwind_info
+ segname: __TEXT
+ addr: 0x100004020
+ size: 4152
+ offset: 0x4020
+ align: 2
+ reloff: 0x0
+ nreloc: 0
+ flags: 0x0
+ reserved1: 0x0
+ reserved2: 0x0
+ reserved3: 0x0
+ - cmd: LC_SEGMENT_64
+ cmdsize: 72
+ segname: __LINKEDIT
+ vmaddr: 4295000064
+ vmsize: 592
+ fileoff: 32768
+ filesize: 592
+ maxprot: 1
+ initprot: 1
+ nsects: 0
+ flags: 0
+ - cmd: LC_DYLD_CHAINED_FIXUPS
+ cmdsize: 16
+ dataoff: 32768
+ datasize: 48
+ - cmd: LC_DYLD_EXPORTS_TRIE
+ cmdsize: 16
+ dataoff: 32816
+ datasize: 48
+ - cmd: LC_SYMTAB
+ cmdsize: 24
+ symoff: 32872
+ nsyms: 2
+ stroff: 32904
+ strsize: 32
+ - cmd: LC_DYSYMTAB
+ cmdsize: 80
+ ilocalsym: 0
+ nlocalsym: 0
+ iextdefsym: 0
+ nextdefsym: 2
+ iundefsym: 2
+ nundefsym: 0
+ tocoff: 0
+ ntoc: 0
+ modtaboff: 0
+ nmodtab: 0
+ extrefsymoff: 0
+ nextrefsyms: 0
+ indirectsymoff: 0
+ nindirectsyms: 0
+ extreloff: 0
+ nextrel: 0
+ locreloff: 0
+ nlocrel: 0
+ - cmd: LC_ENCRYPTION_INFO_64
+ cmdsize: 24
+ cryptoff: 16384
+ cryptsize: 16384
+ cryptid: 0
+ pad: 0
+ - cmd: LC_LOAD_DYLINKER
+ cmdsize: 32
+ name: 12
+ Content: '/usr/lib/dyld'
+ ZeroPadBytes: 7
+ - cmd: LC_UUID
+ cmdsize: 24
+ uuid: 4C4C4447-5555-3144-A18A-01E9EB7E7D92
+ - cmd: LC_BUILD_VERSION
+ cmdsize: 32
+ platform: 1
+ minos: 983040
+ sdk: 983552
+ ntools: 1
+ Tools:
+ - tool: 4
+ version: 1310720
+ - cmd: LC_MAIN
+ cmdsize: 24
+ entryoff: 16384
+ stacksize: 0
+ - cmd: LC_FUNCTION_STARTS
+ cmdsize: 16
+ dataoff: 32864
+ datasize: 8
+ - cmd: LC_DATA_IN_CODE
+ cmdsize: 16
+ dataoff: 32872
+ datasize: 0
+ - cmd: LC_CODE_SIGNATURE
+ cmdsize: 16
+ dataoff: 32944
+ datasize: 416
+LinkEditData:
+ ExportTrie:
+ TerminalSize: 0
+ NodeOffset: 0
+ Name: ''
+ Flags: 0x0
+ Address: 0x0
+ Other: 0x0
+ ImportName: ''
+ Children:
+ - TerminalSize: 0
+ NodeOffset: 5
+ Name: _
+ Flags: 0x0
+ Address: 0x0
+ Other: 0x0
+ ImportName: ''
+ Children:
+ - TerminalSize: 4
+ NodeOffset: 33
+ Name: main
+ Flags: 0x0
+ Address: 0x4000
+ Other: 0x0
+ ImportName: ''
+ - TerminalSize: 2
+ NodeOffset: 39
+ Name: _mh_execute_header
+ Flags: 0x0
+ Address: 0x0
+ Other: 0x0
+ ImportName: ''
+ NameList:
+ - n_strx: 2
+ n_type: 0xF
+ n_sect: 1
+ n_desc: 0
+ n_value: 4294983680
+ - n_strx: 8
+ n_type: 0xF
+ n_sect: 1
+ n_desc: 16
+ n_value: 4294967296
+ StringTable:
+ - ' '
+ - _main
+ - __mh_execute_header
+ - ''
+ - ''
+ - ''
+ - ''
+ FunctionStarts: [ 0x4000 ]
+ ChainedFixups: [ 0x0, 0x0, 0x0, 0x0, 0x20, 0x0, 0x0, 0x0, 0x30, 0x0,
+ 0x0, 0x0, 0x30, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+ 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+ 0x0, 0x0, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
+ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 ]
+...
+
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
LLD (and other Mach-O linkers) when preparing an encryptable binary make space to leave all the load commands in an non-encrypted page (see [1]) When using objcopy of a small encryptable binary, the code was not respecting this fact, and the encryptable segments were not kept beyond the first page. This was obvious for small or empty binaries. The changes introduced here keep track if a `LC_ENCRYPTION_INFO` or `LC_ENCRYPTION_INFO_64` has been seen, and in such case, it adds a full page of offset in order to leave the load commands in its own page (similar to what LLD is doing). [1]: https://github.com/llvm/llvm-project/blob/d8e792931226b15d9d2424ecd24ccfe13adc2367/lld/MachO/SyntheticSections.cpp#L90-L93
drodriguez
force-pushed
the
encryption-info-bug
branch
from
f6da133 to
c4c7b0d
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/10752 Here is the relevant piece of the build log for the reference |
|
Builder seems to be working after a couple of failed builds (https://lab.llvm.org/buildbot/#/builders/59/builds/10754). |
|
Builds stripped with llvm-strip -S after this change don't run on arm64 macos anymore, while the build before strip runs fine. See for example the binaries in https://glandium.org/files/jsshell.zip . Download (with curl to avoid quarantine) and uncompress the file, then run ./js. It'll work. |
|
@drodriguez fwiw - perhaps, comparing the output of cctools' strip vs llvm-strip would shed some light on what's going wrong here. |
|
FWIW: Starting over: cctools-port does this: and the resulting binary works out of the box. Edit: |
|
@glandium is the binary signed before llvm-project/llvm/lib/ObjCopy/MachO/MachOLayoutBuilder.cpp Lines 419 to 424 in c025b96 I am also suprised that you write "after this change". Were things working before this change? Because I think any change to a signed binary would had invalidated the signature. For what it is worth, our process is creating the binary with If you don't mind, I will not download any binary, but if you find a simple source code + build script to reproduce the problem I can have a better look at it. |
|
So, the repro requires an old version of cctools-port, and ldid. It's possible that it's reproducible on macos, but ldid requires building openssl, and I stopped there. For convenience, here's a Dockerfile that will build it all: You may do something like Build with: I know you said you wouldn't download a binary, but here's a small one you can inspect and see it doesn't do anything nefarious: https://glandium.org/files/dummy (the entirety of the text segment is a function that does nothing, it's compiled from the source above, but with -O3) |
|
I don't believe this patch is quite correct, see #130472 What this patch seems to do, is push the entire What (I believe) it should instead be doing, is push only the first section within According to https://github.com/apple-oss-distributions/dyld/blob/b492ac15734277d89795b6f97f0e2feb1aa45595/mach_o/Header.cpp#L867-L873, the load commands must be included in the |
…#130517) Bug introduced #120995. The LLD code calculates the "size" of the Mach-O headers, and then uses that size to place the segments, but the `__TEXT` section stays at `fileoff` zero. When I wrote the code into llvm-objcopy I calculated the extra space into the initial offset, which moved all the sections back 1 page. Besides the modified test checking for the right `fileoff` values of the sections and the segments, I also manually checked the generated binaries after `llvm-objcopy` using `dyld_info`, as the bug report suggested. Fixes #130472
…llvm#130517) Bug introduced llvm#120995. The LLD code calculates the "size" of the Mach-O headers, and then uses that size to place the segments, but the `__TEXT` section stays at `fileoff` zero. When I wrote the code into llvm-objcopy I calculated the extra space into the initial offset, which moved all the sections back 1 page. Besides the modified test checking for the right `fileoff` values of the sections and the segments, I also manually checked the generated binaries after `llvm-objcopy` using `dyld_info`, as the bug report suggested. Fixes llvm#130472 (cherry picked from commit 8413f4d)
LLD (and other Mach-O linkers) when preparing an encryptable binary make space to leave all the load commands in an non-encrypted page (see 1)
When using objcopy of a small encryptable binary, the code was not respecting this fact, and the encryptable segments were not kept beyond the first page. This was obvious for small or empty binaries.
The changes introduced here keep track if a
LC_ENCRYPTION_INFOorLC_ENCRYPTION_INFO_64has been seen, and in such case, it adds a full page of offset in order to leave the load commands in its own page (similar to what LLD is doing).