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

[Simon Glass]

Hi Thierry,

On 20 March 2015 at 05:51, Thierry Reding <thierry.reding at gmail.com> wrote:
> From: Thierry Reding <treding at 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 at marvell.com>
> Cc: Simon Glass <sjg at chromium.org>
> Signed-off-by: Thierry Reding <treding at 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