Change MEMTAG_STACK to be d_val, and MEMTAG_GLOBALS to be d_ptr

[hctim]

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]

@smithp35

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

[smithp35]

I've marked as approved. LGTM, I'll leave it open just in case my colleagues want to add any additional comments.

[hctim]

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]

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:

• A pretty printing tool like readelf might have different displays for d_val and d_ptr. This would not be the end of the world if it got it wrong though.
• A dynamic linker could do a RelocateMachineSpecificDynamicTagEntries() and add the programs displacement to all even entries. Later a switch statement could process the relocated entries.

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]

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]

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:

• Changes MEMTAG_STACK to be d_val instead of d_ptr without changing the number,
• Changes MEMTAG_GLOBALS to be d_ptr without changing the number,
• Explains why this is strange but why it's the least-awful option over introducing a whole new set of tags.

[smithp35]

Thanks for the update. I've approved the final version. The way the project is set up is a rebase and merge, so all 3 commits will be applied in the current state.

Internally we've been doing a force push of the squashed commit prior to clicking the merge button in github. I'm just double checking that with my colleagues. Hope to get you an answer soon.

Update: Yes, please do squash the changes so we'll be able to apply a single commit.

[hctim]

Wow, squashing was difficult. Sorry for any noise. Looks like I squashed it properly.

[smithp35]

Thanks for the update, and apologies for the faff.

[sallyarmneale]

remove thus - it's archaic language and difficult for non-native speakers.

"The values were updated to more fitting semantic types.

[sallyarmneale]

Couple of minor changes

[sallyarmneale]

change "need to be" to "should be"

Implementations of dynamic loaders should be careful to apply these semantics correctly. Most importantly, you should not apply the load bias to......

[stuij]

addressed by #278

[sallyarmneale]

Change to a bulleted list:

It has two possible values:

• 0 - indicates that the
• 1 - indicates that the....

[stuij]

addressed by #278