Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On 10/29/20 5:06 PM, Etienne Carriere wrote: > On Thu, 29 Oct 2020 at 12:26, Ard Biesheuvel wrote: >> 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 Arm Trustzone stand point, both secure and non-secure worlds > share the very same physical address space. I your example, physical > address 0x0 would refer to the same DRAM cell. Whether this cell is secure > or non-secure is a configuration set in the DRAM firmwall. No, like Ard said it is a possibility but it doesn't have to be the case. See the Armv8-A ARM (DDI 0487F.c) section D5.1.3 VMSA address types and address spaces, "Physical address (PA)". If we need to differentiate between non-secure and secure PA I suppose we could use the status and secure-status properties in the memory nodes, consistent with the usual usage described in [1]. As Etienne says, it seems that a majority of systems actually have a single PA space with access control added on top, and by default the secure state can access non-secure memory. That goes well with memory nodes without a status nor a secure-status property, yet other configurations can easily be supported. [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/secure.txt -- Jerome
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Thu, 29 Oct 2020 at 17:35, Jerome Forissier wrote: > > > > On 10/29/20 5:06 PM, Etienne Carriere wrote: > > On Thu, 29 Oct 2020 at 12:26, Ard Biesheuvel wrote: > >> 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 Arm Trustzone stand point, both secure and non-secure worlds > > share the very same physical address space. I your example, physical > > address 0x0 would refer to the same DRAM cell. Whether this cell is secure > > or non-secure is a configuration set in the DRAM firmwall. > > No, like Ard said it is a possibility but it doesn't have to be the > case. See the Armv8-A ARM (DDI 0487F.c) section D5.1.3 VMSA address > types and address spaces, "Physical address (PA)". Ok. I didn't know that. Thanks both to highlight this and thanks for the refs. However, I think this does not change the question on whether or not a memory node in non-secure world FDT can cover address ranges that are carved out with reserved-memory/no-map because non-secure world generic mapping cannot presume valid default mapping attributes. > If we need to differentiate between non-secure and secure PA I suppose > we could use the status and secure-status properties in the memory > nodes, consistent with the usual usage described in [1]. > > As Etienne says, it seems that a majority of systems actually have a > single PA space with access control added on top, and by default the > secure state can access non-secure memory. That goes well with memory > nodes without a status nor a secure-status property, yet other > configurations can easily be supported. > > [1] > https://www.kernel.org/doc/Documentation/devicetree/bindings/arm/secure.txt > > -- > Jerome Cheers, Etienne
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Thu, 29 Oct 2020 at 17:06, Etienne Carriere
wrote:
>
> On Thu, 29 Oct 2020 at 12:26, Ard Biesheuvel wrote:
> >
> > On Thu, 29 Oct 2020 at 11:40, Etienne Carriere
> > 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
> > > wrote:
> > > >
> > > > Hi,
> > > >
> > > > > From: Ard Biesheuvel
> > > > > Sent: mardi 27 octobre 2020 22:05
> > > > >
> > > > > On Tue, 27 Oct 2020 at 18:25, Tom Rini wrote:
> > > > > >
> > > > > > On Fri, Oct 09, 2020 at 05:00:44PM +, Patrick DELAUNAY wrote:
> > > > > > > Hi Ard,
> > > > > > >
> > > > > > > > From: Ard Biesheuvel
> > > > > > > > Sent: mercredi 7 octobre 2020 15:16
> > > > > > > >
> > > > > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum
> > > > > > > >
> > > > > 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" (0x));
> > > > > > > >
> > > > > > > > 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 +
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Thu, 29 Oct 2020 at 12:26, Ard Biesheuvel wrote:
>
> On Thu, 29 Oct 2020 at 11:40, Etienne Carriere
> 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
> > wrote:
> > >
> > > Hi,
> > >
> > > > From: Ard Biesheuvel
> > > > Sent: mardi 27 octobre 2020 22:05
> > > >
> > > > On Tue, 27 Oct 2020 at 18:25, Tom Rini wrote:
> > > > >
> > > > > On Fri, Oct 09, 2020 at 05:00:44PM +, Patrick DELAUNAY wrote:
> > > > > > Hi Ard,
> > > > > >
> > > > > > > From: Ard Biesheuvel
> > > > > > > Sent: mercredi 7 octobre 2020 15:16
> > > > > > >
> > > > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum
> > > > > > >
> > > > 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" (0x));
> > > > > > >
> > > > > > > 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
> > > > > >
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Thu, 29 Oct 2020 at 11:40, Etienne Carriere
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
> wrote:
> >
> > Hi,
> >
> > > From: Ard Biesheuvel
> > > Sent: mardi 27 octobre 2020 22:05
> > >
> > > On Tue, 27 Oct 2020 at 18:25, Tom Rini wrote:
> > > >
> > > > On Fri, Oct 09, 2020 at 05:00:44PM +, Patrick DELAUNAY wrote:
> > > > > Hi Ard,
> > > > >
> > > > > > From: Ard Biesheuvel
> > > > > > Sent: mercredi 7 octobre 2020 15:16
> > > > > >
> > > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum
> > > 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" (0x));
> > > > > >
> > > > > > 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
> > > > >
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Dear all,
CC some fellow OP-TEE guys for this secure memory description topic.
On Wed, 28 Oct 2020 at 11:33, Patrick DELAUNAY wrote:
>
> Hi,
>
> > From: Ard Biesheuvel
> > Sent: mardi 27 octobre 2020 22:05
> >
> > On Tue, 27 Oct 2020 at 18:25, Tom Rini wrote:
> > >
> > > On Fri, Oct 09, 2020 at 05:00:44PM +, Patrick DELAUNAY wrote:
> > > > Hi Ard,
> > > >
> > > > > From: Ard Biesheuvel
> > > > > Sent: mercredi 7 octobre 2020 15:16
> > > > >
> > > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum
> > 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" (0x));
> > > > >
> > > > > 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);
> > > >
>
RE: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Hi,
> From: Ard Biesheuvel
> Sent: mardi 27 octobre 2020 22:05
>
> On Tue, 27 Oct 2020 at 18:25, Tom Rini wrote:
> >
> > On Fri, Oct 09, 2020 at 05:00:44PM +, Patrick DELAUNAY wrote:
> > > Hi Ard,
> > >
> > > > From: Ard Biesheuvel
> > > > Sent: mercredi 7 octobre 2020 15:16
> > > >
> > > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum
> 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" (0x));
> > > >
> > > > 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 of
> > > when ‘00’ DataAbort(mva, domain, sectionnotpage,
> > > iswrite,
> DAbort_Domain);
> > > when ‘01’ permissioncheck = TRUE;
> > >
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Tue, 27 Oct 2020 at 18:25, Tom Rini wrote:
>
> On Fri, Oct 09, 2020 at 05:00:44PM +, Patrick DELAUNAY wrote:
> > Hi Ard,
> >
> > > From: Ard Biesheuvel
> > > Sent: mercredi 7 octobre 2020 15:16
> > >
> > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum 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" (0x));
> > >
> > > 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 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,
> >
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Fri, Oct 09, 2020 at 05:00:44PM +, Patrick DELAUNAY wrote:
> Hi Ard,
>
> > From: Ard Biesheuvel
> > Sent: mercredi 7 octobre 2020 15:16
> >
> > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum 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" (0x));
> >
> > 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 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;
>
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Mon, 12 Oct 2020 at 11:51, Etienne Carriere wrote: > > On Mon, 12 Oct 2020 at 11:20, Ard Biesheuvel wrote: > > > > On Mon, 12 Oct 2020 at 11:09, Etienne Carriere > > wrote: > > > > > > On Fri, 9 Oct 2020 at 19:13, Ahmad Fatoum wrote: > > > > > > > > Hello Patrick, > > > > > > > > On 10/9/20 5:52 PM, Patrick DELAUNAY wrote: > > > > > I checked DACR behavior and CheckDomain / CheckPermission > > > > > > > > > > In my case the cortex A7 try to access to part of DDR / mapped > > > > > cacheable and bufferable, protected by firewall. > > > > > > > > > > So to use DACR I always need to configure the MMU with several Domain > > > > > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC) > > > > > - secure part of DDR as Domain 1 (DCACHE_OFF) > > > > > > > > > > For other part of MMU region, the I/O registers are mapped as > > > > > register with Domain 0 (D_CACHE_OFF) > > > > > > > > > > Then I can set DACR = 0x > > > > > => Client Accesses are checked against the access permission bits in > > > > > the TLB entry > > > > > > > > > > You ar right, the access permission is check only for domain > > > > > configurated as client in DACR > > > > > > > > > > But in ARM architecture > > > > > > > > > > B2.4.8 Access permission checking > > > > > > > > > > CheckPermission() pseudo code > > > > > Only check perms.ap is checked > > > > > And perms.xp is not checked > > > > > > > > > > But as the secure memory is mapped cacheable by secure OS, OP-TEE > > > > > I think to avoid to map the region is the ARM preconized solution > > > > > As explain in my answer to ard in [1] > > > > > > > > > > [1] > > > > > http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958 > > > > > > > > I don't think the aliasing described in "A3.5.7 Memory access > > > > restrictions" applies if the > > > > same memory is mapped twice only, once in secure and another in normal > > > > world. > > > > Data is already segregated in the caches by means of a NS bit. Only > > > > thing you should need > > > > to do within normal world is mapping it XN, cacheable and not be in > > > > manager domain. > > > > Unmapping sounds unnecessary to me. (You don't unmap peripherals you > > > > aren't using either. > > > > Why handle OP-TEE DRAM specially?) > > > > > > This is right regarding the secure memory/NS=0. But the > > > reserved-memory node for OP-TEE can also cover non-secure (shared) > > > memory that OP-TEE secure world maps Normal/NS=1. So U-boot should not > > > map such memory as Device/NS=0. That would jeopardize non-secure > > > memory content. > > > > > > > Agreed. If secure and non-secure need to have a coherent, cacheable > > view of that shared memory, non-secure device mappings must be > > avoided. > > > > But is no-map really needed for that memory? It needs to be mapped in > > any case to be usable for the non-secure world to communicate with the > > secure world, right? > > > > I'd assume the no-map is only needed if cacheable, inner shareable > > mappings are a problem. > > The non-secure (shared) reserved-memory gets mapped by the related > driver (drivers/tee/optee/) at run time. > Hmm... actually, U-Boot driver does map or use this shared memory, > only Linux does. > But even if U-Boot optee driver does not map the shared memory, OP-TEE > secure world still likely does. > > > > > > Note that platforms can define either a single reserved-memory node in > > > the FDT for both contiguous secure and shared DDR > > > or platforms can alternatively define 2 reserved_memory nodes: one for > > > the secure DDR and one for the non-secure shared DDR. > > > > > > In the 1st case, the no-map property of the single reserved-memory > > > node should really not map the area in U-Boot. > > > > > > In the 2nd case, the no-map property on the secure DDR reserved-memory > > > node must prevent U-Boot speculative accesses while node for shared > > > memory obviously doesn't need no-map. > > > > > > This is to say that mapping as Device memory that has the no-map > > > property can be an issue. > > > > > > > So in summary, there are two separate requirements resulting from this: > > - the DT should not describe secure world memory as ordinary memory, > > as the S and NS address spaces could overlap, and the distinction only > > makes sense for agents running in the secure world; > > I don't mean S and NS can overlap. > I say that reserved-memory with no-map could cover as well S only or > contiguous S and NS ranges. No, it cannot. That is basically the point I am trying to clarify. Even if in this particular case, the firewall configuration simply moves part of the DDR between the secure and non-secure address spaces, this is not required by the architecture. In the general case, non-secure address N and secure address N could both be valid, and refer to different things. When you describe something in the /reserved-memory node, there is no way to
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Mon, 12 Oct 2020 at 11:20, Ard Biesheuvel wrote: > > On Mon, 12 Oct 2020 at 11:09, Etienne Carriere > wrote: > > > > On Fri, 9 Oct 2020 at 19:13, Ahmad Fatoum wrote: > > > > > > Hello Patrick, > > > > > > On 10/9/20 5:52 PM, Patrick DELAUNAY wrote: > > > > I checked DACR behavior and CheckDomain / CheckPermission > > > > > > > > In my case the cortex A7 try to access to part of DDR / mapped > > > > cacheable and bufferable, protected by firewall. > > > > > > > > So to use DACR I always need to configure the MMU with several Domain > > > > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC) > > > > - secure part of DDR as Domain 1 (DCACHE_OFF) > > > > > > > > For other part of MMU region, the I/O registers are mapped as register > > > > with Domain 0 (D_CACHE_OFF) > > > > > > > > Then I can set DACR = 0x > > > > => Client Accesses are checked against the access permission bits in > > > > the TLB entry > > > > > > > > You ar right, the access permission is check only for domain > > > > configurated as client in DACR > > > > > > > > But in ARM architecture > > > > > > > > B2.4.8 Access permission checking > > > > > > > > CheckPermission() pseudo code > > > > Only check perms.ap is checked > > > > And perms.xp is not checked > > > > > > > > But as the secure memory is mapped cacheable by secure OS, OP-TEE > > > > I think to avoid to map the region is the ARM preconized solution > > > > As explain in my answer to ard in [1] > > > > > > > > [1] > > > > http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958 > > > > > > I don't think the aliasing described in "A3.5.7 Memory access > > > restrictions" applies if the > > > same memory is mapped twice only, once in secure and another in normal > > > world. > > > Data is already segregated in the caches by means of a NS bit. Only thing > > > you should need > > > to do within normal world is mapping it XN, cacheable and not be in > > > manager domain. > > > Unmapping sounds unnecessary to me. (You don't unmap peripherals you > > > aren't using either. > > > Why handle OP-TEE DRAM specially?) > > > > This is right regarding the secure memory/NS=0. But the > > reserved-memory node for OP-TEE can also cover non-secure (shared) > > memory that OP-TEE secure world maps Normal/NS=1. So U-boot should not > > map such memory as Device/NS=0. That would jeopardize non-secure > > memory content. > > > > Agreed. If secure and non-secure need to have a coherent, cacheable > view of that shared memory, non-secure device mappings must be > avoided. > > But is no-map really needed for that memory? It needs to be mapped in > any case to be usable for the non-secure world to communicate with the > secure world, right? > > I'd assume the no-map is only needed if cacheable, inner shareable > mappings are a problem. The non-secure (shared) reserved-memory gets mapped by the related driver (drivers/tee/optee/) at run time. Hmm... actually, U-Boot driver does map or use this shared memory, only Linux does. But even if U-Boot optee driver does not map the shared memory, OP-TEE secure world still likely does. > > > Note that platforms can define either a single reserved-memory node in > > the FDT for both contiguous secure and shared DDR > > or platforms can alternatively define 2 reserved_memory nodes: one for > > the secure DDR and one for the non-secure shared DDR. > > > > In the 1st case, the no-map property of the single reserved-memory > > node should really not map the area in U-Boot. > > > > In the 2nd case, the no-map property on the secure DDR reserved-memory > > node must prevent U-Boot speculative accesses while node for shared > > memory obviously doesn't need no-map. > > > > This is to say that mapping as Device memory that has the no-map > > property can be an issue. > > > > So in summary, there are two separate requirements resulting from this: > - the DT should not describe secure world memory as ordinary memory, > as the S and NS address spaces could overlap, and the distinction only > makes sense for agents running in the secure world; I don't mean S and NS can overlap. I say that reserved-memory with no-map could cover as well S only or contiguous S and NS ranges. So no-map does not specifically mean S. It means: only driver knows how to map the thing. > - no-map should be honored by u-boot, but it should only be used if > the existence of a cacheable, inner shareable mapping of that memory > poses a problem. I would say yes, does this description really cover the requirements? I think no-map should be honored for memory that doesn't suit U-Boot default mapping strategy if this one is Device memory in arm arch. Note that linux default memory mapping strategy is Normal/Shared/Cached. No-map should be agnostic from what is software mapping strategy. etienne > > -- > Ard.
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Mon, 12 Oct 2020 at 11:09, Etienne Carriere wrote: > > On Fri, 9 Oct 2020 at 19:13, Ahmad Fatoum wrote: > > > > Hello Patrick, > > > > On 10/9/20 5:52 PM, Patrick DELAUNAY wrote: > > > I checked DACR behavior and CheckDomain / CheckPermission > > > > > > In my case the cortex A7 try to access to part of DDR / mapped cacheable > > > and bufferable, protected by firewall. > > > > > > So to use DACR I always need to configure the MMU with several Domain > > > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC) > > > - secure part of DDR as Domain 1 (DCACHE_OFF) > > > > > > For other part of MMU region, the I/O registers are mapped as register > > > with Domain 0 (D_CACHE_OFF) > > > > > > Then I can set DACR = 0x > > > => Client Accesses are checked against the access permission bits in the > > > TLB entry > > > > > > You ar right, the access permission is check only for domain configurated > > > as client in DACR > > > > > > But in ARM architecture > > > > > > B2.4.8 Access permission checking > > > > > > CheckPermission() pseudo code > > > Only check perms.ap is checked > > > And perms.xp is not checked > > > > > > But as the secure memory is mapped cacheable by secure OS, OP-TEE > > > I think to avoid to map the region is the ARM preconized solution > > > As explain in my answer to ard in [1] > > > > > > [1] > > > http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958 > > > > I don't think the aliasing described in "A3.5.7 Memory access restrictions" > > applies if the > > same memory is mapped twice only, once in secure and another in normal > > world. > > Data is already segregated in the caches by means of a NS bit. Only thing > > you should need > > to do within normal world is mapping it XN, cacheable and not be in manager > > domain. > > Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't > > using either. > > Why handle OP-TEE DRAM specially?) > > This is right regarding the secure memory/NS=0. But the > reserved-memory node for OP-TEE can also cover non-secure (shared) > memory that OP-TEE secure world maps Normal/NS=1. So U-boot should not > map such memory as Device/NS=0. That would jeopardize non-secure > memory content. > Agreed. If secure and non-secure need to have a coherent, cacheable view of that shared memory, non-secure device mappings must be avoided. But is no-map really needed for that memory? It needs to be mapped in any case to be usable for the non-secure world to communicate with the secure world, right? I'd assume the no-map is only needed if cacheable, inner shareable mappings are a problem. > Note that platforms can define either a single reserved-memory node in > the FDT for both contiguous secure and shared DDR > or platforms can alternatively define 2 reserved_memory nodes: one for > the secure DDR and one for the non-secure shared DDR. > > In the 1st case, the no-map property of the single reserved-memory > node should really not map the area in U-Boot. > > In the 2nd case, the no-map property on the secure DDR reserved-memory > node must prevent U-Boot speculative accesses while node for shared > memory obviously doesn't need no-map. > > This is to say that mapping as Device memory that has the no-map > property can be an issue. > So in summary, there are two separate requirements resulting from this: - the DT should not describe secure world memory as ordinary memory, as the S and NS address spaces could overlap, and the distinction only makes sense for agents running in the secure world; - no-map should be honored by u-boot, but it should only be used if the existence of a cacheable, inner shareable mapping of that memory poses a problem. -- Ard.
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Fri, 9 Oct 2020 at 19:13, Ahmad Fatoum wrote: > > Hello Patrick, > > On 10/9/20 5:52 PM, Patrick DELAUNAY wrote: > > I checked DACR behavior and CheckDomain / CheckPermission > > > > In my case the cortex A7 try to access to part of DDR / mapped cacheable > > and bufferable, protected by firewall. > > > > So to use DACR I always need to configure the MMU with several Domain > > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC) > > - secure part of DDR as Domain 1 (DCACHE_OFF) > > > > For other part of MMU region, the I/O registers are mapped as register with > > Domain 0 (D_CACHE_OFF) > > > > Then I can set DACR = 0x > > => Client Accesses are checked against the access permission bits in the > > TLB entry > > > > You ar right, the access permission is check only for domain configurated > > as client in DACR > > > > But in ARM architecture > > > > B2.4.8 Access permission checking > > > > CheckPermission() pseudo code > > Only check perms.ap is checked > > And perms.xp is not checked > > > > But as the secure memory is mapped cacheable by secure OS, OP-TEE > > I think to avoid to map the region is the ARM preconized solution > > As explain in my answer to ard in [1] > > > > [1] > > http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958 > > I don't think the aliasing described in "A3.5.7 Memory access restrictions" > applies if the > same memory is mapped twice only, once in secure and another in normal world. > Data is already segregated in the caches by means of a NS bit. Only thing you > should need > to do within normal world is mapping it XN, cacheable and not be in manager > domain. > Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't > using either. > Why handle OP-TEE DRAM specially?) This is right regarding the secure memory/NS=0. But the reserved-memory node for OP-TEE can also cover non-secure (shared) memory that OP-TEE secure world maps Normal/NS=1. So U-boot should not map such memory as Device/NS=0. That would jeopardize non-secure memory content. Note that platforms can define either a single reserved-memory node in the FDT for both contiguous secure and shared DDR or platforms can alternatively define 2 reserved_memory nodes: one for the secure DDR and one for the non-secure shared DDR. In the 1st case, the no-map property of the single reserved-memory node should really not map the area in U-Boot. In the 2nd case, the no-map property on the secure DDR reserved-memory node must prevent U-Boot speculative accesses while node for shared memory obviously doesn't need no-map. This is to say that mapping as Device memory that has the no-map property can be an issue. Etienne > > Cheers > Ahmad > > > > >> Cheers > >> Ahmad > >> > >> -- > >> Pengutronix e.K. | | > >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | > >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| > >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On 10/9/20 7:12 PM, Ahmad Fatoum wrote: > to do within normal world is mapping it XN, cacheable and not be in manager > domain. s/cacheable/uncacheable/ of course. > Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't > using either. > Why handle OP-TEE DRAM specially?) > > Cheers > Ahmad > >> >>> Cheers >>> Ahmad >>> >>> -- >>> Pengutronix e.K. | | >>> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >>> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| >>> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Hello Patrick, On 10/9/20 5:52 PM, Patrick DELAUNAY wrote: > I checked DACR behavior and CheckDomain / CheckPermission > > In my case the cortex A7 try to access to part of DDR / mapped cacheable and > bufferable, protected by firewall. > > So to use DACR I always need to configure the MMU with several Domain > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC) > - secure part of DDR as Domain 1 (DCACHE_OFF) > > For other part of MMU region, the I/O registers are mapped as register with > Domain 0 (D_CACHE_OFF) > > Then I can set DACR = 0x > => Client Accesses are checked against the access permission bits in the TLB > entry > > You ar right, the access permission is check only for domain configurated as > client in DACR > > But in ARM architecture > > B2.4.8 Access permission checking > > CheckPermission() pseudo code > Only check perms.ap is checked > And perms.xp is not checked > > But as the secure memory is mapped cacheable by secure OS, OP-TEE > I think to avoid to map the region is the ARM preconized solution > As explain in my answer to ard in [1] > > [1] > http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958 I don't think the aliasing described in "A3.5.7 Memory access restrictions" applies if the same memory is mapped twice only, once in secure and another in normal world. Data is already segregated in the caches by means of a NS bit. Only thing you should need to do within normal world is mapping it XN, cacheable and not be in manager domain. Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't using either. Why handle OP-TEE DRAM specially?) Cheers Ahmad > >> Cheers >> Ahmad >> >> -- >> Pengutronix e.K. | | >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Fri, 9 Oct 2020 at 19:13, Ahmad Fatoum wrote: > > Hello Patrick, > > On 10/9/20 5:52 PM, Patrick DELAUNAY wrote: > > I checked DACR behavior and CheckDomain / CheckPermission > > > > In my case the cortex A7 try to access to part of DDR / mapped cacheable > > and bufferable, protected by firewall. > > > > So to use DACR I always need to configure the MMU with several Domain > > - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC) > > - secure part of DDR as Domain 1 (DCACHE_OFF) > > > > For other part of MMU region, the I/O registers are mapped as register with > > Domain 0 (D_CACHE_OFF) > > > > Then I can set DACR = 0x > > => Client Accesses are checked against the access permission bits in the > > TLB entry > > > > You ar right, the access permission is check only for domain configurated > > as client in DACR > > > > But in ARM architecture > > > > B2.4.8 Access permission checking > > > > CheckPermission() pseudo code > > Only check perms.ap is checked > > And perms.xp is not checked > > > > But as the secure memory is mapped cacheable by secure OS, OP-TEE > > I think to avoid to map the region is the ARM preconized solution > > As explain in my answer to ard in [1] > > > > [1] > > http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958 > > I don't think the aliasing described in "A3.5.7 Memory access restrictions" > applies if the > same memory is mapped twice only, once in secure and another in normal world. > Data is already segregated in the caches by means of a NS bit. Only thing you > should need > to do within normal world is mapping it XN, cacheable and not be in manager > domain. > Unmapping sounds unnecessary to me. (You don't unmap peripherals you aren't > using either. > Why handle OP-TEE DRAM specially?) > Ah good point. The secure DDR is in the secure physical address space, whereas the non-secure mapping refers to non-secure physical memory. So from a coherency point of view, they are really not aliases of one another, and so the mismatched attribute concern does not apply. But this actually reinforces my previous point too: the secure and non-secure physical address spaces are disjoint, and the DT can only describe non-secure memory, so these regions don't belong in the DT given to the OS in the first place. > > > > > >> Cheers > >> Ahmad > >> > >> -- > >> Pengutronix e.K. | | > >> Steuerwalder Str. 21 | http://www.pengutronix.de/ | > >> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| > >> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
RE: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Hi Ard,
> From: Ard Biesheuvel
> Sent: mercredi 7 octobre 2020 15:16
>
> On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum 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" (0x));
>
> 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 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;
RE: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Hi Ahmad, > From: Ahmad Fatoum > Sent: mercredi 7 octobre 2020 13:24 > > Hello Ard, Patrick, > > On 10/7/20 12:26 PM, Ard Biesheuvel wrote: > >> The issue is solved only when the region reserved by OP-TEE is no > >> more mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't > enough) > >> as it is already done in Linux kernel. > >> > > > > Spurious peculative accesses to device regions would be a severe > > silicon bug, so I wonder what is going on here. > > > > (Apologies if we are rehashing stuff here that has already been > > discussed - I don't remember the details) > > > > Are you sure that the speculative accesses were not caused by > > misconfigured CPU or page tables, missing TLB maintenance, etc etc? > > Because it really does smell like a software issue not a hardware > > issue. > > I debugged a similar issue a year ago on an i.MX6 UltraLite (also a Cortex-A7) > that turned to ultimately be caused by barebox not mapping I/O memory as non- > executable. This led to very interesting effects. > > 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: > The CPU is speculatively executing from the region that the firewalled DRAM is > mapped at. > > barebox now configures XN for non-RAM before it turns on the MMU. You should > do that as well (in ARM arch code, not only for stm32mp1). Additionally, you > will > want to XN map the region where your OP-TEE sits at. > > [1]: https://community.nxp.com/thread/511925 Thanks to point me this thread. I checked DACR behavior and CheckDomain / CheckPermission In my case the cortex A7 try to access to part of DDR / mapped cacheable and bufferable, protected by firewall. So to use DACR I always need to configure the MMU with several Domain - unsecure part of DDR as Domain 0 (DCACHE_WRITEALLOC) - secure part of DDR as Domain 1 (DCACHE_OFF) For other part of MMU region, the I/O registers are mapped as register with Domain 0 (D_CACHE_OFF) Then I can set DACR = 0x => Client Accesses are checked against the access permission bits in the TLB entry You ar right, the access permission is check only for domain configurated as client in DACR But in ARM architecture B2.4.8 Access permission checking CheckPermission() pseudo code Only check perms.ap is checked And perms.xp is not checked But as the secure memory is mapped cacheable by secure OS, OP-TEE I think to avoid to map the region is the ARM preconized solution As explain in my answer to ard in [1] [1] http://u-boot.10912.n7.nabble.com/PATCH-0-7-arm-cache-cp15-don-t-map-reserved-region-with-no-map-property-tt428715.html#a428958 > Cheers > Ahmad > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Wed, 7 Oct 2020 at 17:08, Ard Biesheuvel wrote:
>
> On Wed, 7 Oct 2020 at 16:55, Etienne Carriere
> wrote:
> >
> > Hello all,
> >
> > On Wed, 7 Oct 2020 at 15:16, Ard Biesheuvel wrote:
> > >
> > > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum 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.
> >
> > Despite the change proposed below seems to fix permissions bypass,
> > I think that not mapping the memory address ranges that are explicitly
> > expected to be not mapped (as in Patrick proposal) seems a consistent
> > approach.
> >
>
> Agreed.
>
> But the issue Patrick is addressing uncovered a real bug that affects
> many platforms, so let's please take advantage of this, and get it
> fixed for everyone.
>
> Then, we can decide whether unmapping the secure DRAM is worth it or
> not, on its own merit, and not based on the justification that it
> fixes a bug that should be fixed in a different way in the first
> place.
Fair.
Etienne
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Wed, 7 Oct 2020 at 16:55, Etienne Carriere
wrote:
>
> Hello all,
>
> On Wed, 7 Oct 2020 at 15:16, Ard Biesheuvel wrote:
> >
> > On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum 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.
>
> Despite the change proposed below seems to fix permissions bypass,
> I think that not mapping the memory address ranges that are explicitly
> expected to be not mapped (as in Patrick proposal) seems a consistent
> approach.
>
Agreed.
But the issue Patrick is addressing uncovered a real bug that affects
many platforms, so let's please take advantage of this, and get it
fixed for everyone.
Then, we can decide whether unmapping the secure DRAM is worth it or
not, on its own merit, and not based on the justification that it
fixes a bug that should be fixed in a different way in the first
place.
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Hello all,
On Wed, 7 Oct 2020 at 15:16, Ard Biesheuvel wrote:
>
> On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum 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.
Despite the change proposed below seems to fix permissions bypass,
I think that not mapping the memory address ranges that are explicitly
expected to be not mapped (as in Patrick proposal) seems a consistent approach.
regards,
etienne
>
> 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" (0x));
>
> arm_init_domains();
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum 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" (0x));
arm_init_domains();
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
Hello Ard, Patrick, On 10/7/20 12:26 PM, Ard Biesheuvel wrote: >> The issue is solved only when the region reserved by OP-TEE is no more >> mapped in U-Boot (mapped as DEVICE/NON-CACHEABLE wasn't enough) as it is >> already done in Linux kernel. >> > > Spurious peculative accesses to device regions would be a severe > silicon bug, so I wonder what is going on here. > > (Apologies if we are rehashing stuff here that has already been > discussed - I don't remember the details) > > Are you sure that the speculative accesses were not caused by > misconfigured CPU or page tables, missing TLB maintenance, etc etc? > Because it really does smell like a software issue not a hardware > issue. I debugged a similar issue a year ago on an i.MX6 UltraLite (also a Cortex-A7) that turned to ultimately be caused by barebox not mapping I/O memory as non-executable. This led to very interesting effects. 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: The CPU is speculatively executing from the region that the firewalled DRAM is mapped at. barebox now configures XN for non-RAM before it turns on the MMU. You should do that as well (in ARM arch code, not only for stm32mp1). Additionally, you will want to XN map the region where your OP-TEE sits at. [1]: https://community.nxp.com/thread/511925 Cheers Ahmad -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
Re: [Uboot-stm32] [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
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
> The CPU is speculatively executing from the region that the firewalled DRAM
> is mapped at.
>
> barebox now configures XN for non-RAM before it turns on the MMU. You should
> do that as well (in ARM arch code, not only for stm32mp1). Additionally,
> you will want to XN map the region where your OP-TEE sits at.
>
> [1]: https://community.nxp.com/thread/511925
>
> Cheers
> Ahmad
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
