diff mbox series

[2/2] arm64: Introduce HWCAPS2_EXECONLY

Message ID 20201119133953.15585-3-vladimir.murzin@arm.com (mailing list archive)
State New, archived
Headers show
Series arm64: Support Enhanced PAN | expand

Commit Message

Vladimir Murzin Nov. 19, 2020, 1:39 p.m. UTC
With EPAN supported it might be handy to user know that PROT_EXEC
gives execute-only permission, so advertise it via HWCAPS2_EXECONLY

Cc: Kees Cook <keescook@chromium.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
---
 arch/arm64/include/asm/hwcap.h      | 1 +
 arch/arm64/include/asm/sysreg.h     | 1 +
 arch/arm64/include/uapi/asm/hwcap.h | 1 +
 arch/arm64/kernel/cpufeature.c      | 3 +++
 arch/arm64/kernel/cpuinfo.c         | 1 +
 5 files changed, 7 insertions(+)

Comments

Dave Martin Dec. 8, 2020, 4:36 p.m. UTC | #1
On Thu, Nov 19, 2020 at 01:39:53PM +0000, Vladimir Murzin wrote:
> With EPAN supported it might be handy to user know that PROT_EXEC
> gives execute-only permission, so advertise it via HWCAPS2_EXECONLY
> 
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> ---
>  arch/arm64/include/asm/hwcap.h      | 1 +
>  arch/arm64/include/asm/sysreg.h     | 1 +
>  arch/arm64/include/uapi/asm/hwcap.h | 1 +
>  arch/arm64/kernel/cpufeature.c      | 3 +++
>  arch/arm64/kernel/cpuinfo.c         | 1 +
>  5 files changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h
> index 9a5498c..5ee5bce 100644
> --- a/arch/arm64/include/asm/hwcap.h
> +++ b/arch/arm64/include/asm/hwcap.h
> @@ -105,6 +105,7 @@
>  #define KERNEL_HWCAP_RNG		__khwcap2_feature(RNG)
>  #define KERNEL_HWCAP_BTI		__khwcap2_feature(BTI)
>  #define KERNEL_HWCAP_MTE		__khwcap2_feature(MTE)
> +#define KERNEL_HWCAP_EXECONLY		__khwcap2_feature(EXECONLY)

Should this definitely be an hwcap?

[Apologies if I already made this comment, but if I did I can't find a
record of it, so here it is again (or not)]:

This seems to have the wrong semantics for hwcaps: it's not a (purely) a
property of the hardware, not an arch-specific concept, and old code
that doesn't know about this flag may not work properly when the flag
is set.

Software that requires that any memory mapped without PROT_READ is
readable would be nonportable according to POSIX, but nonportable
doesn't mean not correct; it just means that POSIX doesn't gurarantee
that it works everywhere.


So:

1) Is true execute-only memory an ABI break that we care about, and do
we need an explicit opt-in?

2) Otherwise, is there another more suitable and less arch-specific
mechanism that could be used?  (Maybe AT_FLAGS or similar?)

This issue may have come up on other arches.  I've not gone digging.

Cheers
---Dave

[...]
Catalin Marinas Dec. 8, 2020, 5:34 p.m. UTC | #2
On Tue, Dec 08, 2020 at 04:36:16PM +0000, Dave P Martin wrote:
> On Thu, Nov 19, 2020 at 01:39:53PM +0000, Vladimir Murzin wrote:
> > With EPAN supported it might be handy to user know that PROT_EXEC
> > gives execute-only permission, so advertise it via HWCAPS2_EXECONLY
> > 
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> > ---
> >  arch/arm64/include/asm/hwcap.h      | 1 +
> >  arch/arm64/include/asm/sysreg.h     | 1 +
> >  arch/arm64/include/uapi/asm/hwcap.h | 1 +
> >  arch/arm64/kernel/cpufeature.c      | 3 +++
> >  arch/arm64/kernel/cpuinfo.c         | 1 +
> >  5 files changed, 7 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h
> > index 9a5498c..5ee5bce 100644
> > --- a/arch/arm64/include/asm/hwcap.h
> > +++ b/arch/arm64/include/asm/hwcap.h
> > @@ -105,6 +105,7 @@
> >  #define KERNEL_HWCAP_RNG		__khwcap2_feature(RNG)
> >  #define KERNEL_HWCAP_BTI		__khwcap2_feature(BTI)
> >  #define KERNEL_HWCAP_MTE		__khwcap2_feature(MTE)
> > +#define KERNEL_HWCAP_EXECONLY		__khwcap2_feature(EXECONLY)
> 
> Should this definitely be an hwcap?
> 
> [Apologies if I already made this comment, but if I did I can't find a
> record of it, so here it is again (or not)]:

I don't think you did ;).

> This seems to have the wrong semantics for hwcaps: it's not a (purely) a
> property of the hardware, not an arch-specific concept, and old code
> that doesn't know about this flag may not work properly when the flag
> is set.

We could expose HWCAP2_EPAN which implies exec-only but I find it weird
(we had the precedent of HWCAP_LPAE on arm32 which meant 64-bit atomics
available). You can look at this as an architecture feature allowing
user execute-only permissions.

> Software that requires that any memory mapped without PROT_READ is
> readable would be nonportable according to POSIX, but nonportable
> doesn't mean not correct; it just means that POSIX doesn't gurarantee
> that it works everywhere.

We already made this decision when we first introduced the execute-only
permission. We've had it for a while and haven't heard of any instance
of PROT_EXEC-only mapping expecting PROT_READ. The reason we reverted
that change was that it was invalidating the PAN kernel protection. So
I'm not concerned about changing the ABI but what I'd like is to inform
the user that exec-only is available, in case it wants to do something
with it.

> So:
> 
> 1) Is true execute-only memory an ABI break that we care about, and do
> we need an explicit opt-in?

See above and commit cab15ce604e5 ("arm64: Introduce execute-only page
access permissions") from 2016.

> 2) Otherwise, is there another more suitable and less arch-specific
> mechanism that could be used?  (Maybe AT_FLAGS or similar?)

If you don't like HWCAP, we could use a bit in AT_FLAGS (they are all
currently 0). But, arguably, exec-only is a property that the hardware
offers, though indirectly. I agree you can look at this either way.

> This issue may have come up on other arches.  I've not gone digging.

I think x86 with protection keys can offer a similar mechanism but I
haven't checked.
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/hwcap.h b/arch/arm64/include/asm/hwcap.h
index 9a5498c..5ee5bce 100644
--- a/arch/arm64/include/asm/hwcap.h
+++ b/arch/arm64/include/asm/hwcap.h
@@ -105,6 +105,7 @@ 
 #define KERNEL_HWCAP_RNG		__khwcap2_feature(RNG)
 #define KERNEL_HWCAP_BTI		__khwcap2_feature(BTI)
 #define KERNEL_HWCAP_MTE		__khwcap2_feature(MTE)
+#define KERNEL_HWCAP_EXECONLY		__khwcap2_feature(EXECONLY)
 
 /*
  * This yields a mask that user programs can use to figure out what
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 19147b6..e7bc373 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -796,6 +796,7 @@ 
 
 #define ID_AA64MMFR1_VMIDBITS_8		0
 #define ID_AA64MMFR1_VMIDBITS_16	2
+#define ID_AA64MMFR1_EPAN		3
 
 /* id_aa64mmfr2 */
 #define ID_AA64MMFR2_E0PD_SHIFT		60
diff --git a/arch/arm64/include/uapi/asm/hwcap.h b/arch/arm64/include/uapi/asm/hwcap.h
index b8f41aa..61471f4 100644
--- a/arch/arm64/include/uapi/asm/hwcap.h
+++ b/arch/arm64/include/uapi/asm/hwcap.h
@@ -75,5 +75,6 @@ 
 #define HWCAP2_RNG		(1 << 16)
 #define HWCAP2_BTI		(1 << 17)
 #define HWCAP2_MTE		(1 << 18)
+#define HWCAP2_EXECONLY		(1 << 19)
 
 #endif /* _UAPI__ASM_HWCAP_H */
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 540245c..4faab5b 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2276,6 +2276,9 @@  static const struct arm64_cpu_capabilities arm64_elf_hwcaps[] = {
 #ifdef CONFIG_ARM64_MTE
 	HWCAP_CAP(SYS_ID_AA64PFR1_EL1, ID_AA64PFR1_MTE_SHIFT, FTR_UNSIGNED, ID_AA64PFR1_MTE, CAP_HWCAP, KERNEL_HWCAP_MTE),
 #endif /* CONFIG_ARM64_MTE */
+#ifdef CONFIG_ARM64_EPAN
+	HWCAP_CAP(SYS_ID_AA64MMFR1_EL1, ID_AA64MMFR1_PAN_SHIFT, FTR_UNSIGNED, ID_AA64MMFR1_EPAN, CAP_HWCAP, KERNEL_HWCAP_EXECONLY),
+#endif
 	{},
 };
 
diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
index 77605ae..34c98d9 100644
--- a/arch/arm64/kernel/cpuinfo.c
+++ b/arch/arm64/kernel/cpuinfo.c
@@ -94,6 +94,7 @@  static const char *const hwcap_str[] = {
 	[KERNEL_HWCAP_RNG]		= "rng",
 	[KERNEL_HWCAP_BTI]		= "bti",
 	[KERNEL_HWCAP_MTE]		= "mte",
+	[KERNEL_HWCAP_EXECONLY]		= "xo",
 };
 
 #ifdef CONFIG_COMPAT