[PATCH v3 1/3] fdtdec: optionally add property no-map to created reserved memory node

[Etienne Carriere]

Hello Heinrich,

On Tue, 25 Aug 2020 at 16:50, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 25.08.20 13:42, Patrice Chotard wrote:
> > From: Etienne Carriere <etienne.carriere at st.com>
> >
> > Add boolean input argument @no_map to helper function
> > fdtdec_add_reserved_memory() to add "no-map" property for an added
> > reserved memory node. This is needed for example when the reserved
> > memory relates to secure memory that the dear Linux kernel shall
> > not even map unless what non-secure world speculative accesses of the
>
> This sentence needs rework. Do you mean "to avoid non-secure world
> accesses of the CPU that might reveal the secure-world memory"?
>
> Is there any evidence that mapping memory as reserved can lead to an
> information leak and that not mapping solves the problem? Is there a CVE
> for it?

There should not be issues related to revealing secure world memory.
Such memory is likely to be protected by a hardware firewall.
Issues are rather due to CPU speculative execution that could emit
read requests to such firewalled memory areas. These read request
would infringe the firewall policy which would likely report a system
error and would possibly panic the system.

By not mapping such memory areas, speculative reads are discarded
before leaving the CPU, at least on Arm architecture.

Note that "no-map" property is also used by Linux to prevent inconsistent
kernel mapping for memories that are mapped non-cached at runtime
by some specific driver. Such memory must be not mapped by the
kernel static memory mapping.

Ok, let's rephrase this commit log. Proposal for the patchset v4:
I hope it looks better:

|  fdtdec: optionally add property no-map to created reserved memory node
|
|  Add boolean input argument @no_map to helper function
|  fdtdec_add_reserved_memory() to add or not "no-map" property
|  for an added reserved memory node.
|
|  Property no-map is used by the Linux kernel to not not map memory
|  in its static memory mapping. It is needed for example for the|
|  consistency of system non-cached memory and to prevent speculative
|  accesses to some firewalled memory.
|
|  No functional change. A later change will update to OPTEE library to
|  add no-map property to OP-TEE reserved memory nodes.
|
| Signed-off-by: (...)


> > CPU can violate the memory firmware configuration.
>
> Most Linux distributions boot via EFI.
>
> In U-Boot's UEFI sub-system we pass reserved memory as
> EFI_RESERVED_MEMORY_TYPE in the memory map to Linux. See function
> efi_carve_out_dt_rsv(). We do not consider the no-map attribute in the
> device-tree.
>
> Does the Linux kernel care about this no-map attribute in the
> device-tree if we are booting via UEFI?

Sorry I lack experience and knowledge on how Linux relies on UEFI versus DT
to describe the system memory. I think the EFI_RESERVED_MEMORY_TYPE
should consider such reserved memory attributes, but I'm far from knowing
how UEFI should handle that.

This is quite a late answer from me. Since your post I saw there are discussion
in this area:
https://lists.linaro.org/pipermail/boot-architecture/2020-September/001389.html


>
> >
> > No function change. A later change will update to OPTEE library to
> > add no-map property to OP-TEE reserved memory nodes.
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere at st.com>
> > Signed-off-by: Patrice Chotard <patrice.chotard at st.com>
> > ---
> >
> > (no changes since v2)
> >
> > Changes in v2:
> >    - fix dm fdtdec test and arch/riscv/lib/fdt_fixup.c with
> >    fdtdec_add_reserved_memory() new parameter
> >
> >  arch/riscv/lib/fdt_fixup.c |  2 +-
> >  include/fdtdec.h           |  5 +++--
> >  lib/fdtdec.c               | 10 ++++++++--
> >  lib/optee/optee.c          |  2 +-
> >  test/dm/fdtdec.c           |  6 +++---
> >  5 files changed, 16 insertions(+), 9 deletions(-)
> >
> > diff --git a/arch/riscv/lib/fdt_fixup.c b/arch/riscv/lib/fdt_fixup.c
> > index 5b2420243f..d02062fd5b 100644
> > --- a/arch/riscv/lib/fdt_fixup.c
> > +++ b/arch/riscv/lib/fdt_fixup.c
> > @@ -75,7 +75,7 @@ int riscv_fdt_copy_resv_mem_node(const void *src, void *dst)
> >               pmp_mem.start = addr;
> >               pmp_mem.end = addr + size - 1;
> >               err = fdtdec_add_reserved_memory(dst, basename, &pmp_mem,
> > -                                              &phandle);
> > +                                              &phandle, false);
>
> I guess in a future patch we would want to set nomap=true here too as
> this is the memory reserved by the secure execution environment (e.g.
> OpenSBI).

I think so, yes, but I would prefer RISCV maintainers to comment on that.

Best regards,
Etienne

>
> Best regards
>
> Heinrich
>
> >               if (err < 0 && err != -FDT_ERR_EXISTS) {
> >                       log_err("failed to add reserved memory: %d\n", err);
> >                       return err;
> > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > index bc79389260..f127c7d386 100644
> > --- a/include/fdtdec.h
> > +++ b/include/fdtdec.h
> > (...)