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

[Etienne Carriere]

Hello all,

On Wed, 7 Oct 2020 at 15:16, Ard Biesheuvel <ardb at kernel.org> wrote:
>
> On Wed, 7 Oct 2020 at 13:53, Ahmad Fatoum <a.fatoum at pengutronix.de> wrote:
> >
> > Hello,
> >
> > On 10/7/20 1:23 PM, Ahmad Fatoum wrote:
> > > My findings[1] back then were that U-Boot did set the eXecute Never bit only on
> > > OMAP, but not for other platforms.  So I could imagine this being the root cause
> > > of Patrick's issues as well:
> >
> > Rereading my own link, my memory is a little less fuzzy: eXecute Never was being
> > set, but was without effect due Manager mode being set in the DACR:
> >
> > > The ARM Architecture Reference Manual notes[1]:
> > > > When using the Short-descriptor translation table format, the XN
> > > > attribute is not checked for domains marked as Manager.
> > > > Therefore, the system must not include read-sensitive memory in
> > > > domains marked as Manager, because the XN bit does not prevent
> > > > speculative fetches from a Manager domain.
> >
> > > To avoid speculative access to read-sensitive memory-mapped peripherals
> > > on ARMv7, we'll need U-Boot to use client domain permissions, so the XN
> > > bit can function.
> >
> > > This issue has come up before and was fixed in de63ac278
> > > ("ARM: mmu: Set domain permissions to client access") for OMAP2 only.
> > > It's equally applicable to all ARMv7-A platforms where caches are
> > > enabled.
> > > [1]: B3.7.2 - Execute-never restrictions on instruction fetching
> >
> > Hope this helps,
> > Ahmad
> >
>
> It most definitely does, thanks a lot.
>
> U-boot's mmu_setup() currently sets DACR to manager for all domains,
> so this is broken for all non-LPAE configurations running on v7 CPUs
> (except OMAP and perhaps others that fixed it individually). This
> affects all device mappings: not just secure DRAM for OP-TEE, but any
> MMIO register for any peripheral that is mapped into the CPU's address
> space.

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" (0x55555555));
>
>         arm_init_domains();