[U-Boot,v2,11/12] tegra124: Reserve secure RAM using MC_SECURITY_CFG{0, 1}_0

From: Ian Campbell <ijc@hellion.org.uk>

These registers can be used to prevent non-secure world from accessing a
megabyte aligned region of RAM, use them to protect the u-boot secure monitor
code.

At first I tried to do this from s_init(), however this inexplicably causes
u-boot's networking (e.g. DHCP) to fail, while networking under Linux was fine.

So instead I have added a new weak arch function protect_secure_section()
called from relocate_secure_section() and reserved the region there. This is
better overall since it defers the reservation until after the sec vs. non-sec
decision (which can be influenced by an envvar) has been made when booting the
os.

Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 arch/arm/cpu/armv7/virt-v7.c   |  5 +++++
 arch/arm/cpu/tegra-common/ap.c | 15 +++++++++++++++
 arch/arm/include/asm/system.h  |  1 +
 3 files changed, 21 insertions(+)

On Mon, Feb 16, 2015 at 12:54:48PM +0000, Jan Kiszka wrote:
> From: Ian Campbell <ijc@hellion.org.uk>
> 
> These registers can be used to prevent non-secure world from accessing a
> megabyte aligned region of RAM, use them to protect the u-boot secure monitor
> code.

What happens if the CPU tried to read this memory from the non-secure
world? If the OS has it mapped then the CPU could perform speculative
reads at any point in time.

If that can raise an abort then the OS needs to not map the region.

I take it U-Boot uses a secure mapping for the region (which I believe
should avoid the mismatched attributes issue I mentioned in my other
reply).

Thanks,
Mark.

> At first I tried to do this from s_init(), however this inexplicably causes
> u-boot's networking (e.g. DHCP) to fail, while networking under Linux was fine.
> 
> So instead I have added a new weak arch function protect_secure_section()
> called from relocate_secure_section() and reserved the region there. This is
> better overall since it defers the reservation until after the sec vs. non-sec
> decision (which can be influenced by an envvar) has been made when booting the
> os.
> 
> Signed-off-by: Ian Campbell <ijc@hellion.org.uk>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  arch/arm/cpu/armv7/virt-v7.c   |  5 +++++
>  arch/arm/cpu/tegra-common/ap.c | 15 +++++++++++++++
>  arch/arm/include/asm/system.h  |  1 +
>  3 files changed, 21 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/virt-v7.c b/arch/arm/cpu/armv7/virt-v7.c
> index b69fd37..eb6195c 100644
> --- a/arch/arm/cpu/armv7/virt-v7.c
> +++ b/arch/arm/cpu/armv7/virt-v7.c
> @@ -46,6 +46,10 @@ static unsigned long get_gicd_base_address(void)
>  #endif
>  }
>  
> +/* Define a specific version of this function to enable any available
> + * hardware protections for the reserved region */
> +void __weak protect_secure_section(void) {}
> +
>  static void relocate_secure_section(void)
>  {
>  #ifdef CONFIG_ARMV7_SECURE_BASE
> @@ -54,6 +58,7 @@ static void relocate_secure_section(void)
>  	memcpy((void *)CONFIG_ARMV7_SECURE_BASE, __secure_start, sz);
>  	flush_dcache_range(CONFIG_ARMV7_SECURE_BASE,
>  			   CONFIG_ARMV7_SECURE_BASE + sz + 1);
> +	protect_secure_section();
>  	invalidate_icache_all();
>  #endif
>  }
> diff --git a/arch/arm/cpu/tegra-common/ap.c b/arch/arm/cpu/tegra-common/ap.c
> index a17dfd1..f1d3070 100644
> --- a/arch/arm/cpu/tegra-common/ap.c
> +++ b/arch/arm/cpu/tegra-common/ap.c
> @@ -10,6 +10,7 @@
>  #include <common.h>
>  #include <asm/io.h>
>  #include <asm/arch/gp_padctrl.h>
> +#include <asm/arch/mc.h>
>  #include <asm/arch-tegra/ap.h>
>  #include <asm/arch-tegra/clock.h>
>  #include <asm/arch-tegra/fuse.h>
> @@ -154,6 +155,20 @@ static void init_pmc_scratch(void)
>  	writel(odmdata, &pmc->pmc_scratch20);
>  }
>  
> +#ifdef CONFIG_ARMV7_SECURE_RESERVE_SIZE
> +void protect_secure_section(void)
> +{
> +	struct mc_ctlr *mc = (struct mc_ctlr *)NV_PA_MC_BASE;
> +
> +	/* Must be MB aligned */
> +	BUILD_BUG_ON(CONFIG_ARMV7_SECURE_BASE & 0xFFFFF);
> +	BUILD_BUG_ON(CONFIG_ARMV7_SECURE_RESERVE_SIZE & 0xFFFFF);
> +
> +	writel(CONFIG_ARMV7_SECURE_BASE, &mc->mc_security_cfg0);
> +	writel(CONFIG_ARMV7_SECURE_RESERVE_SIZE>>20, &mc->mc_security_cfg1);
> +}
> +#endif
> +
>  void s_init(void)
>  {
>  	/* Init PMC scratch memory */
> diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
> index 89f2294..21be69d 100644
> --- a/arch/arm/include/asm/system.h
> +++ b/arch/arm/include/asm/system.h
> @@ -76,6 +76,7 @@ void armv8_switch_to_el1(void);
>  void gic_init(void);
>  void gic_send_sgi(unsigned long sgino);
>  void wait_for_wakeup(void);
> +void protect_secure_region(void);
>  void smp_kick_all_cpus(void);
>  
>  void flush_l3_cache(void);
> -- 
> 2.1.4
> 
>

On 2015-02-16 14:49, Mark Rutland wrote:
> On Mon, Feb 16, 2015 at 12:54:48PM +0000, Jan Kiszka wrote:
>> From: Ian Campbell <ijc@hellion.org.uk>
>>
>> These registers can be used to prevent non-secure world from accessing a
>> megabyte aligned region of RAM, use them to protect the u-boot secure monitor
>> code.
> 
> What happens if the CPU tried to read this memory from the non-secure
> world? If the OS has it mapped then the CPU could perform speculative
> reads at any point in time.
> 
> If that can raise an abort then the OS needs to not map the region.
> 
> I take it U-Boot uses a secure mapping for the region (which I believe
> should avoid the mismatched attributes issue I mentioned in my other
> reply).

What I can contribute to this are kernel messages due to a
misconfiguration of our hypervisor Jailhouse (while Linux was still
trying to boot it):

[   61.896860] tegra-mc 70019000.memory-controller: mpcorew: write @0x00000000fff00040: Security violation (TrustZone violation)
[   61.896888] tegra-mc 70019000.memory-controller: mpcorew: write @0x00000000fff2d340: Security violation (TrustZone violation)

So it seems that Linux is receiving a violation report here when trying
to access the memory.

Jan

On 02/16/2015 05:54 AM, Jan Kiszka wrote:
> From: Ian Campbell <ijc@hellion.org.uk>
>
> These registers can be used to prevent non-secure world from accessing a
> megabyte aligned region of RAM, use them to protect the u-boot secure monitor
> code.
>
> At first I tried to do this from s_init(), however this inexplicably causes
> u-boot's networking (e.g. DHCP) to fail, while networking under Linux was fine.
>
> So instead I have added a new weak arch function protect_secure_section()
> called from relocate_secure_section() and reserved the region there. This is
> better overall since it defers the reservation until after the sec vs. non-sec
> decision (which can be influenced by an envvar) has been made when booting the
> os.

> diff --git a/arch/arm/cpu/tegra-common/ap.c b/arch/arm/cpu/tegra-common/ap.c

> +void protect_secure_section(void)

> +	writel(CONFIG_ARMV7_SECURE_BASE, &mc->mc_security_cfg0);
> +	writel(CONFIG_ARMV7_SECURE_RESERVE_SIZE>>20, &mc->mc_security_cfg1);

Spaces around the >> ?

On 2015-02-17 22:06, Stephen Warren wrote:
> On 02/16/2015 05:54 AM, Jan Kiszka wrote:
>> From: Ian Campbell <ijc@hellion.org.uk>
>>
>> These registers can be used to prevent non-secure world from accessing a
>> megabyte aligned region of RAM, use them to protect the u-boot secure
>> monitor
>> code.
>>
>> At first I tried to do this from s_init(), however this inexplicably
>> causes
>> u-boot's networking (e.g. DHCP) to fail, while networking under Linux
>> was fine.
>>
>> So instead I have added a new weak arch function protect_secure_section()
>> called from relocate_secure_section() and reserved the region there.
>> This is
>> better overall since it defers the reservation until after the sec vs.
>> non-sec
>> decision (which can be influenced by an envvar) has been made when
>> booting the
>> os.
> 
>> diff --git a/arch/arm/cpu/tegra-common/ap.c
>> b/arch/arm/cpu/tegra-common/ap.c
> 
>> +void protect_secure_section(void)
> 
>> +    writel(CONFIG_ARMV7_SECURE_BASE, &mc->mc_security_cfg0);
>> +    writel(CONFIG_ARMV7_SECURE_RESERVE_SIZE>>20, &mc->mc_security_cfg1);
> 
> Spaces around the >> ?

Fixed for v3.

Thanks,
Jan