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

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(-)

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