aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAndre Przywara <andre.przywara@arm.com>2020-03-26 11:22:37 +0000
committerAndre Przywara <andre.przywara@arm.com>2020-04-29 10:19:17 +0100
commitff4e6c35c9f3f0b1d190b5d3761a13d701af6925 (patch)
tree0a3d428afc45dfe79feea509660fff6418e56761
parent52a616b48c617fe8721106f29f2910ca4681afea (diff)
downloadtrusted-firmware-a-ff4e6c35c9f3f0b1d190b5d3761a13d701af6925.tar.gz
fdt/wrappers: Replace fdtw_read_cells() implementation
Our fdtw_read_cells() implementation goes to great lengths to sanity-check every parameter and result, but leaves a big hole open: The size of the storage the value pointer points at needs to match the number of cells given. This can't be easily checked at compile time, since we lose the size information by using a void pointer. Regardless the current usage of this function is somewhat wrong anyways, since we use it on single-element, fixed-length properties only, for which the DT binding specifies the size. Typically we use those functions dealing with a number of cells in DT context to deal with *dynamically* sized properties, which depend on other properties (#size-cells, #clock-cells, ...), to specify the number of cells needed. Another problem with the current implementation is the use of ambiguously sized types (uintptr_t, size_t) together with a certain expectation about their size. In general there is no relation between the length of a DT property and the bitness of the code that parses the DTB: AArch64 code could encounter 32-bit addresses (where the physical address space is limited to 4GB [1]), while AArch32 code could read 64-bit sized properties (/memory nodes on LPAE systems, [2]). To make this more clear, fix the potential issues and also align more with other DT users (Linux and U-Boot), introduce functions to explicitly read uint32 and uint64 properties. As the other DT consumers, we do this based on the generic "read array" function. Convert all users to use either of those two new functions, and make sure we never use a pointer to anything other than uint32_t or uint64_t variables directly. This reveals (and fixes) a bug in plat_spmd_manifest.c, where we write 4 bytes into a uint16_t variable (passed via a void pointer). Also we change the implementation of the function to better align with other libfdt users, by using the right types (fdt32_t) and common variable names (*prop, prop_names). [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi#n874 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/ecx-2000.dts Change-Id: I718de960515117ac7a3331a1b177d2ec224a3890 Signed-off-by: Andre Przywara <andre.przywara@arm.com>
-rw-r--r--common/fdt_wrappers.c70
-rw-r--r--include/common/fdt_wrappers.h6
-rw-r--r--lib/fconf/fconf_dyn_cfg_getter.c12
-rw-r--r--lib/fconf/fconf_tbbr_getter.c12
-rw-r--r--plat/arm/board/fvp/fconf/fconf_hw_config_getter.c9
-rw-r--r--plat/arm/common/fconf/arm_fconf_sp.c5
-rw-r--r--plat/common/plat_spmd_manifest.c16
7 files changed, 58 insertions, 72 deletions
diff --git a/common/fdt_wrappers.c b/common/fdt_wrappers.c
index 5afa14271a..394f3b0ca0 100644
--- a/common/fdt_wrappers.c
+++ b/common/fdt_wrappers.c
@@ -15,55 +15,6 @@
#include <common/fdt_wrappers.h>
/*
- * Read cells from a given property of the given node. At most 2 cells of the
- * property are read, and pointer is updated. Returns 0 on success, and -1 upon
- * error
- */
-int fdtw_read_cells(const void *dtb, int node, const char *prop,
- unsigned int cells, void *value)
-{
- const uint32_t *value_ptr;
- uint32_t hi = 0, lo;
- int value_len;
-
- assert(dtb != NULL);
- assert(prop != NULL);
- assert(value != NULL);
- assert(node >= 0);
-
- /* We expect either 1 or 2 cell property */
- assert(cells <= 2U);
-
- /* Access property and obtain its length (in bytes) */
- value_ptr = fdt_getprop_namelen(dtb, node, prop, (int)strlen(prop),
- &value_len);
- if (value_ptr == NULL) {
- WARN("Couldn't find property %s in dtb\n", prop);
- return -1;
- }
-
- /* Verify that property length accords with cell length */
- if (NCELLS((unsigned int)value_len) != cells) {
- WARN("Property length mismatch\n");
- return -1;
- }
-
- if (cells == 2U) {
- hi = fdt32_to_cpu(*value_ptr);
- value_ptr++;
- }
-
- lo = fdt32_to_cpu(*value_ptr);
-
- if (cells == 2U)
- *((uint64_t *) value) = ((uint64_t) hi << 32) | lo;
- else
- *((uint32_t *) value) = lo;
-
- return 0;
-}
-
-/*
* Read cells from a given property of the given node. Any number of 32-bit
* cells of the property can be read. Returns 0 on success, or a negative
* FDT error value otherwise.
@@ -99,6 +50,27 @@ int fdt_read_uint32_array(const void *dtb, int node, const char *prop_name,
return 0;
}
+int fdt_read_uint32(const void *dtb, int node, const char *prop_name,
+ uint32_t *value)
+{
+ return fdt_read_uint32_array(dtb, node, prop_name, 1, value);
+}
+
+int fdt_read_uint64(const void *dtb, int node, const char *prop_name,
+ uint64_t *value)
+{
+ uint32_t array[2] = {0, 0};
+ int ret;
+
+ ret = fdt_read_uint32_array(dtb, node, prop_name, 2, array);
+ if (ret < 0) {
+ return ret;
+ }
+
+ *value = ((uint64_t)array[0] << 32) | array[1];
+ return 0;
+}
+
/*
* Read bytes from a given property of the given node. Any number of
* bytes of the property can be read. The fdt pointer is updated.
diff --git a/include/common/fdt_wrappers.h b/include/common/fdt_wrappers.h
index e3158f1c59..e28dee1423 100644
--- a/include/common/fdt_wrappers.h
+++ b/include/common/fdt_wrappers.h
@@ -12,8 +12,10 @@
/* Number of cells, given total length in bytes. Each cell is 4 bytes long */
#define NCELLS(len) ((len) / 4U)
-int fdtw_read_cells(const void *dtb, int node, const char *prop,
- unsigned int cells, void *value);
+int fdt_read_uint32(const void *dtb, int node, const char *prop_name,
+ uint32_t *value);
+int fdt_read_uint64(const void *dtb, int node, const char *prop_name,
+ uint64_t *value);
int fdt_read_uint32_array(const void *dtb, int node, const char *prop_name,
unsigned int cells, uint32_t *value);
int fdtw_read_string(const void *dtb, int node, const char *prop,
diff --git a/lib/fconf/fconf_dyn_cfg_getter.c b/lib/fconf/fconf_dyn_cfg_getter.c
index 317d3e5d30..03aaf9bb61 100644
--- a/lib/fconf/fconf_dyn_cfg_getter.c
+++ b/lib/fconf/fconf_dyn_cfg_getter.c
@@ -57,26 +57,32 @@ int fconf_populate_dtb_registry(uintptr_t config)
}
fdt_for_each_subnode(child, dtb, node) {
+ uint32_t val32;
+ uint64_t val64;
+
dtb_info = pool_alloc(&dtb_info_pool);
/* Read configuration dtb information */
- rc = fdtw_read_cells(dtb, child, "load-address", 2, &dtb_info->config_addr);
+ rc = fdt_read_uint64(dtb, child, "load-address", &val64);
if (rc < 0) {
ERROR("FCONF: Incomplete configuration property in dtb-registry.\n");
return rc;
}
+ dtb_info->config_addr = (uintptr_t)val64;
- rc = fdtw_read_cells(dtb, child, "max-size", 1, &dtb_info->config_max_size);
+ rc = fdt_read_uint32(dtb, child, "max-size", &val32);
if (rc < 0) {
ERROR("FCONF: Incomplete configuration property in dtb-registry.\n");
return rc;
}
+ dtb_info->config_max_size = val32;
- rc = fdtw_read_cells(dtb, child, "id", 1, &dtb_info->config_id);
+ rc = fdt_read_uint32(dtb, child, "id", &val32);
if (rc < 0) {
ERROR("FCONF: Incomplete configuration property in dtb-registry.\n");
return rc;
}
+ dtb_info->config_id = val32;
VERBOSE("FCONF: dyn_cfg.dtb_registry cell found with:\n");
VERBOSE("\tload-address = %lx\n", dtb_info->config_addr);
diff --git a/lib/fconf/fconf_tbbr_getter.c b/lib/fconf/fconf_tbbr_getter.c
index a4d61d8cde..21278019eb 100644
--- a/lib/fconf/fconf_tbbr_getter.c
+++ b/lib/fconf/fconf_tbbr_getter.c
@@ -17,6 +17,8 @@ int fconf_populate_tbbr_dyn_config(uintptr_t config)
{
int err;
int node;
+ uint64_t val64;
+ uint32_t val32;
/* As libfdt use void *, we can't avoid this cast */
const void *dtb = (void *)config;
@@ -30,7 +32,7 @@ int fconf_populate_tbbr_dyn_config(uintptr_t config)
}
/* Locate the disable_auth cell and read the value */
- err = fdtw_read_cells(dtb, node, "disable_auth", 1, &tbbr_dyn_config.disable_auth);
+ err = fdt_read_uint32(dtb, node, "disable_auth", &tbbr_dyn_config.disable_auth);
if (err < 0) {
WARN("FCONF: Read cell failed for `disable_auth`\n");
return err;
@@ -48,19 +50,19 @@ int fconf_populate_tbbr_dyn_config(uintptr_t config)
#endif
/* Retrieve the Mbed TLS heap details from the DTB */
- err = fdtw_read_cells(dtb, node,
- "mbedtls_heap_addr", 2, &tbbr_dyn_config.mbedtls_heap_addr);
+ err = fdt_read_uint64(dtb, node, "mbedtls_heap_addr", &val64);
if (err < 0) {
ERROR("FCONF: Read cell failed for mbedtls_heap\n");
return err;
}
+ tbbr_dyn_config.mbedtls_heap_addr = (void *)(uintptr_t)val64;
- err = fdtw_read_cells(dtb, node,
- "mbedtls_heap_size", 1, &tbbr_dyn_config.mbedtls_heap_size);
+ err = fdt_read_uint32(dtb, node, "mbedtls_heap_size", &val32);
if (err < 0) {
ERROR("FCONF: Read cell failed for mbedtls_heap\n");
return err;
}
+ tbbr_dyn_config.mbedtls_heap_size = val32;
VERBOSE("FCONF:tbbr.disable_auth cell found with value = %d\n",
tbbr_dyn_config.disable_auth);
diff --git a/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c b/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c
index bac1f1587a..44e9d0135f 100644
--- a/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c
+++ b/plat/arm/board/fvp/fconf/fconf_hw_config_getter.c
@@ -51,8 +51,9 @@ int fconf_populate_gicv3_config(uintptr_t config)
int fconf_populate_topology(uintptr_t config)
{
- int err, node, cluster_node, core_node, thread_node, max_pwr_lvl = 0;
+ int err, node, cluster_node, core_node, thread_node;
uint32_t cluster_count = 0, max_cpu_per_cluster = 0, total_cpu_count = 0;
+ uint32_t max_pwr_lvl = 0;
/* Necessary to work with libfdt APIs */
const void *hw_config_dtb = (const void *)config;
@@ -64,7 +65,7 @@ int fconf_populate_topology(uintptr_t config)
return node;
}
- err = fdtw_read_cells(hw_config_dtb, node, "max-pwr-lvl", 1, &max_pwr_lvl);
+ err = fdt_read_uint32(hw_config_dtb, node, "max-pwr-lvl", &max_pwr_lvl);
if (err < 0) {
/*
* Some legacy FVP dts may not have this property. Assign the default
@@ -74,7 +75,7 @@ int fconf_populate_topology(uintptr_t config)
max_pwr_lvl = 2;
}
- assert((uint32_t)max_pwr_lvl <= MPIDR_AFFLVL2);
+ assert(max_pwr_lvl <= MPIDR_AFFLVL2);
/* Find the offset of the "cpus" node */
node = fdt_path_offset(hw_config_dtb, "/cpus");
@@ -156,7 +157,7 @@ int fconf_populate_topology(uintptr_t config)
return -1;
}
- soc_topology.plat_max_pwr_level = (uint32_t)max_pwr_lvl;
+ soc_topology.plat_max_pwr_level = max_pwr_lvl;
soc_topology.plat_cluster_count = cluster_count;
soc_topology.cluster_cpu_count = max_cpu_per_cluster;
soc_topology.plat_cpu_count = total_cpu_count;
diff --git a/plat/arm/common/fconf/arm_fconf_sp.c b/plat/arm/common/fconf/arm_fconf_sp.c
index c46ecb1140..1b09bc8e50 100644
--- a/plat/arm/common/fconf/arm_fconf_sp.c
+++ b/plat/arm/common/fconf/arm_fconf_sp.c
@@ -29,6 +29,7 @@ int fconf_populate_arm_sp(uintptr_t config)
int sp_node, node, err;
union uuid_helper_t uuid_helper;
unsigned int index = 0;
+ uint32_t val32;
const unsigned int sp_start_index = MAX_NUMBER_IDS - MAX_SP_IDS;
/* As libfdt use void *, we can't avoid this cast */
@@ -53,12 +54,12 @@ int fconf_populate_arm_sp(uintptr_t config)
arm_sp.uuids[index] = uuid_helper;
- err = fdtw_read_cells(dtb, sp_node, "load-address", 1,
- &arm_sp.load_addr[index]);
+ err = fdt_read_uint32(dtb, sp_node, "load-address", &val32);
if (err < 0) {
ERROR("FCONF: cannot read SP load address\n");
return -1;
}
+ arm_sp.load_addr[index] = val32;
VERBOSE("FCONF: %s UUID %x-%x-%x-%x load_addr=%lx\n",
__func__,
diff --git a/plat/common/plat_spmd_manifest.c b/plat/common/plat_spmd_manifest.c
index f0aa27cfb0..8330356ae9 100644
--- a/plat/common/plat_spmd_manifest.c
+++ b/plat/common/plat_spmd_manifest.c
@@ -21,41 +21,43 @@ static int manifest_parse_attribute(spmc_manifest_sect_attribute_t *attr,
const void *fdt,
int node)
{
+ uint32_t val32;
int rc = 0;
assert(attr && fdt);
- rc = fdtw_read_cells(fdt, node, "maj_ver", 1, &attr->major_version);
+ rc = fdt_read_uint32(fdt, node, "maj_ver", &attr->major_version);
if (rc) {
ERROR("Missing SPCI major version in SPM core manifest.\n");
return -ENOENT;
}
- rc = fdtw_read_cells(fdt, node, "min_ver", 1, &attr->minor_version);
+ rc = fdt_read_uint32(fdt, node, "min_ver", &attr->minor_version);
if (rc) {
ERROR("Missing SPCI minor version in SPM core manifest.\n");
return -ENOENT;
}
- rc = fdtw_read_cells(fdt, node, "spmc_id", 1, &attr->spmc_id);
+ rc = fdt_read_uint32(fdt, node, "spmc_id", &val32);
if (rc) {
ERROR("Missing SPMC ID in manifest.\n");
return -ENOENT;
}
+ attr->spmc_id = val32;
- rc = fdtw_read_cells(fdt, node, "exec_state", 1, &attr->exec_state);
+ rc = fdt_read_uint32(fdt, node, "exec_state", &attr->exec_state);
if (rc)
NOTICE("Execution state not specified in SPM core manifest.\n");
- rc = fdtw_read_cells(fdt, node, "binary_size", 1, &attr->binary_size);
+ rc = fdt_read_uint32(fdt, node, "binary_size", &attr->binary_size);
if (rc)
NOTICE("Binary size not specified in SPM core manifest.\n");
- rc = fdtw_read_cells(fdt, node, "load_address", 2, &attr->load_address);
+ rc = fdt_read_uint64(fdt, node, "load_address", &attr->load_address);
if (rc)
NOTICE("Load address not specified in SPM core manifest.\n");
- rc = fdtw_read_cells(fdt, node, "entrypoint", 2, &attr->entrypoint);
+ rc = fdt_read_uint64(fdt, node, "entrypoint", &attr->entrypoint);
if (rc)
NOTICE("Entrypoint not specified in SPM core manifest.\n");