[PAuthABI] Use a program property to mark PAuth compatibility #105

pbarrio · 2021-07-21T09:33:15Z

A new program property GNU_PROPERTY_AARCH64_FEATURE_PAUTH in
section .note.gnu.property replaces the previous proposal of a
full section .note.AARCH64-PAUTH-ABI-tag. This is easier
to implement, at least in the Bionic dynamic linker: the property
(and the existing .note.gnu.property section) can be trivially
extracted from a DSO, and is already happening for other properties
(mainly GNU_PROPERTY_AARCH64_FEATURE_1_AND to check for BTI and
PAC-ret). In contrast, a new section would require traversing all
the sections in the ELF and checking for the section name, which is
more complicated to implement and possibly slower.

pauthabielf64/pauthabielf64.rst

pbarrio · 2021-07-23T14:34:30Z

pauthabielf64/pauthabielf64.rst

+to the platform identified in the first word. Consequently, the
+``pr_datasz`` field must be at least 16. When ``pr_datasz`` is larger
+than 16, the remainder of the contents of ``pr_data`` are specific
+to the (platform id, version number).

 This ABI does not determine the format of the platform identifier. Arm
 reserves the platform id 0 for a bare-metal platform.


@smithp35 would it be reasonable to specify some format, or at least add the values in a table so that platforms can add their identifiers here as they get allocated? I.e.
|| Platform || ID ||
| Baremetal | 0 |
| <Platform 1> | <ID 1> |
| ... | ... |

Keeping the allocations clearly in one place might help platforms avoid stepping on each other's toes.

I was expecting the majority of holders to use short null terminated strings such as "Arm\0" as there is 64-bits to play with. This would limit the possibility of clashes. However no objections to introducing a table. With the ABI in Github it should be easy to submit pull requests to update the table.

(On holiday today, so will be a bit slow responding)

Peter, thank you for the review. Feel free to respond after you are back from holidays :)

How about a table like this:

|| Platform || ID (hex) || ID (human-readable) ||
| Baremetal | 0x0 | 0 |
| Platform 1 | 0x506c617431000000 | "Plat1" |

Then we can define in this ABI the rule by which two platforms are different. For example:

The rule is that the full 64 bytes must be the same. "Arm\0\0\0\0\0" != "Arm\0Leg\0". I prefer this rule as it allows platforms to decide on the human-readable representation (string, integer, whatever) as long as the hex value is different to the rest of the platforms.

The rule is that the field represents a null-terminated string. "Arm\0\0\0\0\0" and "Arm\0Leg\0" are the same vendor.

Defining the rule in this ABI would make it easier for tools to implement the comparison consistently for all platforms.

WDYT?

apazos · 2021-07-27T16:42:15Z

Hi Pablo, your suggestion to use a new program property GNU_PROPERTY_AARCH64_FEATURE_PAUTH is good, and aligns with how BTI feature is handled too.

rearnsha · 2021-07-30T10:16:35Z

Use of GNU properties in the generic Arm ABI is not appropriate, IMO. It's too specific to one platform.

pbarrio · 2021-07-30T14:12:31Z

Use of GNU properties in the generic Arm ABI is not appropriate, IMO. It's too specific to one platform.

@rearnsha, thank you for the comment. What is too platform-specific, 1) using GNU properties in the generic Arm ABI or 2) the specific property I am proposing?

We already use GNU properties in the generic Arm ABI, e.g. https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst#special-sections. Do you see a difference between the new property and, say, GNU_PROPERTY_AARCH64_FEATURE_1_AND?

rearnsha · 2021-07-30T16:18:01Z

We should not be using any GNU properties in the the platform-agnostic parts of the ABI. If they're needed in platform-specific executables for Linux, that's OK. But another solution needs to be found in object files.

smithp35 · 2021-08-02T16:43:46Z

Just come back from holiday. Will have a think about what the alternatives are. Off the top of my head:

Use the raw SHT_NOTE section from the existing specification in relocatable objects but when targeting Linux/Android then write a GNU property section.
Have a mirror of .note.GNU.property in the Arm namespace (not particularly keen on this).
Some other mechanism, either a specific one for PAuthABI or a generic extensible one like BuildAttributes.

pbarrio · 2021-08-26T15:08:11Z

I just updated a new proposal, which is a bit of a mix between the original approach (a separate PAuth note section) and my later proposal (use the existing .note.gnu.property). The changes are:

We keep the separate note section as the default marking scheme. This is for all ELF files: relocatable, executables, shared objects... doesn't matter.
For platforms that understand .note.gnu.property, it is possible to use that section instead of a PAuth-specific one. This is, again, for all ELF files.

This means it is possible for a platform A to use .note.gnu.property and platform B to use .note.AARCH64-PAUTH-ABI-tag. Then, if we build an object for platform A and try to link it into an executable for platform B, the linker may assume that the object was built without PAuth, which is incorrect. Therefore, should we harden this patch to mandate the use of .note.AARCH64-PAUTH-ABI-tag in relocatable objects? This way, at least for relocatable objects, there would be only one way to look for PAuth ABI information, at the cost that a single platform may have to implement two different approaches, one for relocatable objects and one for the rest.

IMVHO, trying to build an object for one platform and linking it for another one with a different ABI is looking for trouble anyways, but I'd really like to know other people's views.

smithp35

I agree that platforms are realistically not going be mixed with any hope of success.

I've left a few alternative suggestions for how to present.

pauthabielf64/pauthabielf64.rst

pbarrio · 2021-08-31T16:27:38Z

Thank you Peter, that was very useful! All changes made now. Please have a look at the second change you proposed: I've structured it a bit differently and you may have other ideas. The rest of the changes I've made almost verbatim.

smithp35

Thanks for the update Pablo, I'm happy with the updated wording.

I think that this addresses the concerns of us exclusively using the GNU property in relocatable objects.

pbarrio · 2021-09-01T15:14:57Z

Ok one more update after I received some questions elsewhere, which I'm posting here for visibility:

seem to remember there were some initial issues in the GNU property tags with alignment - has that been thought about and resolved - it would be sensible to get this right from the get-go.

Addressed with a clarification of alignment requirements when using GNU program properties and the use of pr_padding.

the source of the specification for the tuple numbers seems a bit vague - How do I know which numbers to use?

Added a table with the platform IDs currently in use. Platforms are expected to start adding their choice of IDs to that table in order to prevent clashes.

I've also moved the baremetal platform from 0x0 to 0x1 in order to use 0x0 to represent an invalid platform. I have some uneasy feeling about using a valid platform ID (0x0 representing baremetal) in a tuple (0x0, 0x0) that means "invalid file". @smithp35 feel free to contradict! I also though (but haven't added to the patch) that, when using 0x0 as platform ID, we could use the second field of the tuple (version number) to contain a "reason for failure" so that consumers of the file could figure out why the file is invalid and provide good diagnostics at a minimum.

smithp35

LGTM thanks for the update.

A new program property GNU_PROPERTY_AARCH64_FEATURE_PAUTH is defined in section .note.gnu.property that can be used as an alternative method for signing PAuthABI-compatible ELF files.At least in the Bionic linker, this is easier to implement: the property (and the existing .note.gnu.property section) can be trivially extracted from a DSO, and is already happening for other properties (mainly GNU_PROPERTY_AARCH64_FEATURE_1_AND to check for BTI and PAC-ret). In contrast, a new section would require traversing all the sections in the ELF and checking for the section name, which is more complicated to implement and possibly slower. Unfortunately, .note.gnu.property is not guaranteed to be understood by all ELF-based platforms, so we still keep the new section .note.AARCH64-PAUTH-ABI-tag as the default marking schema.

pbarrio · 2021-09-03T16:43:39Z

Ok last change I hope.

I've replaced the requirement to use (0,0) as an "invalid" combination by just using a platform id = 0. This was reserved as "Invalid" in my previous iteration of the patch yesterday. This also means that we could in theory use the "version id" field for other purposes when the platform id is 0 - if we ever want to do that.
Slightly reworded the compatibility model to explicitly allow files without pointers to be marked with the section: "the presence of a .note.AARCH64-PAUTH-ABI-tag means that, if the file contains pointers, they were signed in a compatible way with the default signing...". This makes it easier for toolchains and loaders to reason about file compatibility, and for loaders to leave PAC on, instead of conservatively having to turn it off. It's still the platform's choice, though, to mark them or just trust files without any marking.

Let me know if you have any comments on these changes. I'll ask the ABI owner to merge the patch later next week.

Thanks everyone for all the reviews!

smithp35

I'm happy with the changes. Will need to ask an editor nicely to merge the changes.

pbarrio · 2021-09-06T10:28:10Z

@stuij, could you merge this request?

sallyarmneale

LGTM

pbarrio force-pushed the PAuthABIMarking branch 2 times, most recently from df8b343 to 343edfe Compare July 21, 2021 09:44

DanielKristofKiss suggested changes Jul 22, 2021

View reviewed changes

pauthabielf64/pauthabielf64.rst Outdated Show resolved Hide resolved

pbarrio force-pushed the PAuthABIMarking branch from 343edfe to c548c4b Compare July 23, 2021 14:23

pbarrio commented Jul 23, 2021

View reviewed changes

pbarrio force-pushed the PAuthABIMarking branch 3 times, most recently from dac78c5 to 19621f9 Compare August 26, 2021 14:46

smithp35 reviewed Aug 26, 2021

View reviewed changes

pbarrio force-pushed the PAuthABIMarking branch from 19621f9 to 0ff2e7b Compare August 31, 2021 16:23

smithp35 approved these changes Aug 31, 2021

View reviewed changes

pbarrio force-pushed the PAuthABIMarking branch from 0ff2e7b to c7fa642 Compare September 1, 2021 14:57

smithp35 approved these changes Sep 2, 2021

View reviewed changes

pbarrio force-pushed the PAuthABIMarking branch from c7fa642 to efbeb2c Compare September 3, 2021 16:22

smithp35 approved these changes Sep 6, 2021

View reviewed changes

stuij merged commit 9700e11 into ARM-software:main Sep 8, 2021

sallyarmneale reviewed Oct 28, 2021

View reviewed changes

pbarrio deleted the PAuthABIMarking branch December 3, 2021 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PAuthABI] Use a program property to mark PAuth compatibility #105

[PAuthABI] Use a program property to mark PAuth compatibility #105

pbarrio commented Jul 21, 2021 •

edited

Loading

pbarrio Jul 23, 2021 •

edited

Loading

smithp35 Jul 26, 2021

pbarrio Jul 26, 2021

apazos commented Jul 27, 2021

rearnsha commented Jul 30, 2021

pbarrio commented Jul 30, 2021

rearnsha commented Jul 30, 2021

smithp35 commented Aug 2, 2021

pbarrio commented Aug 26, 2021

smithp35 left a comment

pbarrio commented Aug 31, 2021

smithp35 left a comment

pbarrio commented Sep 1, 2021 •

edited

Loading

smithp35 left a comment

pbarrio commented Sep 3, 2021

smithp35 left a comment

pbarrio commented Sep 6, 2021 •

edited

Loading

sallyarmneale left a comment

[PAuthABI] Use a program property to mark PAuth compatibility #105

[PAuthABI] Use a program property to mark PAuth compatibility #105

Conversation

pbarrio commented Jul 21, 2021 • edited Loading

pbarrio Jul 23, 2021 • edited Loading

Choose a reason for hiding this comment

smithp35 Jul 26, 2021

Choose a reason for hiding this comment

pbarrio Jul 26, 2021

Choose a reason for hiding this comment

apazos commented Jul 27, 2021

rearnsha commented Jul 30, 2021

pbarrio commented Jul 30, 2021

rearnsha commented Jul 30, 2021

smithp35 commented Aug 2, 2021

pbarrio commented Aug 26, 2021

smithp35 left a comment

Choose a reason for hiding this comment

pbarrio commented Aug 31, 2021

smithp35 left a comment

Choose a reason for hiding this comment

pbarrio commented Sep 1, 2021 • edited Loading

smithp35 left a comment

Choose a reason for hiding this comment

pbarrio commented Sep 3, 2021

smithp35 left a comment

Choose a reason for hiding this comment

pbarrio commented Sep 6, 2021 • edited Loading

sallyarmneale left a comment

Choose a reason for hiding this comment

pbarrio commented Jul 21, 2021 •

edited

Loading

pbarrio Jul 23, 2021 •

edited

Loading

pbarrio commented Sep 1, 2021 •

edited

Loading

pbarrio commented Sep 6, 2021 •

edited

Loading