From mboxrd@z Thu Jan 1 00:00:00 1970 From: Etienne Carriere Date: Thu, 29 Oct 2020 11:40:36 +0100 Subject: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property In-Reply-To: <976b2b1443424f659fa85a2d11b4b507@SFHDAG2NODE3.st.com> References: <20201006163602.21687-1-patrick.delaunay@st.com> <190d019a-7e18-b4bc-9276-e14bbe4c2855@pengutronix.de> <258ba4fa-8d1e-56be-e0de-2d6c09812c13@pengutronix.de> <20201027172533.GD14816@bill-the-cat> <976b2b1443424f659fa85a2d11b4b507@SFHDAG2NODE3.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 Dear all, CC some fellow OP-TEE guys for this secure memory description topic. On Wed, 28 Oct 2020 at 11:33, Patrick DELAUNAY wrote: > > Hi, > > > From: Ard Biesheuvel > > Sent: mardi 27 octobre 2020 22:05 > > > > On Tue, 27 Oct 2020 at 18:25, Tom Rini wrote: > > > > > > On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote: > > > > Hi Ard, > > > > > > > > > From: Ard Biesheuvel > > > > > Sent: mercredi 7 octobre 2020 15:16 > > > > > > > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum > > wrote: > > > > > > > > > > > > Hello, > > > > > > > > > > > > On 10/7/20 1:23 PM, Ahmad Fatoum wrote: > > > > > > > My findings[1] back then were that U-Boot did set the eXecute > > > > > > > Never bit only on OMAP, but not for other platforms. So I > > > > > > > could imagine this being the root cause of Patrick's issues as well: > > > > > > > > > > > > Rereading my own link, my memory is a little less fuzzy: eXecute > > > > > > Never was being set, but was without effect due Manager mode > > > > > > being set in the > > > > > DACR: > > > > > > > > > > > > > The ARM Architecture Reference Manual notes[1]: > > > > > > > > When using the Short-descriptor translation table format, > > > > > > > > the XN attribute is not checked for domains marked as Manager. > > > > > > > > Therefore, the system must not include read-sensitive memory > > > > > > > > in domains marked as Manager, because the XN bit does not > > > > > > > > prevent speculative fetches from a Manager domain. > > > > > > > > > > > > > To avoid speculative access to read-sensitive memory-mapped > > > > > > > peripherals on ARMv7, we'll need U-Boot to use client domain > > > > > > > permissions, so the XN bit can function. > > > > > > > > > > > > > This issue has come up before and was fixed in de63ac278 > > > > > > > ("ARM: mmu: Set domain permissions to client access") for OMAP2 > > only. > > > > > > > It's equally applicable to all ARMv7-A platforms where caches > > > > > > > are enabled. > > > > > > > [1]: B3.7.2 - Execute-never restrictions on instruction > > > > > > > fetching > > > > > > > > > > > > Hope this helps, > > > > > > Ahmad > > > > > > > > > > > > > > > > It most definitely does, thanks a lot. > > > > > > > > > > U-boot's mmu_setup() currently sets DACR to manager for all > > > > > domains, so this is broken for all non-LPAE configurations running > > > > > on v7 CPUs (except OMAP and perhaps others that fixed it > > > > > individually). This affects all device mappings: not just secure > > > > > DRAM for OP-TEE, but any MMIO register for any peripheral that is mapped > > into the CPU's address space. > > > > > > > > > > Patrick, could you please check whether this fixes the issue as well? > > > > > > > > > > --- a/arch/arm/lib/cache-cp15.c > > > > > +++ b/arch/arm/lib/cache-cp15.c > > > > > @@ -202,9 +202,9 @@ static inline void mmu_setup(void) > > > > > asm volatile("mcr p15, 0, %0, c2, c0, 0" > > > > > : : "r" (gd->arch.tlb_addr) : "memory"); #endif > > > > > - /* Set the access control to all-supervisor */ > > > > > + /* Set the access control to client (0b01) for each of the > > > > > + 16 domains */ > > > > > asm volatile("mcr p15, 0, %0, c3, c0, 0" > > > > > - : : "r" (~0)); > > > > > + : : "r" (0x55555555)); > > > > > > > > > > arm_init_domains(); > > > > > > > > The test will take some time to be sure that solve my remaining issue because > > issue is not always reproductible. > > > > > > > > At fist chek, I wasn't sure of DACR bahavior, but I found in [1] the line : > > > > > > > > The XN attribute is not checked for domains marked as Manager. Read- > > sensitive memory must > > > > not be included in domains marked as Manager, because the XN bit does > > not prevent prefetches > > > > in these cases. > > > > > > > > So, I need to test your patch + DCACHE_OFF instead of INVALID (to > > > > map with XN the OP-TEE region) in my patchset. > > > > > > > > FYI: I found the same DACR configuration is done in: > > > > arch/arm/cpu/armv7/ls102xa/cpu.c:199 > > > > > > > > [1] > > > > https://developer.arm.com/documentation/ddi0406/b/System-Level-Archi > > > > tecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-cont > > > > rol/The-Execute-Never--XN--attribute-and-instruction-prefetching?lan > > > > g=en > > > > > > > > Patrick > > > > > > > > For information: > > > > > > > > At the beginning I wasn't sure that the current DACR configuration > > > > is an issue because in found in pseudo code of > > > > DDI0406B_arm_architecture_reference_manual_errata_markup_8_0.pdf > > > > > > > > B3.13.3 Address translation > > > > if CheckDomain(tlbrecord.domain, mva, tlbrecord.sectionnotpage, iswrite) > > then > > > > CheckPermission(tlbrecord.perms, mva, > > > > tlbrecord.sectionnotpage, iswrite, ispriv); > > > > > > > > B3.13.4 Domain checking > > > > boolean CheckDomain(bits(4) domain, bits(32) mva, boolean > > sectionnotpage, boolean iswrite) > > > > bitpos = 2*UInt(domain); > > > > case DACR of > > > > when ?00? DataAbort(mva, domain, sectionnotpage, iswrite, > > DAbort_Domain); > > > > when ?01? permissioncheck = TRUE; > > > > when ?10? UNPREDICTABLE; > > > > when ?11? permissioncheck = FALSE; > > > > return permissioncheck; > > > > > > > > B2.4.8 Access permission checking > > > > // CheckPermission() > > > > // ================= > > > > CheckPermission(Permissions perms, bits(32) mva, > > > > boolean sectionnotpage, bits(4) domain, boolean > > > > iswrite, boolean ispriv) > > > > > > > > if SCTLR.AFE == ?0? then > > > > perms.ap<0> = ?1?; > > > > case perms.ap of > > > > when ?000? abort = TRUE; > > > > when ?001? abort = !ispriv; > > > > when ?010? abort = !ispriv && iswrite; > > > > when ?011? abort = FALSE; > > > > when ?100? UNPREDICTABLE; > > > > when ?101? abort = !ispriv || iswrite; > > > > when ?110? abort = iswrite; > > > > when ?111? > > > > if MemorySystemArchitecture() == MemArch_VMSA then > > > > abort = iswrite > > > > else > > > > UNPREDICTABLE; > > > > if abort then > > > > DataAbort(mva, domain, sectionnotpage, iswrite, > > DAbort_Permission); > > > > return; > > > > > > > > => it seens only the read/write permission is checked here > > > > (perms.ap) => perms.xn is not used here > > > > > > > > access_control = DRACR[r]; > > > > perms.ap = access_control<10:8>; > > > > perms.xn = access_control<12>; > > > > > > > > with AP[2:0], bits [10:8] > > > > Access Permissions field. Indicates the read and write access permissions > > for unprivileged > > > > and privileged accesses to the memory region. > > > > > > > > But now it is clear with [1] > > > > > > So, where did everything end up here? I specifically didn't grab this > > > series as it sounded like there was concern the problem should be > > > solved via another patch. Or would that be an in-addition-to? Thanks! > > > > > > > There are three different problems: > > - ARMv7 non-LPAE uses the wrong domain settings > > - no-map non-secure memory should not be mapped by u-boot > > - secure world-only memory should not be described as memory in the device tree > > > > So I think this series definitely needs a respin at the very least. > > I gree: 3 differents issues in the thread > > - ARMv7 non-LPAE uses the wrong domain settings > > => bad DRACR configuration as raised by Ard & patch proposed by Ard in thread (but not pushed in mailing list) > => I need to test it to check if it is correct my issue : today I can't reproduce the issue, still in my TODO list > > - no-map non-secure memory should not be mapped by u-boot > > => this serie, not ready for v2021.01, I think... I will push a V2 after my tests > > - secure world-only memory should not be described as memory in the device tree > > => I let Etienne Carriere manage it for OP-TEE point of view: today the secure memory is added by OP-TEE > in U-Boot device tree and U-Boot copy this node to Kernel Device tree > > I have no a clear opinion about it >From the overall system description, it is far more convenient for platforms to describe the full physical memory through memory at ... nodes and exclude specific areas with reserved-memory nodes. Among those specific areas, using no-map for secure address ranges seems applicable since the property is also intended for that purpose (as the reference to speculative accesses in the binding description). >From my perspective, the issue discussed here seems rather more related to how U-Boot handles FDT memory description. From my understanding, fixing the Arm domain mapping description in U-Boot should address the issue, as for secure areas are concerned. Whereas should secure areas not be covered at all by FDT memory nodes, I have no real strong opinion. - There are platforms statically describing memory and reserved-memory nodes in DTS files. I guess these could update DTS files exclude secure memory from memory nodes, rather than using reserved-memory nodes. - There are platforms using runtime (actually boot time) update of the FDT: secure world (booting before the non-secure world) update memory description with knowledge of the secure ranges. Adding reserved-memory node(s) is quite straightforward. I guess replacing memory@ nodes with new nodes describing non-secure memory ranges is also feasible. - If no-map is to be used by U-Boot to not map related memory (needed for the non-secure reserved memory as discussed in this thread), why not reusing this scheme also for secure memory. Here we discussed statically assigned memory. If we consider a platform where secure world can dynamically re-assigned memory to secure/non-secure world, such areas are not secure or non-secure memory, they are just memory.... reserved to secure services eco-system. In a previous post, Ard asked: > 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? For the first question, I think the answer is that we can have a unique description for physical memory shared by both worlds. So I would say convenience as I stated above. As for the other questions, yes, DT can definitely help the secure world to configure firewalls for the non-secure allowed accesses which both secure and non-secure rely on. The secure world relies on it because non-secure memory is seen by the secure world as legitimate areas where both worlds can share buffers for communication. Best regards, Etienne > > Regards > > PAtrick