[PATCH v3 1/3] fdtdec: optionally add property no-map to created reserved memory node
From: Etienne Carriere etienne.carriere@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 CPU can violate the memory firmware configuration.
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@st.com Signed-off-by: Patrice Chotard patrice.chotard@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); 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 @@ -1016,7 +1016,7 @@ static inline int fdtdec_set_phandle(void *blob, int node, uint32_t phandle) * }; * uint32_t phandle; * - * fdtdec_add_reserved_memory(fdt, "framebuffer", &fb, &phandle); + * fdtdec_add_reserved_memory(fdt, "framebuffer", &fb, &phandle, false); * * This results in the following subnode being added to the top-level * /reserved-memory node: @@ -1043,11 +1043,12 @@ static inline int fdtdec_set_phandle(void *blob, int node, uint32_t phandle) * @param carveout information about the carveout region * @param phandlep return location for the phandle of the carveout region * can be NULL if no phandle should be added + * @param no_map add "no-map" property if true * @return 0 on success or a negative error code on failure */ int fdtdec_add_reserved_memory(void *blob, const char *basename, const struct fdt_memory *carveout, - uint32_t *phandlep); + uint32_t *phandlep, bool no_map);
/** * fdtdec_get_carveout() - reads a carveout from an FDT diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 30a1c6a217..bf40d87cb3 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1303,7 +1303,7 @@ static int fdtdec_init_reserved_memory(void *blob)
int fdtdec_add_reserved_memory(void *blob, const char *basename, const struct fdt_memory *carveout, - uint32_t *phandlep) + uint32_t *phandlep, bool no_map) { fdt32_t cells[4] = {}, *ptr = cells; uint32_t upper, lower, phandle; @@ -1403,6 +1403,12 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename, if (err < 0) return err;
+ if (no_map) { + err = fdt_setprop(blob, node, "no-map", NULL, 0); + if (err < 0) + return err; + } + /* return the phandle for the new node for the caller to use */ if (phandlep) *phandlep = phandle; @@ -1468,7 +1474,7 @@ int fdtdec_set_carveout(void *blob, const char *node, const char *prop_name, fdt32_t value; void *prop;
- err = fdtdec_add_reserved_memory(blob, name, carveout, &phandle); + err = fdtdec_add_reserved_memory(blob, name, carveout, &phandle, false); if (err < 0) { debug("failed to add reserved memory: %d\n", err); return err; diff --git a/lib/optee/optee.c b/lib/optee/optee.c index 457d4cca8a..963c2ff430 100644 --- a/lib/optee/optee.c +++ b/lib/optee/optee.c @@ -192,7 +192,7 @@ int optee_copy_fdt_nodes(const void *old_blob, void *new_blob) ret = fdtdec_add_reserved_memory(new_blob, nodename, &carveout, - NULL); + NULL, false); free(oldname);
if (ret < 0) diff --git a/test/dm/fdtdec.c b/test/dm/fdtdec.c index 716993f706..4119003041 100644 --- a/test/dm/fdtdec.c +++ b/test/dm/fdtdec.c @@ -80,7 +80,7 @@ static int dm_test_fdtdec_add_reserved_memory(struct unit_test_state *uts) resv.start = 0x1000; resv.end = 0x1fff; ut_assertok(fdtdec_add_reserved_memory(blob, "rsvd_region", - &resv, &phandle)); + &resv, &phandle, false));
/* Test /reserve-memory and its subnode should exist */ parent = fdt_path_offset(blob, "/reserved-memory"); @@ -101,7 +101,7 @@ static int dm_test_fdtdec_add_reserved_memory(struct unit_test_state *uts) resv.start = 0x2000; resv.end = 0x2fff; ut_assertok(fdtdec_add_reserved_memory(blob, "rsvd_region1", - &resv, &phandle1)); + &resv, &phandle1, false)); subnode = fdt_path_offset(blob, "/reserved-memory/rsvd_region1"); ut_assert(subnode > 0);
@@ -115,7 +115,7 @@ static int dm_test_fdtdec_add_reserved_memory(struct unit_test_state *uts) resv.start = 0x1000; resv.end = 0x1fff; ut_assertok(fdtdec_add_reserved_memory(blob, "rsvd_region2", - &resv, &phandle1)); + &resv, &phandle1, false)); subnode = fdt_path_offset(blob, "/reserved-memory/rsvd_region2"); ut_assert(subnode < 0);
Add a test to verify that the no-map property is added in reserved-memory node when fdtdec_add_reserved_memory() no-map parameter is set to true.
Signed-off-by: Patrice Chotard patrice.chotard@st.com
---
(no changes since v2)
Changes in v2: - Add no-map property test into fdtdec test
test/dm/fdtdec.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/test/dm/fdtdec.c b/test/dm/fdtdec.c index 4119003041..017157a2ec 100644 --- a/test/dm/fdtdec.c +++ b/test/dm/fdtdec.c @@ -101,10 +101,13 @@ static int dm_test_fdtdec_add_reserved_memory(struct unit_test_state *uts) resv.start = 0x2000; resv.end = 0x2fff; ut_assertok(fdtdec_add_reserved_memory(blob, "rsvd_region1", - &resv, &phandle1, false)); + &resv, &phandle1, true)); subnode = fdt_path_offset(blob, "/reserved-memory/rsvd_region1"); ut_assert(subnode > 0);
+ /* check that no-map property is present */ + ut_assert(fdt_getprop(blob, subnode, "no-map", NULL) > 0); + /* phandles must be different */ ut_assert(phandle != phandle1);
On Tue, 25 Aug 2020 at 05:42, Patrice Chotard patrice.chotard@st.com wrote:
Add a test to verify that the no-map property is added in reserved-memory node when fdtdec_add_reserved_memory() no-map parameter is set to true.
Signed-off-by: Patrice Chotard patrice.chotard@st.com
(no changes since v2)
Changes in v2:
- Add no-map property test into fdtdec test
test/dm/fdtdec.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
From: Etienne Carriere etienne.carriere@st.com
OP-TEE reserved memory node must set property "no-map" to prevent Linux kernel from mapping secure memory unless what non-secure world speculative accesses of the CPU can violate the memory firmware configuration.
Fixes: 6ccb05eae01b ("image: fdt: copy possible optee nodes to a loaded devicetree") Signed-off-by: Etienne Carriere etienne.carriere@st.com Signed-off-by: Patrice Chotard patrice.chotard@st.com ---
Changes in v3: - Fix changelogs
lib/optee/optee.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/optee/optee.c b/lib/optee/optee.c index 963c2ff430..9e6606568f 100644 --- a/lib/optee/optee.c +++ b/lib/optee/optee.c @@ -192,7 +192,7 @@ int optee_copy_fdt_nodes(const void *old_blob, void *new_blob) ret = fdtdec_add_reserved_memory(new_blob, nodename, &carveout, - NULL, false); + NULL, true); free(oldname);
if (ret < 0)
On 25.08.20 13:42, Patrice Chotard wrote:
From: Etienne Carriere etienne.carriere@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?
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?
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@st.com Signed-off-by: Patrice Chotard patrice.chotard@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).
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 @@ -1016,7 +1016,7 @@ static inline int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
};uint32_t phandle;
fdtdec_add_reserved_memory(fdt, "framebuffer", &fb, &phandle);
fdtdec_add_reserved_memory(fdt, "framebuffer", &fb, &phandle, false);- This results in the following subnode being added to the top-level
- /reserved-memory node:
@@ -1043,11 +1043,12 @@ static inline int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
- @param carveout information about the carveout region
- @param phandlep return location for the phandle of the carveout region
can be NULL if no phandle should be added
*/
- @param no_map add "no-map" property if true
- @return 0 on success or a negative error code on failure
int fdtdec_add_reserved_memory(void *blob, const char *basename, const struct fdt_memory *carveout,
uint32_t *phandlep);
uint32_t *phandlep, bool no_map);/**
- fdtdec_get_carveout() - reads a carveout from an FDT
diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 30a1c6a217..bf40d87cb3 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1303,7 +1303,7 @@ static int fdtdec_init_reserved_memory(void *blob)
int fdtdec_add_reserved_memory(void *blob, const char *basename, const struct fdt_memory *carveout,
uint32_t *phandlep)
uint32_t *phandlep, bool no_map){ fdt32_t cells[4] = {}, *ptr = cells; uint32_t upper, lower, phandle; @@ -1403,6 +1403,12 @@ int fdtdec_add_reserved_memory(void *blob, const char *basename, if (err < 0) return err;
- if (no_map) {
err = fdt_setprop(blob, node, "no-map", NULL, 0);if (err < 0)return err;- }
- /* return the phandle for the new node for the caller to use */ if (phandlep) *phandlep = phandle;
@@ -1468,7 +1474,7 @@ int fdtdec_set_carveout(void *blob, const char *node, const char *prop_name, fdt32_t value; void *prop;
- err = fdtdec_add_reserved_memory(blob, name, carveout, &phandle);
- err = fdtdec_add_reserved_memory(blob, name, carveout, &phandle, false); if (err < 0) { debug("failed to add reserved memory: %d\n", err); return err;
diff --git a/lib/optee/optee.c b/lib/optee/optee.c index 457d4cca8a..963c2ff430 100644 --- a/lib/optee/optee.c +++ b/lib/optee/optee.c @@ -192,7 +192,7 @@ int optee_copy_fdt_nodes(const void *old_blob, void *new_blob) ret = fdtdec_add_reserved_memory(new_blob, nodename, &carveout,
NULL);
NULL, false); free(oldname); if (ret < 0)diff --git a/test/dm/fdtdec.c b/test/dm/fdtdec.c index 716993f706..4119003041 100644 --- a/test/dm/fdtdec.c +++ b/test/dm/fdtdec.c @@ -80,7 +80,7 @@ static int dm_test_fdtdec_add_reserved_memory(struct unit_test_state *uts) resv.start = 0x1000; resv.end = 0x1fff; ut_assertok(fdtdec_add_reserved_memory(blob, "rsvd_region",
&resv, &phandle));
&resv, &phandle, false));/* Test /reserve-memory and its subnode should exist */ parent = fdt_path_offset(blob, "/reserved-memory");
@@ -101,7 +101,7 @@ static int dm_test_fdtdec_add_reserved_memory(struct unit_test_state *uts) resv.start = 0x2000; resv.end = 0x2fff; ut_assertok(fdtdec_add_reserved_memory(blob, "rsvd_region1",
&resv, &phandle1));
&resv, &phandle1, false));@@ -115,7 +115,7 @@ static int dm_test_fdtdec_add_reserved_memory(struct unit_test_state *uts) resv.start = 0x1000; resv.end = 0x1fff; ut_assertok(fdtdec_add_reserved_memory(blob, "rsvd_region2",
&resv, &phandle1));
&resv, &phandle1, false));
Hello Heinrich,
On Tue, 25 Aug 2020 at 16:50, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 25.08.20 13:42, Patrice Chotard wrote:
From: Etienne Carriere etienne.carriere@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.h...
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@st.com Signed-off-by: Patrice Chotard patrice.chotard@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 (...)
participants (4)
-
Etienne Carriere -
Heinrich Schuchardt -
Patrice Chotard -
Simon Glass