On Thu, 29 Oct 2020 at 17:06, Etienne Carriere etienne.carriere@linaro.org wrote:
On Thu, 29 Oct 2020 at 12:26, Ard Biesheuvel ardb@kernel.org wrote:
On Thu, 29 Oct 2020 at 11:40, Etienne Carriere etienne.carriere@linaro.org wrote:
Dear all,
CC some fellow OP-TEE guys for this secure memory description topic.
On Wed, 28 Oct 2020 at 11:33, Patrick DELAUNAY patrick.delaunay@st.com wrote:
Hi,
From: Ard Biesheuvel ardb@kernel.org Sent: mardi 27 octobre 2020 22:05
On Tue, 27 Oct 2020 at 18:25, Tom Rini trini@konsulko.com wrote:
On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote: > Hi Ard, > > > From: Ard Biesheuvel ardb@kernel.org > > Sent: mercredi 7 octobre 2020 15:16 > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum a.fatoum@pengutronix.de
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 DACRbitpos+1:bitpos 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@... 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).
The point I made before was that secure and non-secure are two disjoint address spaces. The fact that TZ firewalls exist where you can move things from one side to the other does not imply that things works like this in the general case.
E.g., you could have
secure DRAM at S 0x0 non-secure DRAM at NS 0x0
where the ranges are backed by *different* memory. Since the DT description does not include the S/NS distinction, only the address range, the only thing we can assume when looking at memory@ and /reserved-memory is that everything it describes is NS.
From Arm Trustzone stand point, both secure and non-secure worlds share the very same physical address space. I your example, physical address 0x0 would refer to the same DRAM cell. Whether this cell is secure or non-secure is a configuration set in the DRAM firmwall.
How interesting! I looked this up in the ARM ARM, and the ARMv7 ARM has
(DDI0406C B1.5.1) The memory system provides mechanisms that prevent the Non-secure state accessing regions of the physical memory designated as Secure.
which implies that there is one physical address space, with some parts marked secure and some parts marked as non-secure.
However, the latest ARM ARM has
(DDI0487E D1.4) The Armv8-A architecture provides two Security states, each with an associated physical memory address space
So this means that what you are saying is correct for 32-bit ARM but not for 64-bit ARM, which is a bit disappointing.
This is why memory node(s) describe physical DRAM that is meaningful to both worlds but device configuration (firewall configuration) will make DRAM address ranges legitimate to be accessed by either secure or non-secure attribute of the memory mapping.
Yeah, so the problem is that this is no longer true. The same physical address value may be valid in both address spaces, and so we either need to quality addresses in DT as S or NS, or omit S addresses entirely.
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