[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property

[Tom Rini <trini@konsulko.com>]

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-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]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20201027/5a147f22/attachment.sig>