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.delau...@st.com> wrote:
>
> Hi,
>
> > From: Ard Biesheuvel <a...@kernel.org>
> > Sent: mardi 27 octobre 2020 22:05
> >
> > On Tue, 27 Oct 2020 at 18:25, Tom Rini <tr...@konsulko.com> wrote:
> > >
> > > On Fri, Oct 09, 2020 at 05:00:44PM +0000, Patrick DELAUNAY wrote:
> > > > 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-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@... 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
