Re: [U-Boot] [PATCH 2/3] fdt: Fix fdtdec_get_addr_size() for 64-bit

Hi Thierry,

On 20 March 2015 at 05:51, Thierry Reding thierry.reding@gmail.com wrote:

From: Thierry Reding treding@nvidia.com

The current implementation of fdtdec_get_addr_size() assumes that the sizes of fdt_addr_t and fdt_size_t match the number of cells specified by the #address-cells and #size-cells properties. However, there is no reason why that needs to be the case, so the function potentially decodes address and size wrongly.

Furthermore the function casts an array of FDT cells (32-bit unsigned) to fdt_addr_t/fdt_size_t and dereferences them directly. This can cause aborts on 64-bit architectures where fdt_addr_t/fdt_size_t are 64 bits wide, because cells are only guaranteed to be aligned to 32 bits. Fix this by reading in the address and size one cell at a time.

Cc: Hanna Hawa hannah@marvell.com Cc: Simon Glass sjg@chromium.org Signed-off-by: Thierry Reding treding@nvidia.com

---

lib/fdtdec.c | 56 ++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 20 deletions(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 613cc8494a74..fd244440381e 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -89,29 +89,45 @@ const char *fdtdec_get_compatible(enum fdt_compat_id id) fdt_addr_t fdtdec_get_addr_size(const void *blob, int node, const char *prop_name, fdt_size_t *sizep) {

•   const fdt_addr_t *cell;
  

•   int len;
  

•   const fdt32_t *ptr, *end;
  

•   int parent, na, ns, len;
  

•   fdt_addr_t addr;
  
    debug("%s: %s: ", __func__, prop_name);
  

•   cell = fdt_getprop(blob, node, prop_name, &len);
  

•   if (cell && ((!sizep && len == sizeof(fdt_addr_t)) ||
  

•                len == sizeof(fdt_addr_t) * 2)) {
  

•           fdt_addr_t addr = fdt_addr_to_cpu(*cell);
  

•           if (sizep) {
  

•                   const fdt_size_t *size;
  

•                   size = (fdt_size_t *)((char *)cell +
  

•                                   sizeof(fdt_addr_t));
  

•                   *sizep = fdt_size_to_cpu(*size);
  

•                   debug("addr=%08lx, size=%08lx\n",
  

•                         (ulong)addr, (ulong)*sizep);
  

•           } else {
  

•                   debug("%08lx\n", (ulong)addr);
  

•           }
  

•           return addr;
  

•   parent = fdt_parent_offset(blob, node);
  

This function is very slow as it must scan the whole tree. Can we instead pass in the parent node? Also, how about (in addition) a version of this function that works for devices? Like:

device_get_addr_size(struct udevice *dev, ...)

so that it can handle this for you.

•   if (parent < 0) {
  

•           debug("(no parent found)\n");
  

•           return FDT_ADDR_T_NONE;
    }
  

•   debug("(not found)\n");
  

•   return FDT_ADDR_T_NONE;
  

•   na = fdt_address_cells(blob, parent);
  

•   ns = fdt_size_cells(blob, parent);
  

•   ptr = fdt_getprop(blob, node, prop_name, &len);
  

•   if (!ptr) {
  

•           debug("(not found)\n");
  

•           return FDT_ADDR_T_NONE;
  

•   }
  

•   end = ptr + len / sizeof(*ptr);
  

•   if (ptr + na + ns > end) {
  

•           debug("(not enough data: expected %d bytes, got %d bytes)\n",
  

•                 (na + ns) * 4, len);
  

•           return FDT_ADDR_T_NONE;
  

•   }
  

•   addr = fdtdec_get_number(ptr, na);
  

•   if (sizep) {
  

•           *sizep = fdtdec_get_number(ptr + na, ns);
  

•           debug("addr=%pa, size=%pa\n", &addr, sizep);
  

•   } else {
  

•           debug("%pa\n", &addr);
  

•   }
  

•   return addr;
  

}

fdt_addr_t fdtdec_get_addr(const void *blob, int node,

2.3.2

Regards, Simon