All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] arm64: Support Enhanced PAN
@ 2020-11-19 13:39 Vladimir Murzin
  2020-11-19 13:39 ` [PATCH 1/2] arm64: Support execute-only permissions with " Vladimir Murzin
  2020-11-19 13:39 ` [PATCH 2/2] arm64: Introduce HWCAPS2_EXECONLY Vladimir Murzin
  0 siblings, 2 replies; 10+ messages in thread
From: Vladimir Murzin @ 2020-11-19 13:39 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: catalin.marinas, keescook

Hi

ARM architecture gains support of Enhanced Privileged Access Never
(EPAN) which allows Privileged Access Never to be used with
Execute-only mappings.

As a consequence 24cecc377463 ("arm64: Revert support for execute-only
user mappings") can be revisited and re-enabled.

Changelog:

	RFC -> v1
	    - removed cap check in pte_valid_not_user (per Catalin)
	    - local_flush_tlb_all() in cpu_enable_epan() (per Catalin)
	    - reordered with CnP (per Catalin)
	    - s/HWCAP2_EPAN/HWCAP2_EXECONLY/ (per Catalin)

Thanks!

Vladimir Murzin (2):
  arm64: Support execute-only permissions with Enhanced PAN
  arm64: Introduce HWCAPS2_EXECONLY

 arch/arm64/Kconfig                    | 17 +++++++++++++++++
 arch/arm64/include/asm/cpucaps.h      |  5 +++--
 arch/arm64/include/asm/hwcap.h        |  1 +
 arch/arm64/include/asm/pgtable-prot.h |  5 +++--
 arch/arm64/include/asm/pgtable.h      | 14 +++++++++++++-
 arch/arm64/include/asm/sysreg.h       |  2 ++
 arch/arm64/include/uapi/asm/hwcap.h   |  1 +
 arch/arm64/kernel/cpufeature.c        | 24 ++++++++++++++++++++++++
 arch/arm64/kernel/cpuinfo.c           |  1 +
 arch/arm64/mm/fault.c                 |  3 +++
 10 files changed, 68 insertions(+), 5 deletions(-)

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] arm64: Support execute-only permissions with Enhanced PAN
  2020-11-19 13:39 [PATCH 0/2] arm64: Support Enhanced PAN Vladimir Murzin
@ 2020-11-19 13:39 ` Vladimir Murzin
  2020-11-19 18:22   ` Catalin Marinas
                     ` (2 more replies)
  2020-11-19 13:39 ` [PATCH 2/2] arm64: Introduce HWCAPS2_EXECONLY Vladimir Murzin
  1 sibling, 3 replies; 10+ messages in thread
From: Vladimir Murzin @ 2020-11-19 13:39 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: catalin.marinas, keescook

Enhanced Privileged Access Never (EPAN) allows Privileged Access Never
to be used with Execute-only mappings.

Absence of such support was a reason for 24cecc377463 ("arm64: Revert
support for execute-only user mappings"). Thus now it can be revisited
and re-enabled.

Cc: Kees Cook <keescook@chromium.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
---
 arch/arm64/Kconfig                    | 17 +++++++++++++++++
 arch/arm64/include/asm/cpucaps.h      |  5 +++--
 arch/arm64/include/asm/pgtable-prot.h |  5 +++--
 arch/arm64/include/asm/pgtable.h      | 14 +++++++++++++-
 arch/arm64/include/asm/sysreg.h       |  1 +
 arch/arm64/kernel/cpufeature.c        | 21 +++++++++++++++++++++
 arch/arm64/mm/fault.c                 |  3 +++
 7 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1515f6f..6639244 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1056,6 +1056,9 @@ config ARCH_WANT_HUGE_PMD_SHARE
 config ARCH_HAS_CACHE_LINE_SIZE
 	def_bool y
 
+config ARCH_HAS_FILTER_PGPROT
+	def_bool y
+
 config ARCH_ENABLE_SPLIT_PMD_PTLOCK
 	def_bool y if PGTABLE_LEVELS > 2
 
@@ -1688,6 +1691,20 @@ config ARM64_MTE
 
 endmenu
 
+menu "ARMv8.7 architectural features"
+
+config ARM64_EPAN
+	bool "Enable support for Enhanced Privileged Access Never (EPAN)"
+	default y
+	depends on ARM64_PAN
+	help
+	 Enhanced Privileged Access Never (EPAN) allows Privileged
+	 Access Never to be used with Execute-only mappings.
+
+	 The feature is detected at runtime, and will remain disabled
+	 if the cpu does not implement the feature.
+endmenu
+
 config ARM64_SVE
 	bool "ARM Scalable Vector Extension support"
 	default y
diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index e7d9899..3ea4fbdf 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -22,7 +22,7 @@
 #define ARM64_WORKAROUND_CAVIUM_27456		12
 #define ARM64_HAS_32BIT_EL0			13
 #define ARM64_HARDEN_EL2_VECTORS		14
-#define ARM64_HAS_CNP				15
+#define ARM64_HAS_EPAN				15
 #define ARM64_HAS_NO_FPSIMD			16
 #define ARM64_WORKAROUND_REPEAT_TLBI		17
 #define ARM64_WORKAROUND_QCOM_FALKOR_E1003	18
@@ -66,7 +66,8 @@
 #define ARM64_HAS_TLB_RANGE			56
 #define ARM64_MTE				57
 #define ARM64_WORKAROUND_1508412		58
+#define ARM64_HAS_CNP				59
 
-#define ARM64_NCAPS				59
+#define ARM64_NCAPS				60
 
 #endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index 046be78..f91c2aa 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -88,12 +88,13 @@ extern bool arm64_use_ng_mappings;
 #define PAGE_SHARED_EXEC	__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_WRITE)
 #define PAGE_READONLY		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_RDONLY | PTE_NG | PTE_PXN | PTE_UXN)
 #define PAGE_READONLY_EXEC	__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_RDONLY | PTE_NG | PTE_PXN)
+#define PAGE_EXECONLY		__pgprot(_PAGE_DEFAULT | PTE_RDONLY | PTE_NG | PTE_PXN)
 
 #define __P000  PAGE_NONE
 #define __P001  PAGE_READONLY
 #define __P010  PAGE_READONLY
 #define __P011  PAGE_READONLY
-#define __P100  PAGE_READONLY_EXEC
+#define __P100  PAGE_EXECONLY
 #define __P101  PAGE_READONLY_EXEC
 #define __P110  PAGE_READONLY_EXEC
 #define __P111  PAGE_READONLY_EXEC
@@ -102,7 +103,7 @@ extern bool arm64_use_ng_mappings;
 #define __S001  PAGE_READONLY
 #define __S010  PAGE_SHARED
 #define __S011  PAGE_SHARED
-#define __S100  PAGE_READONLY_EXEC
+#define __S100  PAGE_EXECONLY
 #define __S101  PAGE_READONLY_EXEC
 #define __S110  PAGE_SHARED_EXEC
 #define __S111  PAGE_SHARED_EXEC
diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 4ff12a7..e4ab9e0 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -114,7 +114,7 @@ extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
 
 #define pte_valid(pte)		(!!(pte_val(pte) & PTE_VALID))
 #define pte_valid_not_user(pte) \
-	((pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID)
+	((pte_val(pte) & (PTE_VALID | PTE_USER | PTE_UXN)) == (PTE_VALID | PTE_UXN))
 #define pte_valid_young(pte) \
 	((pte_val(pte) & (PTE_VALID | PTE_AF)) == (PTE_VALID | PTE_AF))
 #define pte_valid_user(pte) \
@@ -974,6 +974,18 @@ static inline bool arch_faults_on_old_pte(void)
 }
 #define arch_faults_on_old_pte arch_faults_on_old_pte
 
+static inline pgprot_t arch_filter_pgprot(pgprot_t prot)
+{
+	if (cpus_have_const_cap(ARM64_HAS_EPAN))
+		return prot;
+
+	if (pgprot_val(prot) != pgprot_val(PAGE_EXECONLY))
+		return prot;
+
+	return PAGE_READONLY_EXEC;
+}
+
+
 #endif /* !__ASSEMBLY__ */
 
 #endif /* __ASM_PGTABLE_H */
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 174817b..19147b6 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -579,6 +579,7 @@
 #endif
 
 /* SCTLR_EL1 specific flags. */
+#define SCTLR_EL1_EPAN		(BIT(57))
 #define SCTLR_EL1_ATA0		(BIT(42))
 
 #define SCTLR_EL1_TCF0_SHIFT	38
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index dcc165b..540245c 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1602,6 +1602,14 @@ static void cpu_enable_pan(const struct arm64_cpu_capabilities *__unused)
 }
 #endif /* CONFIG_ARM64_PAN */
 
+#ifdef CONFIG_ARM64_EPAN
+static void cpu_enable_epan(const struct arm64_cpu_capabilities *__unused)
+{
+	sysreg_clear_set(sctlr_el1, 0, SCTLR_EL1_EPAN);
+	local_flush_tlb_all();
+}
+#endif /* CONFIG_ARM64_EPAN */
+
 #ifdef CONFIG_ARM64_RAS_EXTN
 static void cpu_clear_disr(const struct arm64_cpu_capabilities *__unused)
 {
@@ -1750,6 +1758,19 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 		.cpu_enable = cpu_enable_pan,
 	},
 #endif /* CONFIG_ARM64_PAN */
+#ifdef CONFIG_ARM64_EPAN
+	{
+		.desc = "Enhanced Privileged Access Never",
+		.capability = ARM64_HAS_EPAN,
+		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+		.matches = has_cpuid_feature,
+		.sys_reg = SYS_ID_AA64MMFR1_EL1,
+		.field_pos = ID_AA64MMFR1_PAN_SHIFT,
+		.sign = FTR_UNSIGNED,
+		.min_field_value = 3,
+		.cpu_enable = cpu_enable_epan,
+	},
+#endif /* CONFIG_ARM64_EPAN */
 #ifdef CONFIG_ARM64_LSE_ATOMICS
 	{
 		.desc = "LSE atomic instructions",
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 1ee9400..b93222e 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -467,6 +467,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 	if (faulthandler_disabled() || !mm)
 		goto no_context;
 
+	if (cpus_have_const_cap(ARM64_HAS_EPAN))
+		vm_flags &= ~VM_EXEC;
+
 	if (user_mode(regs))
 		mm_flags |= FAULT_FLAG_USER;
 
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] arm64: Introduce HWCAPS2_EXECONLY
  2020-11-19 13:39 [PATCH 0/2] arm64: Support Enhanced PAN Vladimir Murzin
  2020-11-19 13:39 ` [PATCH 1/2] arm64: Support execute-only permissions with " Vladimir Murzin
@ 2020-11-19 13:39 ` Vladimir Murzin
  2020-12-08 16:36   ` Dave Martin
  1 sibling, 1 reply; 10+ messages in thread
From: Vladimir Murzin @ 2020-11-19 13:39 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: catalin.marinas, keescook

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)
 
 /*
  * 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
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: Support execute-only permissions with Enhanced PAN
  2020-11-19 13:39 ` [PATCH 1/2] arm64: Support execute-only permissions with " Vladimir Murzin
@ 2020-11-19 18:22   ` Catalin Marinas
  2020-11-19 18:52   ` Dave Martin
  2020-12-02 18:23   ` Catalin Marinas
  2 siblings, 0 replies; 10+ messages in thread
From: Catalin Marinas @ 2020-11-19 18:22 UTC (permalink / raw)
  To: Vladimir Murzin; +Cc: keescook, linux-arm-kernel

On Thu, Nov 19, 2020 at 01:39:52PM +0000, Vladimir Murzin wrote:
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 4ff12a7..e4ab9e0 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -114,7 +114,7 @@ extern unsigned long empty_zero_page[PAGE_SIZE / sizeof(unsigned long)];
>  
>  #define pte_valid(pte)		(!!(pte_val(pte) & PTE_VALID))
>  #define pte_valid_not_user(pte) \
> -	((pte_val(pte) & (PTE_VALID | PTE_USER)) == PTE_VALID)
> +	((pte_val(pte) & (PTE_VALID | PTE_USER | PTE_UXN)) == (PTE_VALID | PTE_UXN))
>  #define pte_valid_young(pte) \
>  	((pte_val(pte) & (PTE_VALID | PTE_AF)) == (PTE_VALID | PTE_AF))
>  #define pte_valid_user(pte) \

I was wondering if pte_valid_user() needs changing as well. It currently
checks for PTE_VALID | PTE_USER. In theory, a !PTE_UXN is also
user-accessible but it's only used in gup_pte_range() via
pte_access_permitted(). If "access" here means only read/write, we
should be ok. Still parsing this code.

Otherwise the patch is fine.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: Support execute-only permissions with Enhanced PAN
  2020-11-19 13:39 ` [PATCH 1/2] arm64: Support execute-only permissions with " Vladimir Murzin
  2020-11-19 18:22   ` Catalin Marinas
@ 2020-11-19 18:52   ` Dave Martin
  2020-11-27 18:31     ` Catalin Marinas
  2020-12-02 18:23   ` Catalin Marinas
  2 siblings, 1 reply; 10+ messages in thread
From: Dave Martin @ 2020-11-19 18:52 UTC (permalink / raw)
  To: Vladimir Murzin; +Cc: catalin.marinas, keescook, linux-arm-kernel

On Thu, Nov 19, 2020 at 01:39:52PM +0000, Vladimir Murzin wrote:
> Enhanced Privileged Access Never (EPAN) allows Privileged Access Never
> to be used with Execute-only mappings.
> 
> Absence of such support was a reason for 24cecc377463 ("arm64: Revert
> support for execute-only user mappings"). Thus now it can be revisited
> and re-enabled.
> 
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> ---
>  arch/arm64/Kconfig                    | 17 +++++++++++++++++
>  arch/arm64/include/asm/cpucaps.h      |  5 +++--
>  arch/arm64/include/asm/pgtable-prot.h |  5 +++--
>  arch/arm64/include/asm/pgtable.h      | 14 +++++++++++++-
>  arch/arm64/include/asm/sysreg.h       |  1 +
>  arch/arm64/kernel/cpufeature.c        | 21 +++++++++++++++++++++
>  arch/arm64/mm/fault.c                 |  3 +++
>  7 files changed, 61 insertions(+), 5 deletions(-)
> 

[...]

> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 1ee9400..b93222e 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -467,6 +467,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
>  	if (faulthandler_disabled() || !mm)
>  		goto no_context;
>  
> +	if (cpus_have_const_cap(ARM64_HAS_EPAN))
> +		vm_flags &= ~VM_EXEC;
> +

IIUC, this would be telling __do_page_fault() that the access would have
succeeded with any kind of permissions except for write access, which
doesn't seem right.

Also, isn't vm_flags just overwritten by the code after the hunk?

The logic in __do_page_fault() looks like might not have been written
with the assumption that there might be more than a single set bit in
vm_flags.


But I'm not familiar with this code, and might be totally
misunderstanding what's going on here...

>  	if (user_mode(regs))
>  		mm_flags |= FAULT_FLAG_USER;

[...]

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: Support execute-only permissions with Enhanced PAN
  2020-11-19 18:52   ` Dave Martin
@ 2020-11-27 18:31     ` Catalin Marinas
  0 siblings, 0 replies; 10+ messages in thread
From: Catalin Marinas @ 2020-11-27 18:31 UTC (permalink / raw)
  To: Dave Martin; +Cc: Vladimir Murzin, keescook, linux-arm-kernel

On Thu, Nov 19, 2020 at 06:52:05PM +0000, Dave P Martin wrote:
> On Thu, Nov 19, 2020 at 01:39:52PM +0000, Vladimir Murzin wrote:
> > Enhanced Privileged Access Never (EPAN) allows Privileged Access Never
> > to be used with Execute-only mappings.
> > 
> > Absence of such support was a reason for 24cecc377463 ("arm64: Revert
> > support for execute-only user mappings"). Thus now it can be revisited
> > and re-enabled.
> > 
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Signed-off-by: Vladimir Murzin <vladimir.murzin@arm.com>
> > ---
> >  arch/arm64/Kconfig                    | 17 +++++++++++++++++
> >  arch/arm64/include/asm/cpucaps.h      |  5 +++--
> >  arch/arm64/include/asm/pgtable-prot.h |  5 +++--
> >  arch/arm64/include/asm/pgtable.h      | 14 +++++++++++++-
> >  arch/arm64/include/asm/sysreg.h       |  1 +
> >  arch/arm64/kernel/cpufeature.c        | 21 +++++++++++++++++++++
> >  arch/arm64/mm/fault.c                 |  3 +++
> >  7 files changed, 61 insertions(+), 5 deletions(-)
> > 
> 
> [...]
> 
> > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > index 1ee9400..b93222e 100644
> > --- a/arch/arm64/mm/fault.c
> > +++ b/arch/arm64/mm/fault.c
> > @@ -467,6 +467,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
> >  	if (faulthandler_disabled() || !mm)
> >  		goto no_context;
> >  
> > +	if (cpus_have_const_cap(ARM64_HAS_EPAN))
> > +		vm_flags &= ~VM_EXEC;
> > +
> 
> IIUC, this would be telling __do_page_fault() that the access would have
> succeeded with any kind of permissions except for write access, which
> doesn't seem right.

I always have trouble remembering what the vm_flags does. So
__do_page_fault() checks vma->vm_flags & vm_flags and returns an error
if the intersection is empty. We start with all rwx permission but
modify it further down in the in do_page_fault(): if it was an exec
fault, we set vm_flags to VM_EXEC only as that's what we want to check
against vma->vm_flags; similarly, if it was a write fault, we want to
check VM_WRITE only. If it's neither exec nor a write fault (i.e. a
read), we leave it as rwx since both write and exec (prior to EPAN)
imply read.

With the EPAN patches, exec no longer implies read, so if it's neither
an exec nor a write fault, we want vm_flags to be VM_READ|VM_WRITE since
only write now implies read.

> Also, isn't vm_flags just overwritten by the code after the hunk?
> 
> The logic in __do_page_fault() looks like might not have been written
> with the assumption that there might be more than a single set bit in
> vm_flags.

I think it was, it's checking the intersection. We could do with some
comments in this code, otherwise next time someone asks I'll spend
another 30 min reading the code ;).

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: Support execute-only permissions with Enhanced PAN
  2020-11-19 13:39 ` [PATCH 1/2] arm64: Support execute-only permissions with " Vladimir Murzin
  2020-11-19 18:22   ` Catalin Marinas
  2020-11-19 18:52   ` Dave Martin
@ 2020-12-02 18:23   ` Catalin Marinas
  2020-12-08 11:41     ` Vladimir Murzin
  2 siblings, 1 reply; 10+ messages in thread
From: Catalin Marinas @ 2020-12-02 18:23 UTC (permalink / raw)
  To: Vladimir Murzin; +Cc: keescook, linux-arm-kernel

On Thu, Nov 19, 2020 at 01:39:52PM +0000, Vladimir Murzin wrote:
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 174817b..19147b6 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -579,6 +579,7 @@
>  #endif
>  
>  /* SCTLR_EL1 specific flags. */
> +#define SCTLR_EL1_EPAN		(BIT(57))
>  #define SCTLR_EL1_ATA0		(BIT(42))
>  
>  #define SCTLR_EL1_TCF0_SHIFT	38
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index dcc165b..540245c 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1602,6 +1602,14 @@ static void cpu_enable_pan(const struct arm64_cpu_capabilities *__unused)
>  }
>  #endif /* CONFIG_ARM64_PAN */
>  
> +#ifdef CONFIG_ARM64_EPAN
> +static void cpu_enable_epan(const struct arm64_cpu_capabilities *__unused)
> +{
> +	sysreg_clear_set(sctlr_el1, 0, SCTLR_EL1_EPAN);
> +	local_flush_tlb_all();
> +}
> +#endif /* CONFIG_ARM64_EPAN */

Thinking about this, can we not set the SCTLR_EL1.EPAN bit in proc.S
directly, regardless of whether the system supports it or not (it should
be write-ignored)? It would go in INIT_SCTLR_EL1_MMU_ON. We use the
cpufeature entry only for detection, not enabling.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] arm64: Support execute-only permissions with Enhanced PAN
  2020-12-02 18:23   ` Catalin Marinas
@ 2020-12-08 11:41     ` Vladimir Murzin
  0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Murzin @ 2020-12-08 11:41 UTC (permalink / raw)
  To: Catalin Marinas; +Cc: keescook, linux-arm-kernel

On 12/2/20 6:23 PM, Catalin Marinas wrote:
> On Thu, Nov 19, 2020 at 01:39:52PM +0000, Vladimir Murzin wrote:
>> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
>> index 174817b..19147b6 100644
>> --- a/arch/arm64/include/asm/sysreg.h
>> +++ b/arch/arm64/include/asm/sysreg.h
>> @@ -579,6 +579,7 @@
>>  #endif
>>  
>>  /* SCTLR_EL1 specific flags. */
>> +#define SCTLR_EL1_EPAN		(BIT(57))
>>  #define SCTLR_EL1_ATA0		(BIT(42))
>>  
>>  #define SCTLR_EL1_TCF0_SHIFT	38
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index dcc165b..540245c 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -1602,6 +1602,14 @@ static void cpu_enable_pan(const struct arm64_cpu_capabilities *__unused)
>>  }
>>  #endif /* CONFIG_ARM64_PAN */
>>  
>> +#ifdef CONFIG_ARM64_EPAN
>> +static void cpu_enable_epan(const struct arm64_cpu_capabilities *__unused)
>> +{
>> +	sysreg_clear_set(sctlr_el1, 0, SCTLR_EL1_EPAN);
>> +	local_flush_tlb_all();
>> +}
>> +#endif /* CONFIG_ARM64_EPAN */
> 
> Thinking about this, can we not set the SCTLR_EL1.EPAN bit in proc.S
> directly, regardless of whether the system supports it or not (it should
> be write-ignored)? It would go in INIT_SCTLR_EL1_MMU_ON. We use the
> cpufeature entry only for detection, not enabling.
> 

I'll try to restructure that way.

Cheers
Vladimir

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] arm64: Introduce HWCAPS2_EXECONLY
  2020-11-19 13:39 ` [PATCH 2/2] arm64: Introduce HWCAPS2_EXECONLY Vladimir Murzin
@ 2020-12-08 16:36   ` Dave Martin
  2020-12-08 17:34     ` Catalin Marinas
  0 siblings, 1 reply; 10+ messages in thread
From: Dave Martin @ 2020-12-08 16:36 UTC (permalink / raw)
  To: Vladimir Murzin; +Cc: catalin.marinas, keescook, linux-arm-kernel

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

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] arm64: Introduce HWCAPS2_EXECONLY
  2020-12-08 16:36   ` Dave Martin
@ 2020-12-08 17:34     ` Catalin Marinas
  0 siblings, 0 replies; 10+ messages in thread
From: Catalin Marinas @ 2020-12-08 17:34 UTC (permalink / raw)
  To: Dave Martin; +Cc: Vladimir Murzin, keescook, linux-arm-kernel

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.

-- 
Catalin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-12-08 17:35 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-19 13:39 [PATCH 0/2] arm64: Support Enhanced PAN Vladimir Murzin
2020-11-19 13:39 ` [PATCH 1/2] arm64: Support execute-only permissions with " Vladimir Murzin
2020-11-19 18:22   ` Catalin Marinas
2020-11-19 18:52   ` Dave Martin
2020-11-27 18:31     ` Catalin Marinas
2020-12-02 18:23   ` Catalin Marinas
2020-12-08 11:41     ` Vladimir Murzin
2020-11-19 13:39 ` [PATCH 2/2] arm64: Introduce HWCAPS2_EXECONLY Vladimir Murzin
2020-12-08 16:36   ` Dave Martin
2020-12-08 17:34     ` Catalin Marinas

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.