* [PATCH] fdt: Fix bounds check in devfdt_get_addr_index
@ 2022-10-31 3:41 Samuel Holland
2022-10-31 19:27 ` Simon Glass
0 siblings, 1 reply; 5+ messages in thread
From: Samuel Holland @ 2022-10-31 3:41 UTC (permalink / raw)
To: Simon Glass; +Cc: Samuel Holland, Jagan Teki, Mugunthan V N, u-boot
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(-)
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;
}
--
2.37.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] fdt: Fix bounds check in devfdt_get_addr_index
2022-10-31 3:41 [PATCH] fdt: Fix bounds check in devfdt_get_addr_index Samuel Holland
@ 2022-10-31 19:27 ` Simon Glass
2022-11-07 23:35 ` Simon Glass
0 siblings, 1 reply; 5+ messages in thread
From: Simon Glass @ 2022-10-31 19:27 UTC (permalink / raw)
To: Samuel Holland; +Cc: Jagan Teki, Mugunthan V N, u-boot
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>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fdt: Fix bounds check in devfdt_get_addr_index
2022-10-31 19:27 ` Simon Glass
@ 2022-11-07 23:35 ` Simon Glass
2022-11-18 4:00 ` Samuel Holland
0 siblings, 1 reply; 5+ messages in thread
From: Simon Glass @ 2022-11-07 23:35 UTC (permalink / raw)
To: Samuel Holland; +Cc: Jagan Teki, Mugunthan V N, u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fdt: Fix bounds check in devfdt_get_addr_index
2022-11-07 23:35 ` Simon Glass
@ 2022-11-18 4:00 ` Samuel Holland
2022-11-18 20:50 ` Simon Glass
0 siblings, 1 reply; 5+ messages in thread
From: Samuel Holland @ 2022-11-18 4:00 UTC (permalink / raw)
To: Simon Glass; +Cc: Jagan Teki, Mugunthan V N, u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fdt: Fix bounds check in devfdt_get_addr_index
2022-11-18 4:00 ` Samuel Holland
@ 2022-11-18 20:50 ` Simon Glass
0 siblings, 0 replies; 5+ messages in thread
From: Simon Glass @ 2022-11-18 20:50 UTC (permalink / raw)
To: Samuel Holland; +Cc: Jagan Teki, Mugunthan V N, u-boot
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-18 20:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-31 3:41 [PATCH] fdt: Fix bounds check in devfdt_get_addr_index Samuel Holland
2022-10-31 19:27 ` Simon Glass
2022-11-07 23:35 ` Simon Glass
2022-11-18 4:00 ` Samuel Holland
2022-11-18 20:50 ` Simon Glass
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).