Regenerate LC_CODE_SIGNATURE during llvm-objcopy operations

[drodriguez]

Summary

Prior to this change, if a LC_CODE_SIGNATURE load command
was included in the binary passed to llvm-objcopy, the command and
associated section were simply copied and included verbatim in the
new binary. If rest of the binary was modified at all, this results
in an invalid Mach-O file. This change regenerates the signature
rather than copying it.

The code_signature_lc.test test was modified to include the yaml
representation of a small signed MachO executable in order to
effectively test the signature generation.

[nuriamari]

Fix variable name style, clarify CHECK prefix

[nuriamari]

Correct output file name resolution for windows

[alexander-shaposhnikov]

This comment has been deleted.

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

441

It looks like I've missed the previous diff https://reviews.llvm.org/D109803, sorry about being late.

1/ MachO.h is probably not the ideal place for this functionality, according to the top file comment "MachO.h declares the MachOObjectFile class". My first impression is that the public declaration probably should be placed into a separate file, e.g. "MachOCodeSignature.h" (and we need a detailed comment explaining what this class or function does)

2/ The responsibility of the class CodeSignatureSection seems to be somewhat unclear. Perhaps, we don't really need a class here at all. What would you say to just creating a function, e.g. std::string buildCodeSignatureStab(...) or void writeCodeSignatureStab(...) with some clear separation of the input / output and removing the unrelated pieces of functionality (e.g. msync on the buffer, this is probably not suitable for a library function) ? (and not expose the implementation details in the header file).

3/ Some minor comments, e.g. stripOutputFilePath - I suspect Suport/Path.h might already have some helper utilities

What I would recommend here - can we (temporarily) take a step back and reopen & revert D109803, if it were not a library interface I would not be so worried, but for some common and frequently used libraries like libObject I would highly suggest we should clean up the interface first and I'm more than happy to help.

[drodriguez]

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

441

Hi Alex,

We would prefer to fix forward instead of reverting, if possible. As it sits, D109803 haven't changed anything and it is just "dormant" code as far as LLVM respects. Only this diff uses that functionality.

I will review your comment next week more carefully and provide better answers.

1/ I think we can do this. It should not be a problem.
2/ The class tries to imitate the existing code in LLD, which was a class. There's two related functionalities to the class: measuring the size, and serializing. For both one needs the same inputs, so it was interesting to tie data and behaviour together.
2b/ msync on the buffer is actually important, and llvm-objcopy might not work unless we make that call. IIRC without that, if one was writing the same file, and that file was mmap'ed into memory, the kernel will not realize that the signature had changed, and will not evaluate again. I can look for a reference that explained the problem in a little bit more detail.
3/ We realize that it was not portable for Windows, and Nuri changed that this afternoon to use llvm::path instead. This diff has the fix, since the previous one was already landed.

Thanks for the help offering. Any feedback is welcome. We will try to get to the best version of this that satisfies everyone's requirements.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

441

Hi! Many thanks!
My 0.02$ . The current design looks a little bit suboptimal, especially for a library, so I suggest let's improve the interface first and put things into the right place. It's a bit hard to post comments outside of the code context, but yeah, I'm late to the party. The LLD-specific details ("CodeSignatureSection") can still live in the LLD codebase and make use of the function from libObject.
If there are no significant performance differences I would be biased towards readability / simplicity, e.g. one can consider something like this (I didn't verify the details, so maybe some parameters are unnecessary or, instead, missing)

SmallVector<char, 64> buildCodeSignatureStab(
    uint64_t SignatureOffset,
    MachO::HeaderFileType FileType, StringRef FilePath,
    uint64_t TextSegmentOffset, StringRef TextSegmentContent)

and in LLD the method CodeSignatureSection::finalize would call this function - would that be sufficient ? (maybe I'm missing something, feel free to correct me). (Plus in this case we will avoid having two different CodeSignatureSection classes)
and (mostly motivated by the "separation of concerns") I'd leave the unfortunate workaround with msync on the application's side, hopefully it'll be gone in the future.

[jhenderson]

Some stylistic comments. I'll leave @alexander-shaposhnikov to do the real review.

llvm/tools/llvm-objcopy/MachO/MachOLayoutBuilder.h

21

We don't usually bother marking member variables as const in the llvm-objcopy code, so I'd drop the const here and in the constructor parameter argument.

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

10

You should be able to include this as in the inline edit (cf ELF/ELFObjcopy.cpp which doesn't have the relative include). Same goes elsewhere.

llvm/tools/llvm-objcopy/MachO/Object.h

101

Nit: comments must end in a full stop. Same in the two other cases below.

[drodriguez]

@jhenderson: thanks for the feedback, we will apply those when we figure out the best shape for these changes.

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

441

About the ideal place for this code:

We thought about libObject and specifically MachO.h because it seemed like the best place, but we are fine relocating the code if necessary. We have also seen libBinaryFormat which includes some support for MachO and which might be a good place for these. The basic structs for the code signature (CS_CodeDirectory) is there, so it might be a good idea to keep everything together. Our only requirement is that it should be accessible to both llvm-objcopy and LLD to avoid code duplication.

These are not LLD specific details, as far as we understand, but part of how MachO binaries are put together and every tool that creates a MachO binary will need to play by these rules (they apply to each slice independently, so in our testing Universal MachO does not need changes, but there might be edge cases we are not aware).

About the shape of the code:

We don’t mind removing the class and having a couple of free functions instead, but there is the possibility of the responsibilities of the class growing bigger in the future, and we would like to avoid functions with a lot of parameters (there’s already 4 that both the serialization and the calculation of the size would need to receive).

About your proposal of buildCodeSignatureStab, is there an example of that pattern already in the code. We have looked for similar constructs and we cannot find it. Also, being “Stab” a term of art related to dSYM, maybe it is not the best name to use (alternatives can be something like “Blob”, or use “serialize” instead of “build”). Also, using SmallVector might be limited for this usage since the actual signature depends on the size of the binary, and for each 4 KB of binary we need an extra 32 bytes. I have seen signatures being 15 KB for LLVM binaries. We would prefer the versions that write directly into memory to avoid excessive copy of data.

Finally, about msync:

While we can move the msync calls into the application code, and add a note in the documentation of the method that the caller must ensure that msync gets invoked, we think it is better to keep the msync only in one place with a clear explanation of why it is necessary.

The details are in https://openradar.appspot.com/FB8914231 but in short, when mmaping an executable, the kernel seems to cache the results of the signature verification and might not check for the validity of the binary again, even if the signature gets regenerated. This is very typical while iterating on a binary, and creating the binary over and over, so it is something that can happen in both LLD and llvm-objcopy (specially in llvm-strip).

Apple might fix their side eventually, but until then we need the workaround everywhere, so we think a central place is better.

[alexander-shaposhnikov]

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

441

is there an example of that pattern already in the code. We have looked for similar constructs and we cannot find it.

If I'm not mistaken libObject was initially designed to present a read-only view of object files and closely models the binary formats, so, in general, what was added in https://reviews.llvm.org/D109803 does not have many analogs or counterparts there (to the best of my knowledge), although libObject contains some functionality to write archives and universal binaries.

I find the interface introduced in https://reviews.llvm.org/D109803 quite confusing and still think it would be better to revisit that decision (and revert for now) before it has propagated further. This can be subjective so I'm happy if somebody else familiar with libObject weighs in, maybe I'm missing something.

If I am not mistaken this code doesn't use the signer's identity in any way so it's more like a collection of hashes.
That's why above I called it a "stab". I think it also indicates that we need a detailed comment (in the source code) that should clarify these aspects.

These are not LLD specific details, as far as we understand, but part of how MachO binaries

LLD uses the term "section" to model various parts of the LINKEDIT segment, however, the binary format does not use this term (to the best of my knowledge) for this purpose. The class object::CodeSignatureSection looks very similar to what was initially implemented in LLD but I'm not sure it's a good fit for libObject, this is what I was referring to as "LLD-specific details" above.

Subscribing more people who are familiar with libObject & Mach-O:
cc @mtrent , @lattner, @steven_wu

[steven_wu]

+ @davide who might have more context of LC_CODE_SIGNITURE section

I totally agree that the API/Implementation you copied from lld is very not libObject. There should not be a write function inside libObject (you can reimplement it inside llvm-copy or other utilities).
I am also not quite sure what is your usecase for llvm-objcopy? That is not a tool available for Darwin and there is no guarantee that a objcopy of the binary can still run (could be a bincompat problem).

[drodriguez]

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

441

Seems fair that libObject is mainly for reading files. In libBinaryFormat is were the some of the struct used by LC_CODE_SIGNATURE are defined. There are small functions in there for MachO (mostly getters, but some setters). Would that be a good place? It also has a MsgPackWriter, which seems mostly unrelated, but seems to show that "writers" are OK in that library.

About being confusing, we are fine changing the couple of methods into free functions if that allow us to advance further. We chose a class because it was already that shape in LLD, and it made sense to keep that design. Would that be OK for everyone? It would be one function for measuring size, and one function to write the headers of the data contained in LC_CODE_SIGNATURE and calculate the appropriate hashes.

About the name "code signing": you are completely right that this does not use the signer identity at all and it is just a bunch of hashes. I think it should be understood more as a binary "signature", than a cryptographic signature of the code. It is mostly an HMAC, but without the cryptographic pieces. From what I understand it is just a way for the linker to output "verified" binaries in Apple Silicon Macs without having to setup signing identities and stuff like that.

We will add more comments to the code to try to explain better the purpose and which functionality is implemented.

About the term "section": Yes, LLD implements the linkedit segment with synthetic sections, and that name did slip, and I did not realize what you were referring to. These are not what MachO understands as sections. We will change that and probably make it clear that it is related to the CS_CodeDirectory, CS_SuperBlob and CS_BlobIndex.

In summary, if everyone agrees:

• Move the functionality outside libObject and probably into libBinaryFormat, where the struct CS_CodeDirectory is already living.
• Transform the functionality into a couple of free functions that receive all the necessary information.
• Do not mention "Section" as related to any of this functionality. Also try to not mention "code signature" that much, if a better name is available.
• Also the feedback provided by James Henderson above.

We hope the plan sounds good to everyone. Let us know if someone has more feedback that we can integrate.

[steven_wu]

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

441

I don't think libBinaryFormat is a good place to implement write.

I think a good design should be following:

• libObject (CodeSignatureSection) knows how to read the section and generations section with data
• CodeSignatureSection can return generated section data in a buffer if it has to.
• Binary write should be implemented inside tools (like llvm-objcopy, lld or yaml2obj). That is not really a code duplication.

[drodriguez]

@steven_wu: Thanks for the feedback. I think at this point we are completely conviced that libObject was not the right place. We are trying to find a good place, and we would like, as much as possible to not duplicate the same code or logic in two tools. It is not a lot of code right now, but the potential divergence in the future scares us a little.

The usecase for llvm-objcopy is that when trying to use llvm-strip on binaries linked by ld64 or lld the signature is currently copied verbatim, which makes the binary invalid in Apple Silicon machines. The changes here recalculates the signature in order to make the binary valid again. This seems to be what happens when using Xcode's own strip (which is not based on LLVM), and it is something that we wanted to replicate in order to keep being compatible. I am not sure about what you refer to with the "bincompat problem", but if you elaborate, I can try to figure out if there will be a problem that we haven't had into account.

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

441

@steven_wu: Thanks for the input. Let me try to see if we understand each other correctly.

• Neither LLD, nor llvm-objcopy are not really interested in "reading" the contents of the data from the LC_CODE_SIGNATURE. I would not like to design an API that has no users. If we needed to read from the section, and after Alexander explanations above, we now know that the right place is libObject, and we will add the code there, if necessary.
• To generate the data one needs access to the fully layout binary (slice). The process involves hashing every page of the binary (4 KB) and appending a hash of the contents. The data "header" can be written without access to the binary contents, but one still needs some information (mainly about offsets).
• The binary write that we have tried to abstract from the tools still require the tools to provide a buffer where the data is written (and a buffer where the binary contents are read, to generate the hashes). It was a happy coincidence that both tools worked with buffers, but if at some point in the future the writting mechanism have to be abstracted away, it should still be possible. I would not like to have the same process in each of the tools, if possible, since there is tricky pieces that would be bad to have in two parts of the code base (this was the main reason of bringing the LLD implementation into LLVM: we didn't want to simply duplicate the code).
• We are fine moving the msync out of this and into the tools, if that's necessary to make everyone happy. We do not think it is the right decision, but we are fine making that change.

[mtrent]

In D109972#3013610, @drodriguez wrote:

This seems to be what happens when using Xcode's own strip (which is not based on LLVM), and it is something that we wanted to replicate in order to keep being compatible.

I changed Apple's strip to call out to an os-supplied library present in the Xcode toolchain in order to build the code directory section for Apple Silicon. I believe Apple's linker is using this same strategy. The os-supplied library is now a dylib, which allows the details of the code directory to change without modifying the linker or strip, and it means the details may change between versions or maybe even be different between toolchains. I don't know how often this changes in practice.

That's something you should keep in mind while reverse-engineering this data structure for objcopy.

[drodriguez]

@mtrent: I noticed that change around the time of Xcode 12 (thanks!). However, even if the headers were available, I don't think LLVM would accept it as a dependency. What we are trying to do here is similar, with a common piece of code that handles the generation of these parts of the MachO binary. In the future, if it needs to be modified for some reason, all the tools should enjoy the improvements after a recompilation, instead of chasing down every tool implementation and modifying them.

[alexander-shaposhnikov]

I think there are a few considerations here, so to help unblock this effort below I'll try to summarize my understanding/perspective.

1/ Regarding the code placement - if currently there is no good place for it with a deep sigh I'd be okay to have a version of it in objcopy. This duplication is not ideal, to me it somehow feels ok to have it in some library
(e.g. ArchiveWriter is in libObject and it's used in multiple places), but I would recommend not to place it into the existing headers that essentially document the binary format and are not directly related to the new functionality.
But if there is no good place / or there are strong objections - okay, it's important to keep the public interfaces in a good shape.

2/ The blob ("signature" or "signature stab" (not sure which name is better here)) itself doesn't appear to be parsed anywhere, so, perhaps, at the moment no new parsing code is necessary in libObject.
If I understand correctly what this diff and the previous one try to accomplish - they try to add the functionality to generate this blob. I think having it in-tree has some benefits
(though as @mtrent has pointed out it's fragile), the main one - it allows the tools to be hosted pretty much anywhere (e.g. on Windows or Linux)
(similarly to other utilities, e.g. llvm-objdump, llvm-readobj, etc), the downside is that it can break if/when the format changes (but if I understand correctly we are kind of already living with it in LLD).

3/ After looking at the implementation the main concerns (mentioned above) are the following: from the readability perspective it seems suboptimal to mix together input and output
and use a "hybrid" approach to parameters passing.
It's also not obvious from the declaration that the buffer (passed to CodeSignatureSection::write(...)) must contain "a partially constructed mach-o object", that's why the interface looked kinda complicated
and anyone reading this code will have to dig into the actual implementation details.
The main work is done by the method CodeSignatureSection::write(...), so it seemed natural to have a function that either simply returns the blob (preferred way), or, if it's performance-critical (need numbers)
writes the content in-place, but I'd try to make the signature more intuitive (e.g. it should be clear to the reader what the input/output are).
If we end up introducing a class it would be good to clean up the interface (the same considerations as above),
hide the implementation details and probably use an appropriate name (e.g. CodeSignatureBuilder or <your suggestion is here>).

P.S. unlike StringTableBuilder here the whole thing is constructed in a single step, that's why my first impression was that a simpler solution should suffice, but I do not insist on it.
P.P.S. the current code makes sense in LLD's context, but the situation changes once we step away from it.

[drodriguez]

A follow up: after some discussion internally, we have decided that the best way forward should be the following:

1. Revert the previous commit as soon as possible.
2. Abandon this commit.
3. We will try to figure out doing the modifications locally in objcopy and try to add enough notices to point people towards modifying LLD at the same time to avoid divergences. Hopefully when the code needs to be added to a third tool (like install-name-tool or similar), we can figure out a way of reducing duplication or sharing more common pieces.
4. We are aware of the problems of fragility with the code. As you point out, those were already present in LLD. The only good thing we can say is that, from looking at the versioning, it seems to be well-thought about backwards compatibility, so we can only hope that it will work for some time before we have to play catch up again.
5. We will try to make it more obvious with better documentation, better naming, and better function signatures that this processing needs a full Mach-O binary in the buffer and what is going to happen to it. We were probably influenced by trying to understand how the code worked in LLD that we did not see that it might not have been easy to understand for someone without that experience.

llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp

441

@steven_wu: Thanks for you feedback. You can read the next steps we plan to take in the other comment. I think many of the points will not make sense in the new approach, but if you think something is important, or you want to provide commentary to my points above, we are happy to hear your feedback.

[drodriguez]

Reverted the previous one in https://reviews.llvm.org/D110974

[mtrent]

In D109972#3013964, @drodriguez wrote:

@mtrent: I noticed that change around the time of Xcode 12 (thanks!). However, even if the headers were available, I don't think LLVM would accept it as a dependency. What we are trying to do here is similar, with a common piece of code that handles the generation of these parts of the MachO binary. In the future, if it needs to be modified for some reason, all the tools should enjoy the improvements after a recompilation, instead of chasing down every tool implementation and modifying them.

That's fine. I'm just explaining that the code directory format is not part of/defined by the Mach-O file format.