| Message ID | 20221031034120.26159-1-samuel@sholland.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | fdt: Fix bounds check in devfdt_get_addr_index | expand |
On Sun, 30 Oct 2022 at 21:41, Samuel Holland <samuel@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@sholland.org> > --- > > drivers/core/fdtaddr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Simon Glass <sjg@chromium.org>
Hi Samuel, On Mon, 31 Oct 2022 at 13:27, Simon Glass <sjg@chromium.org> wrote: > > On Sun, 30 Oct 2022 at 21:41, Samuel Holland <samuel@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@sholland.org> > > --- > > > > drivers/core/fdtaddr.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > Reviewed-by: Simon Glass <sjg@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? Regards, SImon
Hi Simon, On 11/7/22 17:35, Simon Glass wrote: > Hi Samuel, > > On Mon, 31 Oct 2022 at 13:27, Simon Glass <sjg@chromium.org> wrote: >> >> On Sun, 30 Oct 2022 at 21:41, Samuel Holland <samuel@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@sholland.org> >>> --- >>> >>> drivers/core/fdtaddr.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> Reviewed-by: Simon Glass <sjg@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 }; 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); fdt_addr_t addr; if (CONFIG_IS_ENABLED(OF_TRANSLATE)) { Regards, Samuel
Hi Samuel, On Thu, 17 Nov 2022 at 21:00, Samuel Holland <samuel@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@chromium.org> wrote: > >> > >> On Sun, 30 Oct 2022 at 21:41, Samuel Holland <samuel@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@sholland.org> > >>> --- > >>> > >>> drivers/core/fdtaddr.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> Reviewed-by: Simon Glass <sjg@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
diff --git a/drivers/core/fdtaddr.c b/drivers/core/fdtaddr.c index 91bcd1a2c2..50ea05263e 100644 --- a/drivers/core/fdtaddr.c +++ b/drivers/core/fdtaddr.c @@ -43,7 +43,7 @@ fdt_addr_t devfdt_get_addr_index(const struct udevice *dev, int index) } reg = fdt_getprop(gd->fdt_blob, offset, "reg", &len); - if (!reg || (len <= (index * sizeof(fdt32_t) * (na + ns)))) { + if (!reg || (len < ((index + 1) * sizeof(fdt32_t) * (na + ns)))) { debug("Req index out of range\n"); return FDT_ADDR_T_NONE; }
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@sholland.org> --- drivers/core/fdtaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)