[Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Ard Biesheuvel
ardb at kernel.org
Thu Oct 29 12:26:43 CET 2020
On Thu, 29 Oct 2020 at 11:40, Etienne Carriere
<etienne.carriere at 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 at st.com> wrote:
> >
> > Hi,
> >
> > > From: Ard Biesheuvel <ardb at kernel.org>
> > > Sent: mardi 27 octobre 2020 22:05
> > >
> > > On Tue, 27 Oct 2020 at 18:25, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote:
> > > > > Hi Ard,
> > > > >
> > > > > > From: Ard Biesheuvel <ardb at kernel.org>
> > > > > > Sent: mercredi 7 octobre 2020 15:16
> > > > > >
> > > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum at 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 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
>
> 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).
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 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
More information about the U-Boot
mailing list