[PATCH] fdt: Fix bounds check in devfdt_get_addr_index

[Simon Glass]

Hi Samuel,

On Thu, 17 Nov 2022 at 21:00, Samuel Holland <samuel at sholland.org> wrote:
>
> Hi Simon,
>
> On 11/7/22 17:35, Simon Glass wrote:
> > Hi Samuel,
> >
> > On Mon, 31 Oct 2022 at 13:27, Simon Glass <sjg at chromium.org> wrote:
> >>
> >> On Sun, 30 Oct 2022 at 21:41, Samuel Holland <samuel at sholland.org> wrote:
> >>>
> >>> reg must contain enough cells for the entire next address/size pair
> >>> after skipping `index` pairs. The previous code allows an out-of-bounds
> >>> read when na + ns > 1.
> >>>
> >>> Fixes: 69b41388ba45 ("dm: core: Add a new api to get indexed device address")
> >>> Signed-off-by: Samuel Holland <samuel at sholland.org>
> >>> ---
> >>>
> >>>  drivers/core/fdtaddr.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> Reviewed-by: Simon Glass <sjg at chromium.org>
> >
> > Somehow this break PPC in CI:
> >
> > https://source.denx.de/u-boot/custodians/u-boot-dm/-/jobs/524776
> >
> > Can you please take a look?
>
> The cause is the call to lists_bind_fdt() inside serial_check_stdout(),
> which sets the UART's parent device to gd->dm_root, when the real parent
> should be a simple-bus node with a ranges property.
>
> Then when we go to probe the UART, devfdt_get_addr_index() does:
>
>         int parent = dev_of_offset(dev->parent);
>         // ...
>         na = fdt_address_cells(gd->fdt_blob, parent);
>
> So devfdt_get_addr_index() sees the wrong number of address/size cells
> (2 instead of 1) and the check fails. This only worked previously
> because the check in devfdt_get_addr_index() would always succeed for
> index == 0.
>
> This patch fixes things, by setting the UART's parent device correctly:
>
> --- a/drivers/serial/ns16550.c
> +++ b/drivers/serial/ns16550.c
> @@ -623,9 +623,7 @@
>         .priv_auto      = sizeof(struct ns16550),
>         .probe = ns16550_serial_probe,
>         .ops    = &ns16550_serial_ops,
> -#if !CONFIG_IS_ENABLED(OF_CONTROL)
>         .flags  = DM_FLAG_PRE_RELOC,
> -#endif

We should put the dm tag in the device tree node instead.

>  };
>
>  DM_DRIVER_ALIAS(ns16550_serial, ti_da830_uart)
>
> Or maybe devfdt_get_addr_index() should be looking at the FDT node
> parent, instead of the DM parent. This also fixes things:
>
> --- a/drivers/core/fdtaddr.c
> +++ b/drivers/core/fdtaddr.c
> @@ -22,7 +22,7 @@
>  {
>  #if CONFIG_IS_ENABLED(OF_REAL)
>         int offset = dev_of_offset(dev);
> -       int parent = dev_of_offset(dev->parent);
> +       int parent = fdt_parent_offset(gd->fdt_blob, offset);

That is slow, best avoided.


>         fdt_addr_t addr;
>
>         if (CONFIG_IS_ENABLED(OF_TRANSLATE)) {
Regards,
Simon