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 doesnot 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<bitpos+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
Regards
PAtrick