[PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Patrick DELAUNAY
patrick.delaunay at st.com
Fri Oct 9 13:18:49 CEST 2020
Hi Ard,
> From: Ard Biesheuvel <ardb at kernel.org>
> Sent: mercredi 7 octobre 2020 12:27
>
> On Tue, 6 Oct 2020 at 18:36, Patrick Delaunay <patrick.delaunay at st.com>
> wrote:
> >
> >
> > Hi,
> >
> > On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region
> > protected by a firewall. This region is reserved in device with "no-map"
> > property.
> >
> > But sometime the platform boot failed in U-boot on a Cortex A7 access
> > to this region (depending of the binary and the issue can change with
> > compiler version or with code alignment), then the firewall raise a
> > error, for example:
> >
> > E/TC:0 tzc_it_handler:19 TZC permission failure
> > E/TC:0 dump_fail_filter:420 Permission violation on filter 0
> > E/TC:0 dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged
> read,
> > AXI ID 5c0
> > E/TC:0 Panic
> >
> > After investigation, the forbidden access is a speculative request
> > performed by the Cortex A7 because all the DDR is mapped as MEMORY
> > with CACHEABLE property.
> >
> > The issue is solved only when the region reserved by OP-TEE is no more
> > mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it
> > is already done in Linux kernel.
> >
>
> Spurious peculative accesses to device regions would be a severe silicon bug, so
> I wonder what is going on here.
>
> (Apologies if we are rehashing stuff here that has already been discussed - I don't
> remember the details)
>
> Are you sure that the speculative accesses were not caused by misconfigured
> CPU or page tables, missing TLB maintenance, etc etc?
> Because it really does smell like a software issue not a hardware issue.
>
> > I think that can be a general issue for ARM architecture: the no-map
> > tag in device should be respected by U-boot, so I propose a generic
> > solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
> >
> > This serie is composed by 7 patches
> > - 1..4/7: preliminary steps to support flags in library in lmb
> > (as it is done in memblock.c in Linux)
> > - 5/7: unitary test on the added feature in lmb lib
> > - 6/7: save the no-map flags in lmb when the device tree is parsed
> > - 7/7: update the generic behavior for "no-map" region in
> > arm/lib/cache-cp15.c::dram_bank_mmu_setup()
> >
> > It is a rebase on master branch of previous RFC [2].
> >
> > I can change this last patch if this feature is note required by other
> > ARM architecture; it is a weak function so I can avoid to map the
> > region with "no-map" property in device tree only for STM32MP
> > architecture (in arch/arm/mach-stm32mp/cpu.c).
> >
> > See also [1] which handle same speculative access on armv8 for area
> > with Executable attribute.
> >
> > [1]
> > http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-
> > marek.bykowski at gmail.com/ [2]
> > http://patchwork.ozlabs.org/project/uboot/list/?series=199486&archive=
> > both&state=*
> >
> > Regards
> > Patrick
> >
> >
> > Patrick Delaunay (7):
> > lmb: Add support of flags for no-map properties
> > lmb: add lmb_is_reserved_flags
> > lmb: remove lmb_region.size
> > lmb: add lmb_dump_region() function
> > test: lmb: add test for lmb_reserve_flags
> > image-fdt: save no-map parameter of reserve-memory
> > arm: cache: cp15: don't map the reserved region with no-map property
> >
> > arch/arm/include/asm/system.h | 3 +
> > arch/arm/lib/cache-cp15.c | 19 ++++++-
> > common/image-fdt.c | 23 +++++---
> > include/lmb.h | 22 +++++++-
> > lib/lmb.c | 100 +++++++++++++++++++++++-----------
> > test/lib/lmb.c | 89 ++++++++++++++++++++++++++++++
> > 6 files changed, 212 insertions(+), 44 deletions(-)
> >
> > --
> > 2.17.1
> >
No, I don't yet described the issue in details on mailing list.
So I will explain the investigation done and my conclusion.
On STM32MP15x platform we have firewall to protect the access to DDR (TZC400 IP).
When OP-TEE is used, the boot chain is:
1/ ROM-Code (secure)
2/ TF-A (BL2) in SYSRAM (secure)
3/ OP-TEE in SYSRAM and DDR (secure)
4/ U-Boot in DDR
5/ Linux lernel
OP-TEE is loaded by TF-A BL2
- in SYRAM (for pager part)
- in DDR (for pageable part).
U-Boot is loaded by TF-A BL2 in DDR.
When OP-TEE is execute, it protects with TZC400 firewall the reserved part of DDR as defined in device tree :
For example : ./arch/arm/dts/stm32mp157a-ed1-u-boot.dtsi:42:
reserved-memory {
optee at fe000000 {
reg = <0xfe000000 0x02000000>;
no-map;
};
};
When OP-TEE is running in secure world (secure monitor is resident),
it jump to normal world (unsecure) = U-Boot
After relocation, U-Boot maps all the DDR as normal memory
(cacheable, bufferable, executable) and activate data cahe
Then, sometime (because depending of the compilation), the firewall detect an unsecure access
to OP-TEE secured region when U-Boot is running.
We have investigate this access with gdb:
- it is not an direct requet done by U-Boot code : we have no issue whe code is executed step by step
- we have no issue when CACHE (instruction and data) is deactivated
- no cache or TLB maintenance in this part of code
- just after the PC, we found in memory a array whith content decoded as branch instruction
to some address located in OP-TEE protected memory
(even if the content of the array is not instruction but u32 values)
and it is exactly this address which cause the firewall error.
PS: if I modify the code (by adding printf for example), the array change:
it content is no more see as branch instruction and the issue disappears.
My conclusion:
the A7 core "see" the branch instruction near the PC with address in DDR
and this address is marked as cacheable/executable in MMU
So the instruction cache load this code in the cache instruction (preloaded for Cortex A7 pipeline).
I found this note in ARM documentation (found in armv8 but it is the same for armV7):
https://developer.arm.com/architectures/learn-the-architecture/armv8-a-memory-model/device-memory
Note: There is a subtle distinction here that is easy to miss. Marking a region as Device prevents speculative
data accesses only. Marking a region as non-executable prevents speculative instruction accesses.
This means that, to prevent any speculative accesses, a region must be marked as both Device and non-executable.
For me to avoid any unexpected access to protected memory by firewall the OP-TEE reserved
memory must at least be configured as device memory and not executable...
But after tests, it wasn't enough.
Even if we set the OP-TEE memory with DCACHE_OFF value,
TZC400 see other issue for secure to no secure transition access
(during SMC execution for request to secure monitor).
Then I remember a other requirement in ARM architecture:
to avoid unexpected behavior the same region should be mapped not mapped as device and memory:
(OP-TEE mark the reserved region as normal memory / cacheable only accessible
by secure world / protected by TZC400)
Reference = ARMv7 Architecture reference:
A3.5.7 Memory access restrictions
Behavior is UNPREDICTABLE if the same memory location:
— is marked as Shareable Normal and Non-shareable Normal
— is marked as having different memory types (Normal, Device, or Strongly-ordered)
— is marked as having different cacheability attributes
— is marked as being Shareable Device and Non-shareable Device memory.
Such memory marking contradictions can occur, for example, by the use of aliases in a virtual to
physical address mapping
It is why I propose this patchset to un-map the OP-TEE memory, as it is indicated in device tree;
I don't see other solution to respect the ARM requirements.
So for me it is not a SOC issue or a SW bug, but an ARM architecture constraint for speculative access
to normal memory region (marked cacheable/shareable/executable).
This prevent any issue for STM32MP15x platform but also for other platforms
when some part of memory must be protected (no access allowed/protected by firewall)
BEFORE U-Boot execution.
I hope that it is more clear now.
Regards
Patrick
More information about the U-Boot
mailing list