Hi Thierry,
On 20 March 2015 at 05:51, Thierry Reding <thierry.red...@gmail.com> wrote:
> From: Thierry Reding <tred...@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 <han...@marvell.com>
> Cc: Simon Glass <s...@chromium.org>
> Signed-off-by: Thierry Reding <tred...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
