This repository was archived by the owner on Jan 24, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any plans to support this in other linkers, like LLD? Presumably those won't use the GNU section, so we should make sure that we can support that without having multiple definitions of the
__veneer_symbols.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not aware of any plans for LLVM tools.
That's a good point. I think it is possible to have
#definein linker scripts and pass preprocessor constants to activate some code or not. We could do something like that in the future to either choose the LLVM or GNU sections, using thelink-argoption in.cargo/config.Alternatively we can prefix those symbols with
gnu_for now (eg__gnu_veneer_base).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth putting those symbols immediately outside the
.gnu.sgstubssection, for the same reasons we have with the other sections? That way we could define multiple sections within those symbol bounds, or users could define their own inmemory.xwithout needing to change link.x.in.Should the end symbol be aligned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure that sounds like a good idea! That could solve the problem we were talking just before. Do you mean with "users could define their own in memory.x without needing to change link.x.in" that users will be able to override those symbols to put the ones they want?
If that is the case I would be a little bit worried. The content of what is in between
__veneer_baseand__veneer_limitcan be an entry point to the Secure world, needs to be as small as possible and carefully audited. Although Secure application developpers ultimately need to be trusted that is maybe better to leave those symbols being only maintained here, just to avoid possible errors that could lead to security issues?It is fine if it is not, ultimately you would do
limit_nsc | 0x1Fas in the example above to align it with 32 bytes anyway. Maybe better to keep the information of the real size of the veneers there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users (the top-level project, not libraries) can already override anything in link.x.in (by specifying their own link.x). If the symbols are outside the section, then users can put a new section into their
memory.xand specifyINSERT AFTER .gnu.sgstubsand the section will be 'inside' the start/end symbols but with whatever name they require.In any event nothing stops any user or library code writing
#[link_section=".gnu.sgstubs"], right?Re alignment, sure, I don't mind too much. Aligning it in the linker means we don't need to OR it in the code, but you lose knowledge of the 'actual size' (does anything need to know that, though?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😨 oops that is a very valid point and I have just done a test to try putting another function in the
.gnu.sgstubssection with that attribute. I will look into it a bit more but if there is no protection from the linker against that it would mean that the Secure application developper needs to trust all of its dependencies. A malicious one could put a backdoor entry function in the veneers section...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Malicious dependencies can already do pretty much whatever they want, so that doesn't seem like an issue to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, you have to trust all your dependencies anyway. They can all run arbitrary code at build time, so...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you are both absolutely right, it will always come down to the fact that you have to trust your dependencies to not contain security vulnerabilities/be malicious. How a Secure application developper can be sure this is the case is another topic and does not concern this PR.
I will update the PR as discussed but would still like to keep the
__veneer_limitsymbol unaligned. It could be used as a way to count the number of entry functions declared in all the dependency graph as well (well if no malicious crate modify all of the build system 😰).