From mboxrd@z Thu Jan 1 00:00:00 1970 From: Patrick DELAUNAY Date: Fri, 9 Oct 2020 11:18:49 +0000 Subject: [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property In-Reply-To: References: <20201006163602.21687-1-patrick.delaunay@st.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Ard, > From: Ard Biesheuvel > Sent: mercredi 7 octobre 2020 12:27 > > On Tue, 6 Oct 2020 at 18:36, Patrick Delaunay > 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