| Message ID | 20240905-of-resource-overflow-v1-1-0cd8bb92cc1f@linutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | of: address: Report error on resource bounds overflow | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
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
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;