[PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Ard Biesheuvel
ardb at kernel.org
Fri Oct 9 14:26:59 CEST 2020
On Fri, 9 Oct 2020 at 13:19, Patrick DELAUNAY <patrick.delaunay at st.com> wrote:
>
> 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.
>
Yes, thanks for the elaborate explanation. You are correct that the
ARM architecture forbids memory aliases with mismatched attributes, so
it is definitely better not to map the memory at all.
So the next question is then, why describe it in the first place? Does
OP-TEE rely on the DT description to figure out where the secure DDR
is? Does the agent that programs the firewall get this information
from DT?
The /reserved-memory node and its contents are consumed by the OS, and
whether or not u-boot should honor them too is debatable. So I think
the robust way to deal with this on your platform would be not to
describe the secure DDR at all in the DT that is exposed to u-boot and
onwards.
And of course, your issue did uncover a serious problem in the !LPAE
code that still needs to get fixed as well.
Thanks,
Ard.
More information about the U-Boot
mailing list