Hi Ard,
> From: Ard Biesheuvel <a...@kernel.org>
> Sent: mercredi 7 octobre 2020 15:16
>
> On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fat...@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-Architecture/Virtual-Memory-System-Architecture--VMSA-/Memory-access-control/The-Execute-Never--XN--attribute-and-instruction-prefetching?lang=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]
