On Wed, 7 Oct 2020 at 16:55, Etienne Carriere
<etienne.carri...@linaro.org> wrote:
>
> Hello all,
>
> On Wed, 7 Oct 2020 at 15:16, Ard Biesheuvel <a...@kernel.org> wrote:
> >
> > 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.
>
> 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.
