Re: [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Fri, 9 Oct 2020 at 13:19, Patrick DELAUNAY wrote:
>
> Hi Ard,
>
> > From: Ard Biesheuvel
> > Sent: mercredi 7 octobre 2020 12:27
> >
> > On Tue, 6 Oct 2020 at 18:36, Patrick Delaunay
> > wrote:
> > >
> > >
> > > Hi,
> > >
> > > On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region
> > > protected by a firewall. This region is reserved in device with "no-map"
> > > property.
> > >
> > > But sometime the platform boot failed in U-boot on a Cortex A7 access
> > > to this region (depending of the binary and the issue can change with
> > > compiler version or with code alignment), then the firewall raise a
> > > error, for example:
> > >
> > > E/TC:0 tzc_it_handler:19 TZC permission failure
> > > E/TC:0 dump_fail_filter:420 Permission violation on filter 0
> > > E/TC:0 dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged
> > read,
> > > AXI ID 5c0
> > > E/TC:0 Panic
> > >
> > > After investigation, the forbidden access is a speculative request
> > > performed by the Cortex A7 because all the DDR is mapped as MEMORY
> > > with CACHEABLE property.
> > >
> > > 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 think that can be a general issue for ARM architecture: the no-map
> > > tag in device should be respected by U-boot, so I propose a generic
> > > solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
> > >
> > > This serie is composed by 7 patches
> > > - 1..4/7: preliminary steps to support flags in library in lmb
> > > (as it is done in memblock.c in Linux)
> > > - 5/7: unitary test on the added feature in lmb lib
> > > - 6/7: save the no-map flags in lmb when the device tree is parsed
> > > - 7/7: update the generic behavior for "no-map" region in
> > >arm/lib/cache-cp15.c::dram_bank_mmu_setup()
> > >
> > > It is a rebase on master branch of previous RFC [2].
> > >
> > > I can change this last patch if this feature is note required by other
> > > ARM architecture; it is a weak function so I can avoid to map the
> > > region with "no-map" property in device tree only for STM32MP
> > > architecture (in arch/arm/mach-stm32mp/cpu.c).
> > >
> > > See also [1] which handle same speculative access on armv8 for area
> > > with Executable attribute.
> > >
> > > [1]
> > > http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-
> > > marek.bykow...@gmail.com/ [2]
> > > http://patchwork.ozlabs.org/project/uboot/list/?series=199486=
> > > both=*
> > >
> > > Regards
> > > Patrick
> > >
> > >
> > > Patrick Delaunay (7):
> > > lmb: Add support of flags for no-map properties
> > > lmb: add lmb_is_reserved_flags
> > > lmb: remove lmb_region.size
> > > lmb: add lmb_dump_region() function
> > > test: lmb: add test for lmb_reserve_flags
> > > image-fdt: save no-map parameter of reserve-memory
> > > arm: cache: cp15: don't map the reserved region with no-map property
> > >
> > > arch/arm/include/asm/system.h | 3 +
> > > arch/arm/lib/cache-cp15.c | 19 ++-
> > > common/image-fdt.c| 23 +---
> > > include/lmb.h | 22 +++-
> > > lib/lmb.c | 100 +++---
> > > test/lib/lmb.c| 89 ++
> > > 6 files changed, 212 insertions(+), 44 deletions(-)
> > >
> > > --
> > > 2.17.1
> > >
>
> No, I don't yet described the issue in details on mailing list.
> So I will explain the investigation done and my conclusion.
>
> On STM32MP15x platform we have firewall to protect the access to DDR (TZC400
> IP).
>
> When OP-TEE is used, the boot chain is:
>
> 1/ ROM-Code (secure)
> 2/ TF-A (BL2) in SYSRAM (secure)
> 3/ OP-TEE in SYSRAM and DDR (secure)
> 4/ U-Boot in DDR
> 5/ Linux lernel
>
> OP-TEE is loaded by TF-A BL2
> - in SYRAM (for pager part)
> - in DDR (for pageable part).
>
> U-Boot is loaded by TF-A BL2 in DDR.
>
> When OP-TEE is execute, it protects with TZC400 firewall the reserved part
> of DDR as defined in device tree :
>
> For example : ./arch/arm/dts/stm32mp157a-ed1-u-boot.dtsi:42:
> reserved-memory {
> optee@fe00 {
> reg = <0xfe00 0x0200>;
> no-map;
> };
> };
>
> When OP-TEE is running in secure world (secure monitor is resident),
> it jump to normal world (unsecure) = U-Boot
>
>
RE: [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 12:27
>
> On Tue, 6 Oct 2020 at 18:36, Patrick Delaunay
> wrote:
> >
> >
> > Hi,
> >
> > On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region
> > protected by a firewall. This region is reserved in device with "no-map"
> > property.
> >
> > But sometime the platform boot failed in U-boot on a Cortex A7 access
> > to this region (depending of the binary and the issue can change with
> > compiler version or with code alignment), then the firewall raise a
> > error, for example:
> >
> > E/TC:0 tzc_it_handler:19 TZC permission failure
> > E/TC:0 dump_fail_filter:420 Permission violation on filter 0
> > E/TC:0 dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged
> read,
> > AXI ID 5c0
> > E/TC:0 Panic
> >
> > After investigation, the forbidden access is a speculative request
> > performed by the Cortex A7 because all the DDR is mapped as MEMORY
> > with CACHEABLE property.
> >
> > 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 think that can be a general issue for ARM architecture: the no-map
> > tag in device should be respected by U-boot, so I propose a generic
> > solution in arm/lib/cache-cp15.c:dram_bank_mmu_setup().
> >
> > This serie is composed by 7 patches
> > - 1..4/7: preliminary steps to support flags in library in lmb
> > (as it is done in memblock.c in Linux)
> > - 5/7: unitary test on the added feature in lmb lib
> > - 6/7: save the no-map flags in lmb when the device tree is parsed
> > - 7/7: update the generic behavior for "no-map" region in
> >arm/lib/cache-cp15.c::dram_bank_mmu_setup()
> >
> > It is a rebase on master branch of previous RFC [2].
> >
> > I can change this last patch if this feature is note required by other
> > ARM architecture; it is a weak function so I can avoid to map the
> > region with "no-map" property in device tree only for STM32MP
> > architecture (in arch/arm/mach-stm32mp/cpu.c).
> >
> > See also [1] which handle same speculative access on armv8 for area
> > with Executable attribute.
> >
> > [1]
> > http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-
> > marek.bykow...@gmail.com/ [2]
> > http://patchwork.ozlabs.org/project/uboot/list/?series=199486=
> > both=*
> >
> > Regards
> > Patrick
> >
> >
> > Patrick Delaunay (7):
> > lmb: Add support of flags for no-map properties
> > lmb: add lmb_is_reserved_flags
> > lmb: remove lmb_region.size
> > lmb: add lmb_dump_region() function
> > test: lmb: add test for lmb_reserve_flags
> > image-fdt: save no-map parameter of reserve-memory
> > arm: cache: cp15: don't map the reserved region with no-map property
> >
> > arch/arm/include/asm/system.h | 3 +
> > arch/arm/lib/cache-cp15.c | 19 ++-
> > common/image-fdt.c| 23 +---
> > include/lmb.h | 22 +++-
> > lib/lmb.c | 100 +++---
> > test/lib/lmb.c| 89 ++
> > 6 files changed, 212 insertions(+), 44 deletions(-)
> >
> > --
> > 2.17.1
> >
No, I don't yet described the issue in details on mailing list.
So I will explain the investigation done and my conclusion.
On STM32MP15x platform we have firewall to protect the access to DDR (TZC400
IP).
When OP-TEE is used, the boot chain is:
1/ ROM-Code (secure)
2/ TF-A (BL2) in SYSRAM (secure)
3/ OP-TEE in SYSRAM and DDR (secure)
4/ U-Boot in DDR
5/ Linux lernel
OP-TEE is loaded by TF-A BL2
- in SYRAM (for pager part)
- in DDR (for pageable part).
U-Boot is loaded by TF-A BL2 in DDR.
When OP-TEE is execute, it protects with TZC400 firewall the reserved part of
DDR as defined in device tree :
For example : ./arch/arm/dts/stm32mp157a-ed1-u-boot.dtsi:42:
reserved-memory {
optee@fe00 {
reg = <0xfe00 0x0200>;
no-map;
};
};
When OP-TEE is running in secure world (secure monitor is resident),
it jump to normal world (unsecure) = U-Boot
After relocation, U-Boot maps all the DDR as normal memory
(cacheable, bufferable, executable) and activate data cahe
Then, sometime (because depending of the compilation), the firewall detect an
unsecure access
to OP-TEE secured region when U-Boot is running.
We have investigate this access with gdb:
- it
Re: [PATCH 0/7] arm: cache: cp15: don't map reserved region with no-map property
On Tue, 6 Oct 2020 at 18:36, Patrick Delaunay wrote: > > > Hi, > > On STM32MP15x platform we can use OP-TEE, loaded in DDR in a region > protected by a firewall. This region is reserved in device with "no-map" > property. > > But sometime the platform boot failed in U-boot on a Cortex A7 access to > this region (depending of the binary and the issue can change with compiler > version or with code alignment), then the firewall raise a error, > for example: > > E/TC:0 tzc_it_handler:19 TZC permission failure > E/TC:0 dump_fail_filter:420 Permission violation on filter 0 > E/TC:0 dump_fail_filter:425 Violation @0xde5c6bf0, non-secure privileged > read, > AXI ID 5c0 > E/TC:0 Panic > > After investigation, the forbidden access is a speculative request performed > by the Cortex A7 because all the DDR is mapped as MEMORY with CACHEABLE > property. > > 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 think that can be a general issue for ARM architecture: the no-map tag > in device should be respected by U-boot, so I propose a generic solution > in arm/lib/cache-cp15.c:dram_bank_mmu_setup(). > > This serie is composed by 7 patches > - 1..4/7: preliminary steps to support flags in library in lmb > (as it is done in memblock.c in Linux) > - 5/7: unitary test on the added feature in lmb lib > - 6/7: save the no-map flags in lmb when the device tree is parsed > - 7/7: update the generic behavior for "no-map" region in >arm/lib/cache-cp15.c::dram_bank_mmu_setup() > > It is a rebase on master branch of previous RFC [2]. > > I can change this last patch if this feature is note required by other > ARM architecture; it is a weak function so I can avoid to map the region > with "no-map" property in device tree only for STM32MP architecture > (in arch/arm/mach-stm32mp/cpu.c). > > See also [1] which handle same speculative access on armv8 for area > with Executable attribute. > > [1] > http://patchwork.ozlabs.org/project/uboot/patch/20200903000106.5016-1-marek.bykow...@gmail.com/ > [2] > http://patchwork.ozlabs.org/project/uboot/list/?series=199486=both=* > > Regards > Patrick > > > Patrick Delaunay (7): > lmb: Add support of flags for no-map properties > lmb: add lmb_is_reserved_flags > lmb: remove lmb_region.size > lmb: add lmb_dump_region() function > test: lmb: add test for lmb_reserve_flags > image-fdt: save no-map parameter of reserve-memory > arm: cache: cp15: don't map the reserved region with no-map property > > arch/arm/include/asm/system.h | 3 + > arch/arm/lib/cache-cp15.c | 19 ++- > common/image-fdt.c| 23 +--- > include/lmb.h | 22 +++- > lib/lmb.c | 100 +++--- > test/lib/lmb.c| 89 ++ > 6 files changed, 212 insertions(+), 44 deletions(-) > > -- > 2.17.1 >
