-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable write-implies-execute-never when applicable #1550
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a big fan of this. Why add CFG_HWSUPP_MEM_PERM_WXN, and not just set CFG_CORE_RWDATA_NOEXEC only if you want it and your platform supports it?
@@ -189,6 +189,9 @@ UNWIND( .cantunwind) | |||
orr r0, r0, #SCTLR_A | |||
bic r0, r0, #SCTLR_C | |||
bic r0, r0, #SCTLR_I | |||
#if defined(CFG_HWSUPP_MEM_PERM_WXN) && defined(CFG_CORE_RWDATA_NOEXEC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to test only CFG_CORE_RWDATA_NOEXEC
here.
Optionally, add a dependency check in a core/arc/arm/arm.mk
(but see my other comment, I feel the HWSUPP config is useless):
ifeq ($(CFG_CORE_RWDATA_NOEXEC))
ifneq ($(CFG_HWSUPP_MEM_PERM_WXN),y)
$(error ...)
endif
endif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some HW (C-A5, C-A9) do not support WXN and UWXN in SCTLR.
So we need this CFG_HWSUPP_MEM_PERM_WXN
.
This is why the directive is not used in AARCH64, as all ARMv8-A do support SCTLR[WXN].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that. But we can simply assume that platforms that don't support CFG_CORE_RWDATA_NOEXEC simply don't enable it, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. At least for the few platforms lacking this hw hardening, op-tee core can still build a mmu that does rw-data-noexec. Weaker protection but at least optee legitimate mapping is clean.
The arm-tf does both: its mmu api never maps rwx memory and it sets STCRL[WXs], of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I get it. We really have two things to configure here. Sorry for the confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit ("core: factorize cpu support") is a good idea IMO.
Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
rebased |
So, for both patches: |
HW may or may not support STCLR "WXN" configuration field. CFG_HWSUPP_MEM_PERM_WXN reflects this state. AArch64 is assumed to always support this field. Enable the "WXN" (and UWXN) bits in STCLR upon configuration directive CFG_CORE_RWDATA_NOEXEC. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Create core/arch/arm/cpu/<cpu-name>.mk to store CPU generic configurations settings. Update supported platforms to rely on the generic CPU support. Platform shall still specify whether they support or not the NEON extension. Cortex-A53 and Cortex-A57 are all ARMv8.0 compliant. For ARMv8 core, we will use ARMv8-A architecture minor version configuration files. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
Rebased, tag applied. |
No description provided.