This is an archive of the discontinued LLVM Phabricator instance.

[LLD][ELF] InX::BssRelRo should check section contents before marking relro
ClosedPublic

Authored by peter.smith on Dec 1 2017, 8:55 AM.

Details

Summary

When a linker script is used with a pattern like { *(.bss .bss.*) } the InX::BssRelRo section will match against .bss.*. We want to make sure that when InX::BssRelRo hasn't created any .bss.rel.ro InputSections we don't report the .bss OutputSection as relro. We also want to give an error message if relro and non relro bss is placed within the same OutputSection.

This is an attempt at fixing the problem that motivated r318924 (D40364) in a way that doesn't need to check for empty or zero sized sections. The Synthetic Sections InX::BssRelRo and InX::Bss behave differently to other SyntheticSections in that they act as a proxy for creating .bss.rel.ro or .bss InputSections for copy relocations in their parent OutputSections. This means that they are always removed by removeUnusedSyntheticSections() and always have a zero size. This patch attempts to do the following:

  • Move the name test before the InX::BssRelRo so that if it matches into an existing OutputSection that is already relro we are relro.
  • If the OutputSection name is .bss.rel.ro then this is strong statement of intent like .data.rel.ro then we are relro.
  • If all the InputSections in the parent OutputSection are .bss.rel.ro then we are relro.
  • If none of the InputSections in the parent OutputSection are .bss.rel.ro then we are not relro.
  • If there are both relro and non-relro InputSections then give an error message.

Diff Detail

Repository
rL LLVM

Event Timeline

peter.smith created this revision.Dec 1 2017, 8:55 AM

peter.smith created this revision.
Herald added a subscriber: emaste.

When a linker script is used with a pattern like { *(.bss .bss.*) } the InX::BssRelRo section will match against .bss.*. We want to make sure that when InX::BssRelRo hasn't created >>any .bss.rel.ro InputSections we don't report the .bss OutputSection as relro. We also want to give an error message if relro and non relro bss is placed within the same >>OutputSection.

I wonder if we need to do that much.

Any thoughts on the attached patch? It uses the idea of depending on the
output section name for .bss.rel.ro but doesn't issue an error message
if the user puts .bss.rel.ro in a rw section. That is similar to what
happens with .data.rel.ro, no?

I'm happy with Rafael's patch and have incorporated it into the review. This passes the important test case, I've removed the test case for the error message as this is less likely to happen as a user would have to explicitly name a .bss.rel.ro output section and then put .bss in it.

jhenderson added inline comments.Dec 4 2017, 6:12 AM
test/ELF/relro-copyrel-bss-script.s
4–5 ↗(On Diff #125305)

Is there a different test already for the opposite case, where we have .bss.rel.ro (with or without any other .bss), but with the same script? If not, I think that it would be beneficial to add it.

Thanks for the comment, I've adapted the test to include parts of the previous error if mix .bss and .bss.rel.ro. I'm not sure if it adds much value in having two separate test cases, but do let me know if you'd like them split up?

jhenderson accepted this revision.Dec 4 2017, 7:38 AM

LGTM, with one minor test comment inline.

To summarise the new behaviour as I understand it: "if linker script includes .bss.rel.ro in a section named something else and isn't otherwise relro, then don't treat it as relro". Assuming that's correct, I think that's fine. In my view, if you've written a linker script like that, you've done so deliberately, and don't want the linker preventing you having output as you requested. Yes, it means that there's a theoretical security hole, but you did ask for it, so it's not for the linker to judge.

test/ELF/relro-copyrel-bss-script.s
24–25 ↗(On Diff #125331)

Does this bit do anything in the test? If not, please delete it. If it does, please comment on its purpose.

This revision is now accepted and ready to land.Dec 4 2017, 7:38 AM

I agree with your summary, and I'll remove the .space pre-commit. Will wait till tomorrow to do so.

test/ELF/relro-copyrel-bss-script.s
24–25 ↗(On Diff #125331)

I'll remove it. It is a hold over from a previous test where I wanted some non bss, non relro data after the relro for .dynamic etc, but that won't happen with this linker script.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.