linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm: Handle starting up in secure mode
@ 2015-08-24 13:55 Christopher Covington
  2015-08-26 10:37 ` Ard Biesheuvel
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Christopher Covington @ 2015-08-24 13:55 UTC (permalink / raw)
  Cc: Christopher Covington, Russell King, Will Deacon, Mark Rutland,
	Nicolas Pitre, Jon Medhurst (Tixy),
	Ard Biesheuvel, Wang Nan, Nathan Lynch, Stephen Boyd,
	Yingjoe Chen, Masahiro Yamada, Gregory CLEMENT, Arnd Bergmann,
	Uwe Kleine-König, Kees Cook, Florian Fainelli,
	Maxime Coquelin stm32, Linus Walleij, Paul Bolle,
	linux-arm-kernel, linux-kernel

ARM Linux appears to have never been made aware of the ARMv7 security
extensions. When CONFIG_ARM_SEC_EXT=y, have it probe for its security
state by checking whether CNTFRQ is writeable and potentially make
mode changes based on the information. The most features are available
from hypervisor (HYP) mode, so switch to it possible. Failing that,
prefer non-secure supervisor (SVC) mode to secure supervisor mode.

Signed-off-by: Christopher Covington <cov@codeaurora.org>
---
 arch/arm/include/asm/sec.h         |  27 +++++++
 arch/arm/include/uapi/asm/ptrace.h |   1 +
 arch/arm/kernel/Makefile           |   1 +
 arch/arm/kernel/head.S             |   3 +
 arch/arm/kernel/mon-stub.S         | 158 +++++++++++++++++++++++++++++++++++++
 arch/arm/mm/Kconfig                |  20 ++++-
 6 files changed, 206 insertions(+), 4 deletions(-)
 create mode 100644 arch/arm/include/asm/sec.h
 create mode 100644 arch/arm/kernel/mon-stub.S

diff --git a/arch/arm/include/asm/sec.h b/arch/arm/include/asm/sec.h
new file mode 100644
index 0000000..4a9a573
--- /dev/null
+++ b/arch/arm/include/asm/sec.h
@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef SEC_H
+#define SEC_H
+
+#ifndef __ASSEMBLY__
+
+#ifdef CONFIG_ARM_SEC_EXT
+extern int __boot_cpu_secure;
+#else
+#define __boot_cpu_secure 0
+#endif
+
+#endif /* ! __ASSEMBLY__ */
+
+#endif /* SEC_H */
diff --git a/arch/arm/include/uapi/asm/ptrace.h b/arch/arm/include/uapi/asm/ptrace.h
index 5af0ed1..70ff6bf 100644
--- a/arch/arm/include/uapi/asm/ptrace.h
+++ b/arch/arm/include/uapi/asm/ptrace.h
@@ -53,6 +53,7 @@
 #endif
 #define FIQ_MODE	0x00000011
 #define IRQ_MODE	0x00000012
+#define MON_MODE	0x00000016
 #define ABT_MODE	0x00000017
 #define HYP_MODE	0x0000001a
 #define UND_MODE	0x0000001b
diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index e69f7a1..a60a0f7 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -87,6 +87,7 @@ head-y			:= head$(MMUEXT).o
 obj-$(CONFIG_DEBUG_LL)	+= debug.o
 obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
 
+obj-$(CONFIG_ARM_SEC_EXT)	+= mon-stub.o
 obj-$(CONFIG_ARM_VIRT_EXT)	+= hyp-stub.o
 ifeq ($(CONFIG_ARM_PSCI),y)
 obj-y				+= psci.o psci-call.o
diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
index 29e2991..d137ba4 100644
--- a/arch/arm/kernel/head.S
+++ b/arch/arm/kernel/head.S
@@ -85,6 +85,9 @@ ENTRY(stext)
  THUMB(	.thumb			)	@ switch to Thumb now.
  THUMB(1:			)
 
+#ifdef CONFIG_ARM_SEC_EXT
+	bl	__mon_stub_install
+#endif
 #ifdef CONFIG_ARM_VIRT_EXT
 	bl	__hyp_stub_install
 #endif
diff --git a/arch/arm/kernel/mon-stub.S b/arch/arm/kernel/mon-stub.S
new file mode 100644
index 0000000..ab1a361
--- /dev/null
+++ b/arch/arm/kernel/mon-stub.S
@@ -0,0 +1,158 @@
+/*
+ * Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/linkage.h>
+#include <asm/assembler.h>
+#include <asm/sec.h>
+
+.data
+ENTRY(__boot_cpu_secure)
+	.long	0
+.text
+
+/*
+ * ARM Linux has the most features available in hypervisor mode and
+ * running in non-secure mode is recommended. Thus, try to get into
+ * hypervisor mode if we're not already there, or failing that, try
+ * to get into non-secure supervisor mode.
+ */
+ENTRY(__mon_stub_install)
+	/*
+	 * Store the mode field of the CPSR in r4 and return early if we're
+	 * already in hypervisor mode.
+	 */
+	mrs	r4, cpsr
+	and	r4, r4, #MODE_MASK
+	cmp	r4, #HYP_MODE
+	reteq	lr
+
+	/*
+	 * Save the link register in a non-banked register, r5, so that we
+	 * still have access to it after mode switches.
+	 */
+	mov	r5, lr
+
+	/*
+	 * Read ID_PFR1 and store the value in r6. This register indicates
+	 * the presence of the security and virtualization extensions. The
+	 * former is interesting because we must traverse secure monitor mode
+	 * to get to hypervisor mode and it allows easy manipulation of
+	 * exception vectors via the Vector Base Address Register (VBAR).
+	 *
+	 * ID_PFR1 also indicates whether the generic timer is present, which
+	 * has a handy register for our purposes, CNTFRQ. Accesses won't trap
+	 * even with higher exception levels in AArch64 and writes will only
+	 * succeed from the highest exception level on a system (the undefined
+	 * exception from a failed write is used as a branch).
+	 */
+
+	mrc   p15, 0, r6, c0, c1, 1	@ ID_PFR1
+
+	/*
+	 * If we're in monitor mode then we know the security setting and can
+	 * begin the transition to hypervisor or non-secure supervisor mode
+	 * immediately.
+	 */
+
+	cmp	r4, #MON_MODE
+	beq	monitor
+
+	/*
+	 * Check that the security extensions and generic timer are present.
+	 */
+	and	r7, r6, #0xf0
+	cmp	r7, #0x10
+	cmpne	r7, #0x20
+	retne	lr
+	and	r8, r6, #0xf0000
+	cmp	r8, #0x10000
+	retne	lr
+
+	/*
+	 * Set things up so that if a CNTFRQ access causes an undefined
+	 * instruction exception, we return to the address indicated in r5
+	 * in the mode indicated in r4.
+	 */
+	adr	r7, __mon_stub_vectors
+	mcr	p15, 0, r7, c12, c0, 0	@ set vector base address (VBAR)
+
+	mrc	p15, 0, r7, c14, c0, 0	@ CNTFRQ
+	mcr	p15, 0, r7, c14, c0, 0	@ CNTFRQ
+
+	/*
+	 * If we got this far, switch to monitor mode and prepare for further
+	 * switching.
+	 */
+	cps	#MON_MODE
+
+monitor:
+	/*
+	 * TODO: Handle SIF and separate secure physical memory in general
+	 */
+	mrc	p15, 0, r8, c1, c1, 0	@ get secure configuration (SCR)
+	orr	r8, r8, #1		@ non-secure (NS)
+
+	mov	r8, #1
+	ldr	r7, =__boot_cpu_secure
+	str	r8, [r7]
+
+	/*
+	 * See whether the switch will be to hypervisor or supervisor.
+	 */
+	and	r7, r6, #0xf000
+	cmp	r7, #0x1000
+	mcrne	p15, 0, r8, c1, c1, 0	@ set secure configuration (SCR)
+	bne	supervise
+
+	orr	r8, r8, #0x100		@ hypercall enable (HCE)
+	mcr	p15, 0, r8, c1, c1, 0	@ set secure configuration (SCR)
+
+	mov	lr, r5
+	msr	spsr, #HYP_MODE
+	__ERET
+
+supervise:
+	/*
+	 * Switch to supervisor mode and return using a non-banked register,
+	 * in case we're not in the mode we started out in.
+	 */
+	cps	#SVC_MODE
+	ret	r5
+ENDPROC(__mon_stub_install)
+
+ENTRY(__mon_stub_do_undef)
+	/*
+	 * Return to the address at r5 with the mode in r4. If there is an
+	 * illegal or unimplemented mode in r4 the cpsr write will cause an
+	 * undefined exception, recursing. To avoid that, put the current
+	 * mode in r4 before performing the write.
+	 */
+	mrs	r6, cpsr
+	bic	r7, r6, #MODE_MASK
+	orr	r7, r7, r4
+	and	r4, r6, #MODE_MASK
+	msr	cpsr, r7
+	ret	r5
+ENDPROC(__mon_stub_do_undef)
+
+.align 5
+__mon_stub_vectors:
+__mon_stub_reset:	W(b)	.
+__mon_stub_und:		W(b)	__mon_stub_do_undef
+__mon_stub_call:	W(b)	.
+__mon_stub_pabort:	W(b)	.
+__mon_stub_dabort:	W(b)	.
+__mon_stub_trap:	W(b)	.
+__mon_stub_irq:		W(b)	.
+__mon_stub_fiq:		W(b)	.
+ENDPROC(__mon_stub_vectors)
diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
index 7c6b976..32fa451 100644
--- a/arch/arm/mm/Kconfig
+++ b/arch/arm/mm/Kconfig
@@ -676,7 +676,7 @@ config ARM_THUMBEE
 	  make use of it. Say N for code that can run on CPUs without ThumbEE.
 
 config ARM_VIRT_EXT
-	bool
+	bool "Support for Virtualization Extensions"
 	depends on MMU
 	default y if CPU_V7
 	help
@@ -684,9 +684,21 @@ config ARM_VIRT_EXT
 	  Extensions to install hypervisors without run-time firmware
 	  assistance.
 
-	  A compliant bootloader is required in order to make maximum
-	  use of this feature.  Refer to Documentation/arm/Booting for
-	  details.
+	  A compliant bootloader or enabling ARM_SEC_EXT is required in
+	  order to make maximum use of this feature. Refer to
+	  Documentation/arm/Booting for details.
+
+config ARM_SEC_EXT
+	bool "Support for Security Extensions"
+	depends on MMU
+	default n
+	help
+	  Say Y to have the kernel check for the presence of the ARM Security
+	  Extensions and where possible, use them to switch to preferred
+	  security and mode settings. This decreases the kernel's dependency
+	  on bootloaders.
+
+	  If unsure, say N.
 
 config SWP_EMULATE
 	bool "Emulate SWP/SWPB instructions" if !SMP
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project


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

* Re: [PATCH] arm: Handle starting up in secure mode
  2015-08-24 13:55 [PATCH] arm: Handle starting up in secure mode Christopher Covington
@ 2015-08-26 10:37 ` Ard Biesheuvel
  2015-08-26 10:39 ` Dave Martin
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Ard Biesheuvel @ 2015-08-26 10:37 UTC (permalink / raw)
  To: Christopher Covington
  Cc: Russell King, Will Deacon, Mark Rutland, Nicolas Pitre,
	Jon Medhurst (Tixy),
	Wang Nan, Nathan Lynch, Stephen Boyd, Yingjoe Chen,
	Masahiro Yamada, Gregory CLEMENT, Arnd Bergmann,
	Uwe Kleine-König, Kees Cook, Florian Fainelli,
	Maxime Coquelin stm32, Linus Walleij, Paul Bolle,
	linux-arm-kernel, linux-kernel

On 24 August 2015 at 15:55, Christopher Covington <cov@codeaurora.org> wrote:
> ARM Linux appears to have never been made aware of the ARMv7 security
> extensions. When CONFIG_ARM_SEC_EXT=y, have it probe for its security
> state by checking whether CNTFRQ is writeable and potentially make
> mode changes based on the information. The most features are available
> from hypervisor (HYP) mode, so switch to it possible. Failing that,
> prefer non-secure supervisor (SVC) mode to secure supervisor mode.
>
> Signed-off-by: Christopher Covington <cov@codeaurora.org>

Nice hack! One comment below ...

> ---
>  arch/arm/include/asm/sec.h         |  27 +++++++
>  arch/arm/include/uapi/asm/ptrace.h |   1 +
>  arch/arm/kernel/Makefile           |   1 +
>  arch/arm/kernel/head.S             |   3 +
>  arch/arm/kernel/mon-stub.S         | 158 +++++++++++++++++++++++++++++++++++++
>  arch/arm/mm/Kconfig                |  20 ++++-
>  6 files changed, 206 insertions(+), 4 deletions(-)
>  create mode 100644 arch/arm/include/asm/sec.h
>  create mode 100644 arch/arm/kernel/mon-stub.S
>
> diff --git a/arch/arm/include/asm/sec.h b/arch/arm/include/asm/sec.h
> new file mode 100644
> index 0000000..4a9a573
> --- /dev/null
> +++ b/arch/arm/include/asm/sec.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef SEC_H
> +#define SEC_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#ifdef CONFIG_ARM_SEC_EXT
> +extern int __boot_cpu_secure;
> +#else
> +#define __boot_cpu_secure 0
> +#endif
> +
> +#endif /* ! __ASSEMBLY__ */
> +
> +#endif /* SEC_H */
> diff --git a/arch/arm/include/uapi/asm/ptrace.h b/arch/arm/include/uapi/asm/ptrace.h
> index 5af0ed1..70ff6bf 100644
> --- a/arch/arm/include/uapi/asm/ptrace.h
> +++ b/arch/arm/include/uapi/asm/ptrace.h
> @@ -53,6 +53,7 @@
>  #endif
>  #define FIQ_MODE       0x00000011
>  #define IRQ_MODE       0x00000012
> +#define MON_MODE       0x00000016
>  #define ABT_MODE       0x00000017
>  #define HYP_MODE       0x0000001a
>  #define UND_MODE       0x0000001b
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index e69f7a1..a60a0f7 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -87,6 +87,7 @@ head-y                        := head$(MMUEXT).o
>  obj-$(CONFIG_DEBUG_LL) += debug.o
>  obj-$(CONFIG_EARLY_PRINTK)     += early_printk.o
>
> +obj-$(CONFIG_ARM_SEC_EXT)      += mon-stub.o
>  obj-$(CONFIG_ARM_VIRT_EXT)     += hyp-stub.o
>  ifeq ($(CONFIG_ARM_PSCI),y)
>  obj-y                          += psci.o psci-call.o
> diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> index 29e2991..d137ba4 100644
> --- a/arch/arm/kernel/head.S
> +++ b/arch/arm/kernel/head.S
> @@ -85,6 +85,9 @@ ENTRY(stext)
>   THUMB(        .thumb                  )       @ switch to Thumb now.
>   THUMB(1:                      )
>
> +#ifdef CONFIG_ARM_SEC_EXT
> +       bl      __mon_stub_install
> +#endif
>  #ifdef CONFIG_ARM_VIRT_EXT
>         bl      __hyp_stub_install
>  #endif
> diff --git a/arch/arm/kernel/mon-stub.S b/arch/arm/kernel/mon-stub.S
> new file mode 100644
> index 0000000..ab1a361
> --- /dev/null
> +++ b/arch/arm/kernel/mon-stub.S
> @@ -0,0 +1,158 @@
> +/*
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/linkage.h>
> +#include <asm/assembler.h>
> +#include <asm/sec.h>
> +
> +.data
> +ENTRY(__boot_cpu_secure)
> +       .long   0
> +.text
> +
> +/*
> + * ARM Linux has the most features available in hypervisor mode and
> + * running in non-secure mode is recommended. Thus, try to get into
> + * hypervisor mode if we're not already there, or failing that, try
> + * to get into non-secure supervisor mode.
> + */
> +ENTRY(__mon_stub_install)
> +       /*
> +        * Store the mode field of the CPSR in r4 and return early if we're
> +        * already in hypervisor mode.
> +        */
> +       mrs     r4, cpsr
> +       and     r4, r4, #MODE_MASK
> +       cmp     r4, #HYP_MODE
> +       reteq   lr
> +
> +       /*
> +        * Save the link register in a non-banked register, r5, so that we
> +        * still have access to it after mode switches.
> +        */
> +       mov     r5, lr
> +
> +       /*
> +        * Read ID_PFR1 and store the value in r6. This register indicates
> +        * the presence of the security and virtualization extensions. The
> +        * former is interesting because we must traverse secure monitor mode
> +        * to get to hypervisor mode and it allows easy manipulation of
> +        * exception vectors via the Vector Base Address Register (VBAR).
> +        *
> +        * ID_PFR1 also indicates whether the generic timer is present, which
> +        * has a handy register for our purposes, CNTFRQ. Accesses won't trap
> +        * even with higher exception levels in AArch64 and writes will only
> +        * succeed from the highest exception level on a system (the undefined
> +        * exception from a failed write is used as a branch).
> +        */
> +
> +       mrc   p15, 0, r6, c0, c1, 1     @ ID_PFR1
> +
> +       /*
> +        * If we're in monitor mode then we know the security setting and can
> +        * begin the transition to hypervisor or non-secure supervisor mode
> +        * immediately.
> +        */
> +
> +       cmp     r4, #MON_MODE
> +       beq     monitor
> +
> +       /*
> +        * Check that the security extensions and generic timer are present.
> +        */
> +       and     r7, r6, #0xf0
> +       cmp     r7, #0x10
> +       cmpne   r7, #0x20
> +       retne   lr
> +       and     r8, r6, #0xf0000
> +       cmp     r8, #0x10000
> +       retne   lr
> +
> +       /*
> +        * Set things up so that if a CNTFRQ access causes an undefined
> +        * instruction exception, we return to the address indicated in r5
> +        * in the mode indicated in r4.
> +        */
> +       adr     r7, __mon_stub_vectors
> +       mcr     p15, 0, r7, c12, c0, 0  @ set vector base address (VBAR)
> +
> +       mrc     p15, 0, r7, c14, c0, 0  @ CNTFRQ
> +       mcr     p15, 0, r7, c14, c0, 0  @ CNTFRQ
> +
> +       /*
> +        * If we got this far, switch to monitor mode and prepare for further
> +        * switching.
> +        */
> +       cps     #MON_MODE
> +
> +monitor:
> +       /*
> +        * TODO: Handle SIF and separate secure physical memory in general
> +        */
> +       mrc     p15, 0, r8, c1, c1, 0   @ get secure configuration (SCR)
> +       orr     r8, r8, #1              @ non-secure (NS)
> +
> +       mov     r8, #1
> +       ldr     r7, =__boot_cpu_secure
> +       str     r8, [r7]

'=__boot_cpu_secure' gives you a link time virtual address, which is
unusable at this point, since you are still running with the MMU off.

c022f99c:       e59f707c        ldr     r7, [pc, #124]  ; c022fa20
c022f9a0:       e5878000        str     r8, [r7]

and

c022fa20:       c0f31508        .word   0xc0f31508

You need to use a PC relative reference instead.

-- 
Ard.


> +
> +       /*
> +        * See whether the switch will be to hypervisor or supervisor.
> +        */
> +       and     r7, r6, #0xf000
> +       cmp     r7, #0x1000
> +       mcrne   p15, 0, r8, c1, c1, 0   @ set secure configuration (SCR)
> +       bne     supervise
> +
> +       orr     r8, r8, #0x100          @ hypercall enable (HCE)
> +       mcr     p15, 0, r8, c1, c1, 0   @ set secure configuration (SCR)
> +
> +       mov     lr, r5
> +       msr     spsr, #HYP_MODE
> +       __ERET
> +
> +supervise:
> +       /*
> +        * Switch to supervisor mode and return using a non-banked register,
> +        * in case we're not in the mode we started out in.
> +        */
> +       cps     #SVC_MODE
> +       ret     r5
> +ENDPROC(__mon_stub_install)
> +
> +ENTRY(__mon_stub_do_undef)
> +       /*
> +        * Return to the address at r5 with the mode in r4. If there is an
> +        * illegal or unimplemented mode in r4 the cpsr write will cause an
> +        * undefined exception, recursing. To avoid that, put the current
> +        * mode in r4 before performing the write.
> +        */
> +       mrs     r6, cpsr
> +       bic     r7, r6, #MODE_MASK
> +       orr     r7, r7, r4
> +       and     r4, r6, #MODE_MASK
> +       msr     cpsr, r7
> +       ret     r5
> +ENDPROC(__mon_stub_do_undef)
> +
> +.align 5
> +__mon_stub_vectors:
> +__mon_stub_reset:      W(b)    .
> +__mon_stub_und:                W(b)    __mon_stub_do_undef
> +__mon_stub_call:       W(b)    .
> +__mon_stub_pabort:     W(b)    .
> +__mon_stub_dabort:     W(b)    .
> +__mon_stub_trap:       W(b)    .
> +__mon_stub_irq:                W(b)    .
> +__mon_stub_fiq:                W(b)    .
> +ENDPROC(__mon_stub_vectors)
> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> index 7c6b976..32fa451 100644
> --- a/arch/arm/mm/Kconfig
> +++ b/arch/arm/mm/Kconfig
> @@ -676,7 +676,7 @@ config ARM_THUMBEE
>           make use of it. Say N for code that can run on CPUs without ThumbEE.
>
>  config ARM_VIRT_EXT
> -       bool
> +       bool "Support for Virtualization Extensions"
>         depends on MMU
>         default y if CPU_V7
>         help
> @@ -684,9 +684,21 @@ config ARM_VIRT_EXT
>           Extensions to install hypervisors without run-time firmware
>           assistance.
>
> -         A compliant bootloader is required in order to make maximum
> -         use of this feature.  Refer to Documentation/arm/Booting for
> -         details.
> +         A compliant bootloader or enabling ARM_SEC_EXT is required in
> +         order to make maximum use of this feature. Refer to
> +         Documentation/arm/Booting for details.
> +
> +config ARM_SEC_EXT
> +       bool "Support for Security Extensions"
> +       depends on MMU
> +       default n
> +       help
> +         Say Y to have the kernel check for the presence of the ARM Security
> +         Extensions and where possible, use them to switch to preferred
> +         security and mode settings. This decreases the kernel's dependency
> +         on bootloaders.
> +
> +         If unsure, say N.
>
>  config SWP_EMULATE
>         bool "Emulate SWP/SWPB instructions" if !SMP
> --
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>

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

* Re: [PATCH] arm: Handle starting up in secure mode
  2015-08-24 13:55 [PATCH] arm: Handle starting up in secure mode Christopher Covington
  2015-08-26 10:37 ` Ard Biesheuvel
@ 2015-08-26 10:39 ` Dave Martin
  2015-08-26 10:48   ` Russell King - ARM Linux
  2015-09-08 13:21   ` Linus Walleij
  2015-08-26 10:46 ` Russell King - ARM Linux
  2015-08-27 16:17 ` Daniel Thompson
  3 siblings, 2 replies; 9+ messages in thread
From: Dave Martin @ 2015-08-26 10:39 UTC (permalink / raw)
  To: Christopher Covington
  Cc: Mark Rutland, Jon Medhurst (Tixy),
	Stephen Boyd, Linus Walleij, Will Deacon, Florian Fainelli,
	Russell King, Nicolas Pitre, Uwe Kleine-König, Yingjoe Chen,
	Wang Nan, Kees Cook, Arnd Bergmann, Gregory CLEMENT,
	linux-arm-kernel, Paul Bolle, Ard Biesheuvel, Nathan Lynch,
	linux-kernel, Masahiro Yamada, Maxime Coquelin stm32

On Mon, Aug 24, 2015 at 09:55:26AM -0400, Christopher Covington wrote:
> ARM Linux appears to have never been made aware of the ARMv7 security
> extensions. When CONFIG_ARM_SEC_EXT=y, have it probe for its security
> state by checking whether CNTFRQ is writeable and potentially make
> mode changes based on the information. The most features are available
> from hypervisor (HYP) mode, so switch to it possible. Failing that,
> prefer non-secure supervisor (SVC) mode to secure supervisor mode.

Up to now we've steered clear of this, since it's a bit of a fig leaf
for broken firmware unless Linux actually has some valid use for the
Security Extensions itself.

Shouldn't the bootloader or firmware be doing this stuff, and if not,
why not?


Some other things that would need to be considered in any case:

 * SoC-specific setup of the Non-secure view of the system:  This has
   to happen very early, so making it DT aware is going to be hard --
   failing that, we are effectively risking bringing back board files.
   The split in responsibility between firmware/bootloader and kernel
   needs to be clearly defined and (as far as possible) platform-
   independent, otherwise we'll have total chaos.

 * Out of reset, generally the CPU state is only fully defined for the
   highest exception level.  You probably need to be doing more setup
   than you're currently doing.

 * SMP, secondary boot and suspend/resume -- again involving board-
   specific code.

 * You need to safely "park" the Secure World before running anything
   in Non-Secure.  As a minimum, you would need to quiesce any
   Secure interrupt sources, disable all interrupt traps to Monitor
   mode, and make sure that the Monitor vectors point somewhere
   real, so that executing SMC doesn't send the CPU off into the
   long grass...

Cheers
---Dave

> 
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> ---
>  arch/arm/include/asm/sec.h         |  27 +++++++
>  arch/arm/include/uapi/asm/ptrace.h |   1 +
>  arch/arm/kernel/Makefile           |   1 +
>  arch/arm/kernel/head.S             |   3 +
>  arch/arm/kernel/mon-stub.S         | 158 +++++++++++++++++++++++++++++++++++++
>  arch/arm/mm/Kconfig                |  20 ++++-
>  6 files changed, 206 insertions(+), 4 deletions(-)
>  create mode 100644 arch/arm/include/asm/sec.h
>  create mode 100644 arch/arm/kernel/mon-stub.S
> 
> diff --git a/arch/arm/include/asm/sec.h b/arch/arm/include/asm/sec.h
> new file mode 100644
> index 0000000..4a9a573
> --- /dev/null
> +++ b/arch/arm/include/asm/sec.h
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef SEC_H
> +#define SEC_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#ifdef CONFIG_ARM_SEC_EXT
> +extern int __boot_cpu_secure;
> +#else
> +#define __boot_cpu_secure 0
> +#endif
> +
> +#endif /* ! __ASSEMBLY__ */
> +
> +#endif /* SEC_H */
> diff --git a/arch/arm/include/uapi/asm/ptrace.h b/arch/arm/include/uapi/asm/ptrace.h
> index 5af0ed1..70ff6bf 100644
> --- a/arch/arm/include/uapi/asm/ptrace.h
> +++ b/arch/arm/include/uapi/asm/ptrace.h
> @@ -53,6 +53,7 @@
>  #endif
>  #define FIQ_MODE	0x00000011
>  #define IRQ_MODE	0x00000012
> +#define MON_MODE	0x00000016
>  #define ABT_MODE	0x00000017
>  #define HYP_MODE	0x0000001a
>  #define UND_MODE	0x0000001b
> diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
> index e69f7a1..a60a0f7 100644
> --- a/arch/arm/kernel/Makefile
> +++ b/arch/arm/kernel/Makefile
> @@ -87,6 +87,7 @@ head-y			:= head$(MMUEXT).o
>  obj-$(CONFIG_DEBUG_LL)	+= debug.o
>  obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
>  
> +obj-$(CONFIG_ARM_SEC_EXT)	+= mon-stub.o
>  obj-$(CONFIG_ARM_VIRT_EXT)	+= hyp-stub.o
>  ifeq ($(CONFIG_ARM_PSCI),y)
>  obj-y				+= psci.o psci-call.o
> diff --git a/arch/arm/kernel/head.S b/arch/arm/kernel/head.S
> index 29e2991..d137ba4 100644
> --- a/arch/arm/kernel/head.S
> +++ b/arch/arm/kernel/head.S
> @@ -85,6 +85,9 @@ ENTRY(stext)
>   THUMB(	.thumb			)	@ switch to Thumb now.
>   THUMB(1:			)
>  
> +#ifdef CONFIG_ARM_SEC_EXT
> +	bl	__mon_stub_install
> +#endif
>  #ifdef CONFIG_ARM_VIRT_EXT
>  	bl	__hyp_stub_install
>  #endif
> diff --git a/arch/arm/kernel/mon-stub.S b/arch/arm/kernel/mon-stub.S
> new file mode 100644
> index 0000000..ab1a361
> --- /dev/null
> +++ b/arch/arm/kernel/mon-stub.S
> @@ -0,0 +1,158 @@
> +/*
> + * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/linkage.h>
> +#include <asm/assembler.h>
> +#include <asm/sec.h>
> +
> +.data
> +ENTRY(__boot_cpu_secure)
> +	.long	0
> +.text
> +
> +/*
> + * ARM Linux has the most features available in hypervisor mode and
> + * running in non-secure mode is recommended. Thus, try to get into
> + * hypervisor mode if we're not already there, or failing that, try
> + * to get into non-secure supervisor mode.
> + */
> +ENTRY(__mon_stub_install)
> +	/*
> +	 * Store the mode field of the CPSR in r4 and return early if we're
> +	 * already in hypervisor mode.
> +	 */
> +	mrs	r4, cpsr
> +	and	r4, r4, #MODE_MASK
> +	cmp	r4, #HYP_MODE
> +	reteq	lr
> +
> +	/*
> +	 * Save the link register in a non-banked register, r5, so that we
> +	 * still have access to it after mode switches.
> +	 */
> +	mov	r5, lr
> +
> +	/*
> +	 * Read ID_PFR1 and store the value in r6. This register indicates
> +	 * the presence of the security and virtualization extensions. The
> +	 * former is interesting because we must traverse secure monitor mode
> +	 * to get to hypervisor mode and it allows easy manipulation of
> +	 * exception vectors via the Vector Base Address Register (VBAR).
> +	 *
> +	 * ID_PFR1 also indicates whether the generic timer is present, which
> +	 * has a handy register for our purposes, CNTFRQ. Accesses won't trap
> +	 * even with higher exception levels in AArch64 and writes will only
> +	 * succeed from the highest exception level on a system (the undefined
> +	 * exception from a failed write is used as a branch).
> +	 */
> +
> +	mrc   p15, 0, r6, c0, c1, 1	@ ID_PFR1
> +
> +	/*
> +	 * If we're in monitor mode then we know the security setting and can
> +	 * begin the transition to hypervisor or non-secure supervisor mode
> +	 * immediately.
> +	 */
> +
> +	cmp	r4, #MON_MODE
> +	beq	monitor
> +
> +	/*
> +	 * Check that the security extensions and generic timer are present.
> +	 */
> +	and	r7, r6, #0xf0
> +	cmp	r7, #0x10
> +	cmpne	r7, #0x20
> +	retne	lr
> +	and	r8, r6, #0xf0000
> +	cmp	r8, #0x10000
> +	retne	lr
> +
> +	/*
> +	 * Set things up so that if a CNTFRQ access causes an undefined
> +	 * instruction exception, we return to the address indicated in r5
> +	 * in the mode indicated in r4.
> +	 */
> +	adr	r7, __mon_stub_vectors
> +	mcr	p15, 0, r7, c12, c0, 0	@ set vector base address (VBAR)
> +
> +	mrc	p15, 0, r7, c14, c0, 0	@ CNTFRQ
> +	mcr	p15, 0, r7, c14, c0, 0	@ CNTFRQ
> +
> +	/*
> +	 * If we got this far, switch to monitor mode and prepare for further
> +	 * switching.
> +	 */
> +	cps	#MON_MODE
> +
> +monitor:
> +	/*
> +	 * TODO: Handle SIF and separate secure physical memory in general
> +	 */
> +	mrc	p15, 0, r8, c1, c1, 0	@ get secure configuration (SCR)
> +	orr	r8, r8, #1		@ non-secure (NS)
> +
> +	mov	r8, #1
> +	ldr	r7, =__boot_cpu_secure
> +	str	r8, [r7]
> +
> +	/*
> +	 * See whether the switch will be to hypervisor or supervisor.
> +	 */
> +	and	r7, r6, #0xf000
> +	cmp	r7, #0x1000
> +	mcrne	p15, 0, r8, c1, c1, 0	@ set secure configuration (SCR)
> +	bne	supervise
> +
> +	orr	r8, r8, #0x100		@ hypercall enable (HCE)
> +	mcr	p15, 0, r8, c1, c1, 0	@ set secure configuration (SCR)
> +
> +	mov	lr, r5
> +	msr	spsr, #HYP_MODE
> +	__ERET
> +
> +supervise:
> +	/*
> +	 * Switch to supervisor mode and return using a non-banked register,
> +	 * in case we're not in the mode we started out in.
> +	 */
> +	cps	#SVC_MODE
> +	ret	r5
> +ENDPROC(__mon_stub_install)
> +
> +ENTRY(__mon_stub_do_undef)
> +	/*
> +	 * Return to the address at r5 with the mode in r4. If there is an
> +	 * illegal or unimplemented mode in r4 the cpsr write will cause an
> +	 * undefined exception, recursing. To avoid that, put the current
> +	 * mode in r4 before performing the write.
> +	 */
> +	mrs	r6, cpsr
> +	bic	r7, r6, #MODE_MASK
> +	orr	r7, r7, r4
> +	and	r4, r6, #MODE_MASK
> +	msr	cpsr, r7
> +	ret	r5
> +ENDPROC(__mon_stub_do_undef)
> +
> +.align 5
> +__mon_stub_vectors:
> +__mon_stub_reset:	W(b)	.
> +__mon_stub_und:		W(b)	__mon_stub_do_undef
> +__mon_stub_call:	W(b)	.
> +__mon_stub_pabort:	W(b)	.
> +__mon_stub_dabort:	W(b)	.
> +__mon_stub_trap:	W(b)	.
> +__mon_stub_irq:		W(b)	.
> +__mon_stub_fiq:		W(b)	.
> +ENDPROC(__mon_stub_vectors)
> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> index 7c6b976..32fa451 100644
> --- a/arch/arm/mm/Kconfig
> +++ b/arch/arm/mm/Kconfig
> @@ -676,7 +676,7 @@ config ARM_THUMBEE
>  	  make use of it. Say N for code that can run on CPUs without ThumbEE.
>  
>  config ARM_VIRT_EXT
> -	bool
> +	bool "Support for Virtualization Extensions"
>  	depends on MMU
>  	default y if CPU_V7
>  	help
> @@ -684,9 +684,21 @@ config ARM_VIRT_EXT
>  	  Extensions to install hypervisors without run-time firmware
>  	  assistance.
>  
> -	  A compliant bootloader is required in order to make maximum
> -	  use of this feature.  Refer to Documentation/arm/Booting for
> -	  details.
> +	  A compliant bootloader or enabling ARM_SEC_EXT is required in
> +	  order to make maximum use of this feature. Refer to
> +	  Documentation/arm/Booting for details.
> +
> +config ARM_SEC_EXT
> +	bool "Support for Security Extensions"
> +	depends on MMU
> +	default n
> +	help
> +	  Say Y to have the kernel check for the presence of the ARM Security
> +	  Extensions and where possible, use them to switch to preferred
> +	  security and mode settings. This decreases the kernel's dependency
> +	  on bootloaders.
> +
> +	  If unsure, say N.
>  
>  config SWP_EMULATE
>  	bool "Emulate SWP/SWPB instructions" if !SMP
> -- 
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> 
> _______________________________________________
> 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] 9+ messages in thread

* Re: [PATCH] arm: Handle starting up in secure mode
  2015-08-24 13:55 [PATCH] arm: Handle starting up in secure mode Christopher Covington
  2015-08-26 10:37 ` Ard Biesheuvel
  2015-08-26 10:39 ` Dave Martin
@ 2015-08-26 10:46 ` Russell King - ARM Linux
  2015-08-27 16:17 ` Daniel Thompson
  3 siblings, 0 replies; 9+ messages in thread
From: Russell King - ARM Linux @ 2015-08-26 10:46 UTC (permalink / raw)
  To: Christopher Covington
  Cc: Will Deacon, Mark Rutland, Nicolas Pitre, Jon Medhurst (Tixy),
	Ard Biesheuvel, Wang Nan, Nathan Lynch, Stephen Boyd,
	Yingjoe Chen, Masahiro Yamada, Gregory CLEMENT, Arnd Bergmann,
	Uwe Kleine-König, Kees Cook, Florian Fainelli,
	Maxime Coquelin stm32, Linus Walleij, Paul Bolle,
	linux-arm-kernel, linux-kernel

On Mon, Aug 24, 2015 at 09:55:26AM -0400, Christopher Covington wrote:
> +/*
> + * ARM Linux has the most features available in hypervisor mode and
> + * running in non-secure mode is recommended. Thus, try to get into
> + * hypervisor mode if we're not already there, or failing that, try
> + * to get into non-secure supervisor mode.
> + */
> +ENTRY(__mon_stub_install)
> +	/*
> +	 * Store the mode field of the CPSR in r4 and return early if we're
> +	 * already in hypervisor mode.
> +	 */
> +	mrs	r4, cpsr
> +	and	r4, r4, #MODE_MASK
> +	cmp	r4, #HYP_MODE
> +	reteq	lr
> +
> +	/*
> +	 * Save the link register in a non-banked register, r5, so that we
> +	 * still have access to it after mode switches.
> +	 */
> +	mov	r5, lr
> +
> +	/*
> +	 * Read ID_PFR1 and store the value in r6. This register indicates
> +	 * the presence of the security and virtualization extensions. The
> +	 * former is interesting because we must traverse secure monitor mode
> +	 * to get to hypervisor mode and it allows easy manipulation of
> +	 * exception vectors via the Vector Base Address Register (VBAR).
> +	 *
> +	 * ID_PFR1 also indicates whether the generic timer is present, which
> +	 * has a handy register for our purposes, CNTFRQ. Accesses won't trap
> +	 * even with higher exception levels in AArch64 and writes will only
> +	 * succeed from the highest exception level on a system (the undefined
> +	 * exception from a failed write is used as a branch).
> +	 */
> +
> +	mrc   p15, 0, r6, c0, c1, 1	@ ID_PFR1

As this code can be built into any kernel for a CPU containing a MMU,
including all the way back to ARMv4, you had better make sure that
this code will run there without causing faults.

You should probably be checking the main ID register and ensuring that
the CPU supports the new ID scheme before trying to read CP15 registers
that may not be present in older cores.

> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> index 7c6b976..32fa451 100644
> --- a/arch/arm/mm/Kconfig
> +++ b/arch/arm/mm/Kconfig
> @@ -676,7 +676,7 @@ config ARM_THUMBEE
>  	  make use of it. Say N for code that can run on CPUs without ThumbEE.
>  
>  config ARM_VIRT_EXT
> -	bool
> +	bool "Support for Virtualization Extensions"

This change is not explained in the commit message.

>  	depends on MMU
>  	default y if CPU_V7
>  	help
> @@ -684,9 +684,21 @@ config ARM_VIRT_EXT
>  	  Extensions to install hypervisors without run-time firmware
>  	  assistance.
>  
> -	  A compliant bootloader is required in order to make maximum
> -	  use of this feature.  Refer to Documentation/arm/Booting for
> -	  details.
> +	  A compliant bootloader or enabling ARM_SEC_EXT is required in
> +	  order to make maximum use of this feature. Refer to
> +	  Documentation/arm/Booting for details.
> +
> +config ARM_SEC_EXT
> +	bool "Support for Security Extensions"
> +	depends on MMU
> +	default n

Please get rid of this redundant "default n".

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

* Re: [PATCH] arm: Handle starting up in secure mode
  2015-08-26 10:39 ` Dave Martin
@ 2015-08-26 10:48   ` Russell King - ARM Linux
  2015-08-26 14:19     ` Christopher Covington
  2015-09-08 13:21   ` Linus Walleij
  1 sibling, 1 reply; 9+ messages in thread
From: Russell King - ARM Linux @ 2015-08-26 10:48 UTC (permalink / raw)
  To: Dave Martin
  Cc: Christopher Covington, Mark Rutland, Jon Medhurst (Tixy),
	Stephen Boyd, Linus Walleij, Will Deacon, Florian Fainelli,
	Nicolas Pitre, Uwe Kleine-König, Yingjoe Chen, Wang Nan,
	Kees Cook, Arnd Bergmann, Gregory CLEMENT, linux-arm-kernel,
	Paul Bolle, Ard Biesheuvel, Nathan Lynch, linux-kernel,
	Masahiro Yamada, Maxime Coquelin stm32

On Wed, Aug 26, 2015 at 11:39:42AM +0100, Dave Martin wrote:
> On Mon, Aug 24, 2015 at 09:55:26AM -0400, Christopher Covington wrote:
> > ARM Linux appears to have never been made aware of the ARMv7 security
> > extensions. When CONFIG_ARM_SEC_EXT=y, have it probe for its security
> > state by checking whether CNTFRQ is writeable and potentially make
> > mode changes based on the information. The most features are available
> > from hypervisor (HYP) mode, so switch to it possible. Failing that,
> > prefer non-secure supervisor (SVC) mode to secure supervisor mode.
> 
> Up to now we've steered clear of this, since it's a bit of a fig leaf
> for broken firmware unless Linux actually has some valid use for the
> Security Extensions itself.
> 
> Shouldn't the bootloader or firmware be doing this stuff, and if not,
> why not?
> 
> 
> Some other things that would need to be considered in any case:
> 
>  * SoC-specific setup of the Non-secure view of the system:  This has
>    to happen very early, so making it DT aware is going to be hard --
>    failing that, we are effectively risking bringing back board files.
>    The split in responsibility between firmware/bootloader and kernel
>    needs to be clearly defined and (as far as possible) platform-
>    independent, otherwise we'll have total chaos.
> 
>  * Out of reset, generally the CPU state is only fully defined for the
>    highest exception level.  You probably need to be doing more setup
>    than you're currently doing.
> 
>  * SMP, secondary boot and suspend/resume -- again involving board-
>    specific code.
> 
>  * You need to safely "park" the Secure World before running anything
>    in Non-Secure.  As a minimum, you would need to quiesce any
>    Secure interrupt sources, disable all interrupt traps to Monitor
>    mode, and make sure that the Monitor vectors point somewhere
>    real, so that executing SMC doesn't send the CPU off into the
>    long grass...

Another question is: has this been tested with kexec?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

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

* Re: [PATCH] arm: Handle starting up in secure mode
  2015-08-26 10:48   ` Russell King - ARM Linux
@ 2015-08-26 14:19     ` Christopher Covington
  2015-08-27 15:28       ` Dave Martin
  0 siblings, 1 reply; 9+ messages in thread
From: Christopher Covington @ 2015-08-26 14:19 UTC (permalink / raw)
  To: Russell King - ARM Linux, Dave Martin
  Cc: Mark Rutland, Jon Medhurst (Tixy),
	Stephen Boyd, Linus Walleij, Will Deacon, Florian Fainelli,
	Nicolas Pitre, Uwe Kleine-König, Yingjoe Chen, Wang Nan,
	Kees Cook, Arnd Bergmann, Gregory CLEMENT, linux-arm-kernel,
	Paul Bolle, Ard Biesheuvel, Nathan Lynch, linux-kernel,
	Masahiro Yamada, Maxime Coquelin stm32

Hi,

Thank you for the feedback.

On 08/26/2015 06:48 AM, Russell King - ARM Linux wrote:
> On Wed, Aug 26, 2015 at 11:39:42AM +0100, Dave Martin wrote:
>> On Mon, Aug 24, 2015 at 09:55:26AM -0400, Christopher Covington wrote:
>>> ARM Linux appears to have never been made aware of the ARMv7 security
>>> extensions. When CONFIG_ARM_SEC_EXT=y, have it probe for its security
>>> state by checking whether CNTFRQ is writeable and potentially make
>>> mode changes based on the information. The most features are available
>>> from hypervisor (HYP) mode, so switch to it possible. Failing that,
>>> prefer non-secure supervisor (SVC) mode to secure supervisor mode.
>>
>> Up to now we've steered clear of this, since it's a bit of a fig leaf
>> for broken firmware unless Linux actually has some valid use for the
>> Security Extensions itself.
>>
>> Shouldn't the bootloader or firmware be doing this stuff, and if not,
>> why not?

In my opinion, it is sometimes useful, particularly when running on simulators
or performing bring up, to have as little dependency on firmwares and
bootloaders as possible. I want to run Linux out of reset, or get as close to
that as possible. I want an ease of use comparable to QEMU's Linux loader on
other simulators, like ARM's RTSM/AEM/FVPs, without various different,
fragmented model plugins or the overhead and noise of the semihosting
bootwrapper. I have attempted to craft the patches in such a way that the
enjoyment of sophisticated firmwares and bootloaders remains unhindered.

>> Some other things that would need to be considered in any case:
>>
>>  * SoC-specific setup of the Non-secure view of the system:  This has
>>    to happen very early, so making it DT aware is going to be hard --
>>    failing that, we are effectively risking bringing back board files.
>>    The split in responsibility between firmware/bootloader and kernel
>>    needs to be clearly defined and (as far as possible) platform-
>>    independent, otherwise we'll have total chaos.

My intention was to only perform architecturally defined initialization. All
implementation defined specifics would remain delegated to firmware, simulator
reset state hacks, or external debuggers, as the case may be. Note that using
this code does not preclude running firmware, just that if you want Linux to
do the exception level/mode transitions, you can omit those instructions from
firmware. I'll have to refresh my memory--it's been a while since I worked on
this, I apologize--but I think I used a modified bootwrapper to still perform
its usual GIC initialization.

>>  * Out of reset, generally the CPU state is only fully defined for the
>>    highest exception level.  You probably need to be doing more setup
>>    than you're currently doing.

Agreed. This is just the first step.

>>  * SMP, secondary boot and suspend/resume -- again involving board-
>>    specific code.

I've only tested single core so far, but I was thinking external debuggers and
simulators may be able to emulate PSCI in a semihosting-like manner, if
merging holding pen code is as frowned upon as I think it is (that may be an
AArch64-specific statement, but it'd probably be nice if similar facilities
were available across A32 and A64).

>>  * You need to safely "park" the Secure World before running anything
>>    in Non-Secure.  As a minimum, you would need to quiesce any
>>    Secure interrupt sources, disable all interrupt traps to Monitor
>>    mode, and make sure that the Monitor vectors point somewhere
>>    real, so that executing SMC doesn't send the CPU off into the
>>    long grass...

Thanks. I'll work towards support for these aspects in the next version.

> Another question is: has this been tested with kexec?

I have not tested it with kexec. My testing was mostly of cold boots performed
some time back on ARM's AEM/RSTM/FVP using its configurable parameters and
some simple custom firmwares to simulate entry under all the circumstances I
could think of. I grepped instruction traces to make sure the initial mode
after firmware was as intended and then that AArch32 Linux switched into the
intended mode.

EL3: not present, AArch64, AArch32
EL2: not present, AArch64, AArch32
AArch32 Linux starts in: svc_s, svc_n, hyp_n, mon_s

By my calculations, there were 36 - 17 = 19 valid combinations.

Thanks,
Christopher Covington

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

* Re: [PATCH] arm: Handle starting up in secure mode
  2015-08-26 14:19     ` Christopher Covington
@ 2015-08-27 15:28       ` Dave Martin
  0 siblings, 0 replies; 9+ messages in thread
From: Dave Martin @ 2015-08-27 15:28 UTC (permalink / raw)
  To: Christopher Covington
  Cc: Russell King - ARM Linux, Mark Rutland, Jon Medhurst (Tixy),
	Paul Bolle, Florian Fainelli, Kees Cook, Arnd Bergmann,
	Nicolas Pitre, Will Deacon, Linus Walleij, Ard Biesheuvel,
	Stephen Boyd, linux-kernel, Maxime Coquelin stm32, Nathan Lynch,
	Wang Nan, Uwe Kleine-König, Gregory CLEMENT, Yingjoe Chen,
	Masahiro Yamada, linux-arm-kernel

On Wed, Aug 26, 2015 at 10:19:39AM -0400, Christopher Covington wrote:
> Hi,
> 
> Thank you for the feedback.
> 
> On 08/26/2015 06:48 AM, Russell King - ARM Linux wrote:
> > On Wed, Aug 26, 2015 at 11:39:42AM +0100, Dave Martin wrote:
> >> On Mon, Aug 24, 2015 at 09:55:26AM -0400, Christopher Covington wrote:
> >>> ARM Linux appears to have never been made aware of the ARMv7 security
> >>> extensions. When CONFIG_ARM_SEC_EXT=y, have it probe for its security
> >>> state by checking whether CNTFRQ is writeable and potentially make
> >>> mode changes based on the information. The most features are available
> >>> from hypervisor (HYP) mode, so switch to it possible. Failing that,
> >>> prefer non-secure supervisor (SVC) mode to secure supervisor mode.
> >>
> >> Up to now we've steered clear of this, since it's a bit of a fig leaf
> >> for broken firmware unless Linux actually has some valid use for the
> >> Security Extensions itself.
> >>
> >> Shouldn't the bootloader or firmware be doing this stuff, and if not,
> >> why not?
> 
> In my opinion, it is sometimes useful, particularly when running on simulators
> or performing bring up, to have as little dependency on firmwares and
> bootloaders as possible. I want to run Linux out of reset, or get as close to

Definitely :)  That's exactly why the bootwrapper exists.

> that as possible. I want an ease of use comparable to QEMU's Linux loader on
> other simulators, like ARM's RTSM/AEM/FVPs, without various different,
> fragmented model plugins or the overhead and noise of the semihosting
> bootwrapper. I have attempted to craft the patches in such a way that the
> enjoyment of sophisticated firmwares and bootloaders remains unhindered.
> 
> >> Some other things that would need to be considered in any case:
> >>
> >>  * SoC-specific setup of the Non-secure view of the system:  This has
> >>    to happen very early, so making it DT aware is going to be hard --
> >>    failing that, we are effectively risking bringing back board files.
> >>    The split in responsibility between firmware/bootloader and kernel
> >>    needs to be clearly defined and (as far as possible) platform-
> >>    independent, otherwise we'll have total chaos.
> 
> My intention was to only perform architecturally defined initialization. All
> implementation defined specifics would remain delegated to firmware, simulator
> reset state hacks, or external debuggers, as the case may be. Note that using
> this code does not preclude running firmware, just that if you want Linux to
> do the exception level/mode transitions, you can omit those instructions from
> firmware. I'll have to refresh my memory--it's been a while since I worked on
> this, I apologize--but I think I used a modified bootwrapper to still perform
> its usual GIC initialization.

This is really just moving what is probably the smallest and simplest
piece of the initialisation problem into the kernel.  Only in trivial
cases that don't involve real hardware is this going to remove the need
for firmware.

In the general case, the S->NS switch cannot be done without platform-
specific knowledge of both the S and NS physical views of the system.
But except for switching to NS, the kernel isn't going to use the knowledge
about the S view.


It would be interesting to see what the amended Documentation/arm/Booting
would look like...

My worry is that this even if this is handy for development, it will
introduce a new kernel boot protocol that is impossible to standardise.
Nonethess, vendors in a hurry will be tempted to use it if it's there.

[...]

Cheers
---Dave

> 
> >>  * Out of reset, generally the CPU state is only fully defined for the
> >>    highest exception level.  You probably need to be doing more setup
> >>    than you're currently doing.
> 
> Agreed. This is just the first step.
> 
> >>  * SMP, secondary boot and suspend/resume -- again involving board-
> >>    specific code.
> 
> I've only tested single core so far, but I was thinking external debuggers and
> simulators may be able to emulate PSCI in a semihosting-like manner, if
> merging holding pen code is as frowned upon as I think it is (that may be an
> AArch64-specific statement, but it'd probably be nice if similar facilities
> were available across A32 and A64).
> 
> >>  * You need to safely "park" the Secure World before running anything
> >>    in Non-Secure.  As a minimum, you would need to quiesce any
> >>    Secure interrupt sources, disable all interrupt traps to Monitor
> >>    mode, and make sure that the Monitor vectors point somewhere
> >>    real, so that executing SMC doesn't send the CPU off into the
> >>    long grass...
> 
> Thanks. I'll work towards support for these aspects in the next version.
> 
> > Another question is: has this been tested with kexec?
> 
> I have not tested it with kexec. My testing was mostly of cold boots performed
> some time back on ARM's AEM/RSTM/FVP using its configurable parameters and
> some simple custom firmwares to simulate entry under all the circumstances I
> could think of. I grepped instruction traces to make sure the initial mode
> after firmware was as intended and then that AArch32 Linux switched into the
> intended mode.
> 
> EL3: not present, AArch64, AArch32
> EL2: not present, AArch64, AArch32
> AArch32 Linux starts in: svc_s, svc_n, hyp_n, mon_s
> 
> By my calculations, there were 36 - 17 = 19 valid combinations.
> 
> Thanks,
> Christopher Covington
> 
> -- 
> Qualcomm Innovation Center, Inc.
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 
> _______________________________________________
> 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] 9+ messages in thread

* Re: [PATCH] arm: Handle starting up in secure mode
  2015-08-24 13:55 [PATCH] arm: Handle starting up in secure mode Christopher Covington
                   ` (2 preceding siblings ...)
  2015-08-26 10:46 ` Russell King - ARM Linux
@ 2015-08-27 16:17 ` Daniel Thompson
  3 siblings, 0 replies; 9+ messages in thread
From: Daniel Thompson @ 2015-08-27 16:17 UTC (permalink / raw)
  To: Christopher Covington
  Cc: Russell King, Will Deacon, Mark Rutland, Nicolas Pitre,
	Jon Medhurst (Tixy),
	Ard Biesheuvel, Wang Nan, Nathan Lynch, Stephen Boyd,
	Yingjoe Chen, Masahiro Yamada, Gregory CLEMENT, Arnd Bergmann,
	Uwe Kleine-König, Kees Cook, Florian Fainelli,
	Maxime Coquelin stm32, Linus Walleij, Paul Bolle,
	linux-arm-kernel, linux-kernel

On 24/08/15 14:55, Christopher Covington wrote:
> ARM Linux appears to have never been made aware of the ARMv7 security
> extensions. When CONFIG_ARM_SEC_EXT=y, have it probe for its security
> state by checking whether CNTFRQ is writeable and potentially make
> mode changes based on the information. The most features are available
> from hypervisor (HYP) mode, so switch to it possible. Failing that,
> prefer non-secure supervisor (SVC) mode to secure supervisor mode.

If there is no hypervisor mode available what benefit do we get from 
transitioning to non-secure mode?

When running in secure mode we retain access to some potentially useful 
features such as having access to FIQ.


Daniel.


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

* Re: [PATCH] arm: Handle starting up in secure mode
  2015-08-26 10:39 ` Dave Martin
  2015-08-26 10:48   ` Russell King - ARM Linux
@ 2015-09-08 13:21   ` Linus Walleij
  1 sibling, 0 replies; 9+ messages in thread
From: Linus Walleij @ 2015-09-08 13:21 UTC (permalink / raw)
  To: Dave Martin
  Cc: Christopher Covington, Mark Rutland, Jon Medhurst (Tixy),
	Stephen Boyd, Will Deacon, Florian Fainelli, Russell King,
	Nicolas Pitre, Uwe Kleine-König, Yingjoe Chen, Wang Nan,
	Kees Cook, Arnd Bergmann, Gregory CLEMENT, linux-arm-kernel,
	Paul Bolle, Ard Biesheuvel, Nathan Lynch, linux-kernel,
	Masahiro Yamada, Maxime Coquelin stm32, Joakim Bech,
	Jens Wiklander

On Wed, Aug 26, 2015 at 12:39 PM, Dave Martin <Dave.Martin@arm.com> wrote:

> Shouldn't the bootloader or firmware be doing this stuff, and if not,
> why not?

Firmware yes, bootloader no, or maybe.

Bootloaders IMO loads in images, checksum, even public key check or
whatever, then sets up the basics and boot them.

Some hacks in U-Boot install PSCI handlers. It is neat, but a hack:
a piece of code is compiled to a special offset and copied there
by U-Boot to install the PSCI handlers. I would be fine if that software
was not compiled as part of U-Boot, but another binary loaded by it.

This would be another such "neat hack" also leaving a minimal
monitor behind IIUC.

I see the simplicity it brings to compile a load of stuff into U-Boot,
but eahhhhhhh...

Yours,
Linus Walleij

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

end of thread, other threads:[~2015-09-08 13:21 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-08-24 13:55 [PATCH] arm: Handle starting up in secure mode Christopher Covington
2015-08-26 10:37 ` Ard Biesheuvel
2015-08-26 10:39 ` Dave Martin
2015-08-26 10:48   ` Russell King - ARM Linux
2015-08-26 14:19     ` Christopher Covington
2015-08-27 15:28       ` Dave Martin
2015-09-08 13:21   ` Linus Walleij
2015-08-26 10:46 ` Russell King - ARM Linux
2015-08-27 16:17 ` Daniel Thompson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).