atrosinenko wrote:
I would like to share some thoughts on naming of the new options, please don't
consider this as a "change request" but merely as thinking out loud. These are
somewhat inspired by an already existing note in the [downstream
patch](https://github.com/arm/arm-toolchain/pull/731) that "Their naming is
still up for debate in the upstream review" (I didn't find the discussion,
though - it is probably worth adding a direct link).
Having read the PR description, I was a bit concerned that the new argument's
name adds "yet another layer of abstraction":
* there is a regular handling of register spilling that facilitates the very
possibility to perform arbitrary function calls
* there is a hardening of spilled LR in case something went wrong and an
attacker turned out to be able to tamper with the stack contents as a result of
some other vulnerability
* this patch set implements a *hardening of the above hardening*
*(But I have to admit that the resulting command lines in the tests look quite
logical)*
In other words, `-mharden-pac-ret` name (in isolation) might be slightly
misleading to the user: "pac-ret **is** a hardening, does this option enable
pac-ret?". Strictly speaking, "pac-ret" in `-mharden-pac-ret` is an object and
"harden" is a verb, so everything is correct ("apply hardening to pac-ret"),
but what if consider another structure:
* there is regular register spilling
* there is an option to additionally harden the LR when it is spilled to the
stack, which is enabled by `-mbranch-protection=pac-ret`
- the pac-ret hardening can be further configured by adding `+option`s:
`-mbranch-protection=pac-ret+leaf+pc+b-key`
- what if add yet another option for extra PACMAN mitigation
This way, instead of chain of hardenings (spilled LR <- pac-ret <- this patch),
there would be a flattened pac-ret hardening with configuration options:
* [ ] use IB key instead of IA
* [ ] make use of PAuthLR
* [ ] apply pac-ret to all functions instead of only non-leaf ones
* [ ] emit extra mitigations against PACMAN attack **(new)**
There is a drawback, though, that the sub-option's names gets quite long with
my approach (something like `+pacman-load` at the very least). Furthermore, as
far as I understand, it is planned (or at least possible) that other PACMAN
mitigation will be provided for pac-ret in the future. This means the
sub-language of the argument of `-mbranch-protection=` option would get
somewhat complicated.
Another drawback would be if it will be decided later to mitigate PACMAN in
other contexts, as this hardening mode selector would be exclusively tied to
pac-ret. This is not a big issue, I hope, as these other context would probably
deserve their independent configuration options anyway.
https://github.com/llvm/llvm-project/pull/176171
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
