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

[Etienne Carriere <etienne.carriere@linaro.org>]

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 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).
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