search

ARM-software / abi-aa

Application Binary Interface for the Arm® Architecture
Other
878 stars 173 forks source link

Change MEMTAG_STACK to be d_val, and MEMTAG_GLOBALS to be d_ptr #225

Closed hctim closed 9 months ago

hctim commented 9 months ago

MEMTAG_STACK should be d_val (a bit to indicate that MTE stack is either on/off), and MEMTAG_GLOBALS should be d_ptr (a pointer offset to a section containing memtag globals descriptors).

This is the way that lld has implemented it, as it's the local semantics (MEMTAG_GLOBALS is a pointer-to-section that needs load bias adjustment, and MEMTAG_STACK is a single bit that enables PROT_MTE on the stack).

This unfortunatly does mean that we diverge from the accepted semantics of odd == d_val, and even == d_ptr, but this seems like the best trade-off as we've shipped a compiler that did the non-spec-but-more-logical thing and shipped binaries to end users. Much better than shipping a whole new set of MEMTAG_GLOBALS_2 and MEMTAG_STACK_2 to fix the mistake.

Found by eugenis@ in https://r.android.com/2765590

hctim commented 9 months ago

@smithp35

P.S. I don't have access to add reviewers to this patch :)

hctim commented 9 months ago

I had a longthink about the implications of the ABI change, and discussed it with my team. We do have binaries on Android that have been deploayed with the old DT_ values, which are unfortunately going to probably stick around for a while.

But, I think the nature of dynamic entries is to be interpereted in an application-specific way. Yes, d_ptr is supposed to always be a virtual address offset (to which a load bias is to be added), but I think a loader that indiscriminately does that still needs to use the value in an application-specific manner.

I think it's a better option to leave the current values as the wrong type, and just add a note below saying "sorry, we messed this up, please interpret MEMTAG_GLOBALS basically as a d_ptr (which seems totally fine, d_val is super application specific), and please interpret MEMTAG_STACK basically as a d_val (which is a little more gross). Doing this seems substantially better than the alternative of having all dynamic loaders basically handle MEMTAG_STACK || MEMTAG_STACK_REAL for the rest of time. Especially given d_ptr and d_val are a union over the same 64 bit value, I'm honestly not even sure why there's this distinction in the ELF spec to begin with...

smithp35 commented 9 months ago

I think that you are right that a dynamic linker that did not recognize the tag couldn't hope to run the binary. I think the cases where it could be a problem are:

In practice it looks like no open-source ELF reader or dynamic loader that I checked makes use of the even entries being d_ptr. It seems like every one uses a switch statement and combines the relocation and processing at the same time.

It does seem like if a dynamic linker did make use of the property for relocation, it could compensate later by subtracting/adding the displacement when doing the memtag specific processing later on.

To summarise, I personally would be happy with your proposal to put a note explaining that the two tags have the wrong parity (if that's the right word) according to the ELF standard. I'll open it up to my colleagues to see if there are any objections.

MaskRay commented 9 months ago

https://www.sco.com/developers/gabi/latest/ch5.dynamic.html says

A tag whose value is an odd number indicates a dynamic section entry that uses d_val or that uses neither d_ptr nor d_val. Tags whose values are less than the special value DT_ENCODING and tags whose values fall between DT_HIOS and DT_LOPROC do not follow these rules.

A processor can provide additional requirement to processor-specific values. However, some processors, such as MIPS and PPC64, have values that do not follow the even/odd convention. It's probably still a good idea to follow the convention, but I agree that as the values as been deployed, there is not much need to change it now.

If the values haven't been deployed, adding new values looks good to me.

hctim commented 9 months ago

I've updated the patch, commit message, and top post (which I'm guessing should be the squashed merge commit) now with the wording that I think is correct.

The patch: