The members "start" and "end" of struct resource are of type
"resource_size_t" which can be 32bit wide.
Values read from OF however are always 64bit wide.
Avoid silently truncating the value and instead return an error value.

This can happen on real systems when the DT was created for a
PAE-enabled kernel and a non-PAE kernel is actually running.
For example with an arm defconfig and "qemu-system-arm -M virt".

Link: https://bugs.launchpad.net/qemu/+bug/1790975
Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
Tested-by: Nam Cao <namcao@linutronix.de>
Reviewed-by: Nam Cao <namcao@linutronix.de>
---
 drivers/of/address.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index d669ce25b5f9..7e59283a4472 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -8,6 +8,7 @@
 #include <linux/logic_pio.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
+#include <linux/overflow.h>
 #include <linux/pci.h>
 #include <linux/pci_regs.h>
 #include <linux/sizes.h>
@@ -1061,7 +1062,11 @@ static int __of_address_to_resource(struct device_node *dev, int index, int bar_
 	if (of_mmio_is_nonposted(dev))
 		flags |= IORESOURCE_MEM_NONPOSTED;
 
+	if (overflows_type(taddr, r->start))
+		return -EOVERFLOW;
 	r->start = taddr;
+	if (overflows_type(taddr + size - 1, r->end))
+		return -EOVERFLOW;
 	r->end = taddr + size - 1;
 	r->flags = flags;
 	r->name = name ? name : dev->full_name;

---
base-commit: 67784a74e258a467225f0e68335df77acd67b7ab
change-id: 20240904-of-resource-overflow-378ea11dec8b

Best regards,
-- 
Thomas Weißschuh <thomas.weissschuh@linutronix.de>

On Thu, Sep 5, 2024 at 2:46 AM Thomas Weißschuh
<thomas.weissschuh@linutronix.de> wrote:
>
> The members "start" and "end" of struct resource are of type
> "resource_size_t" which can be 32bit wide.
> Values read from OF however are always 64bit wide.
> Avoid silently truncating the value and instead return an error value.
>
> This can happen on real systems when the DT was created for a
> PAE-enabled kernel and a non-PAE kernel is actually running.
> For example with an arm defconfig and "qemu-system-arm -M virt".

A nice follow-up would be to make of_pci_range_to_resource() use
overflows_type() as well instead of open coding it.

> Link: https://bugs.launchpad.net/qemu/+bug/1790975
> Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de>
> Tested-by: Nam Cao <namcao@linutronix.de>
> Reviewed-by: Nam Cao <namcao@linutronix.de>
> ---
>  drivers/of/address.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index d669ce25b5f9..7e59283a4472 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -8,6 +8,7 @@
>  #include <linux/logic_pio.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
> +#include <linux/overflow.h>
>  #include <linux/pci.h>
>  #include <linux/pci_regs.h>
>  #include <linux/sizes.h>
> @@ -1061,7 +1062,11 @@ static int __of_address_to_resource(struct device_node *dev, int index, int bar_
>         if (of_mmio_is_nonposted(dev))
>                 flags |= IORESOURCE_MEM_NONPOSTED;
>
> +       if (overflows_type(taddr, r->start))
> +               return -EOVERFLOW;
>         r->start = taddr;

It looks odd that "r->start" is used before it is set, but I guess
overflows_type isn't using the value and the compiler would warn
otherwise.

Applied, thanks.

Rob

On Thu, Sep 05, 2024 at 08:15:40AM GMT, Rob Herring wrote:
> On Thu, Sep 5, 2024 at 2:46 AM Thomas Weißschuh
> <thomas.weissschuh@linutronix.de> wrote:
> >
> > The members "start" and "end" of struct resource are of type
> > "resource_size_t" which can be 32bit wide.
> > Values read from OF however are always 64bit wide.
> > Avoid silently truncating the value and instead return an error value.
> >
> > This can happen on real systems when the DT was created for a
> > PAE-enabled kernel and a non-PAE kernel is actually running.
> > For example with an arm defconfig and "qemu-system-arm -M virt".
> 
> A nice follow-up would be to make of_pci_range_to_resource() use
> overflows_type() as well instead of open coding it.

Good catch.

There are some differences though, it
* returns -EINVAL on overflow instead of -EOVERFLOW
* sets ->start and ->end to OF_BAD_ADDR on overflow
* does not check ->end for overflow

I don't have much experience with OF, so I'm not sure which of these are
important and if overflow checks on intermediate calculations are also
necessary.

On Thu, Sep 5, 2024 at 8:41 AM Thomas Weißschuh
<thomas.weissschuh@linutronix.de> wrote:
>
> On Thu, Sep 05, 2024 at 08:15:40AM GMT, Rob Herring wrote:
> > On Thu, Sep 5, 2024 at 2:46 AM Thomas Weißschuh
> > <thomas.weissschuh@linutronix.de> wrote:
> > >
> > > The members "start" and "end" of struct resource are of type
> > > "resource_size_t" which can be 32bit wide.
> > > Values read from OF however are always 64bit wide.
> > > Avoid silently truncating the value and instead return an error value.
> > >
> > > This can happen on real systems when the DT was created for a
> > > PAE-enabled kernel and a non-PAE kernel is actually running.
> > > For example with an arm defconfig and "qemu-system-arm -M virt".
> >
> > A nice follow-up would be to make of_pci_range_to_resource() use
> > overflows_type() as well instead of open coding it.
>
> Good catch.
>
> There are some differences though, it
> * returns -EINVAL on overflow instead of -EOVERFLOW

I think that is safe to change. I don't see any cases looking at the
specific errno. Note that of_range_to_resource() kerneldoc would need
updating too.

> * sets ->start and ->end to OF_BAD_ADDR on overflow

Don't need to do that. No user accesses the resource on error.

> * does not check ->end for overflow

Obviously we want to do that.

Rob