| Message ID | 20240906-of-address-overflow-v1-1-19567aaa61da@linutronix.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | of: address: Unify resource bounds overflow checking | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
On Fri, 06 Sep 2024 14:25:19 +0200, Thomas Weißschuh 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. > > Refactor the diff overflow checks into a helper function. > Also extend the checks to validate each calculation step. > > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de> > --- > drivers/of/address.c | 45 ++++++++++++++++++++++++++------------------- > 1 file changed, 26 insertions(+), 19 deletions(-) > Applied, thanks!
Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes: > 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. > > Refactor the diff overflow checks into a helper function. > Also extend the checks to validate each calculation step. > > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de> > --- > drivers/of/address.c | 45 ++++++++++++++++++++++++++------------------- > 1 file changed, 26 insertions(+), 19 deletions(-) > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 7e59283a4472..df854bb427ce 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -198,6 +198,25 @@ static u64 of_bus_pci_map(__be32 *addr, const __be32 *range, int na, int ns, > > #endif /* CONFIG_PCI */ > > +static int __of_address_resource_bounds(struct resource *r, u64 start, u64 size) > +{ > + u64 end = start; > + > + if (overflows_type(start, r->start)) > + return -EOVERFLOW; > + if (size == 0) > + return -EOVERFLOW; > + if (check_add_overflow(end, size - 1, &end)) > + return -EOVERFLOW; > + if (overflows_type(end, r->end)) > + return -EOVERFLOW; This breaks PCI on powerpc qemu. Part of the PCI probe reads a resource that's zero sized, which used to succeed but now fails due to the size check above. The diff below fixes it for me. It leaves r.end == r.start, which is fine in my case, because the code only uses r.start. And it seems more sane than the old code which would return end = start - 1, for zero sized resources. cheers diff --git a/drivers/of/address.c b/drivers/of/address.c index df854bb427ce..a001e789a6c4 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -204,9 +204,7 @@ static int __of_address_resource_bounds(struct resource *r, u64 start, u64 size) if (overflows_type(start, r->start)) return -EOVERFLOW; - if (size == 0) - return -EOVERFLOW; - if (check_add_overflow(end, size - 1, &end)) + if (size > 0 && check_add_overflow(end, size - 1, &end)) return -EOVERFLOW; if (overflows_type(end, r->end)) return -EOVERFLOW;
On Fri, Sep 13, 2024 at 8:15 AM Michael Ellerman <mpe@ellerman.id.au> wrote: > > Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes: > > 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. > > > > Refactor the diff overflow checks into a helper function. > > Also extend the checks to validate each calculation step. > > > > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de> > > --- > > drivers/of/address.c | 45 ++++++++++++++++++++++++++------------------- > > 1 file changed, 26 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > index 7e59283a4472..df854bb427ce 100644 > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -198,6 +198,25 @@ static u64 of_bus_pci_map(__be32 *addr, const __be32 *range, int na, int ns, > > > > #endif /* CONFIG_PCI */ > > > > +static int __of_address_resource_bounds(struct resource *r, u64 start, u64 size) > > +{ > > + u64 end = start; > > + > > + if (overflows_type(start, r->start)) > > + return -EOVERFLOW; > > + if (size == 0) > > + return -EOVERFLOW; > > + if (check_add_overflow(end, size - 1, &end)) > > + return -EOVERFLOW; > > + if (overflows_type(end, r->end)) > > + return -EOVERFLOW; > > This breaks PCI on powerpc qemu. Part of the PCI probe reads a resource > that's zero sized, which used to succeed but now fails due to the size > check above. > > The diff below fixes it for me. I fixed it up with your change. > It leaves r.end == r.start, which is fine in my case, because the code > only uses r.start. > > And it seems more sane than the old code which would return > end = start - 1, for zero sized resources. > > cheers > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index df854bb427ce..a001e789a6c4 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -204,9 +204,7 @@ static int __of_address_resource_bounds(struct resource *r, u64 start, u64 size) > > if (overflows_type(start, r->start)) > return -EOVERFLOW; > - if (size == 0) > - return -EOVERFLOW; > - if (check_add_overflow(end, size - 1, &end)) > + if (size > 0 && check_add_overflow(end, size - 1, &end)) > return -EOVERFLOW; > if (overflows_type(end, r->end)) > return -EOVERFLOW; >
Rob Herring <robh@kernel.org> writes: > On Fri, Sep 13, 2024 at 8:15 AM Michael Ellerman <mpe@ellerman.id.au> wrote: >> Thomas Weißschuh <thomas.weissschuh@linutronix.de> writes: >> > 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. >> > >> > Refactor the diff overflow checks into a helper function. >> > Also extend the checks to validate each calculation step. >> > >> > Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de> >> > --- >> > drivers/of/address.c | 45 ++++++++++++++++++++++++++------------------- >> > 1 file changed, 26 insertions(+), 19 deletions(-) >> > >> > diff --git a/drivers/of/address.c b/drivers/of/address.c >> > index 7e59283a4472..df854bb427ce 100644 >> > --- a/drivers/of/address.c >> > +++ b/drivers/of/address.c >> > @@ -198,6 +198,25 @@ static u64 of_bus_pci_map(__be32 *addr, const __be32 *range, int na, int ns, >> > >> > #endif /* CONFIG_PCI */ >> > >> > +static int __of_address_resource_bounds(struct resource *r, u64 start, u64 size) >> > +{ >> > + u64 end = start; >> > + >> > + if (overflows_type(start, r->start)) >> > + return -EOVERFLOW; >> > + if (size == 0) >> > + return -EOVERFLOW; >> > + if (check_add_overflow(end, size - 1, &end)) >> > + return -EOVERFLOW; >> > + if (overflows_type(end, r->end)) >> > + return -EOVERFLOW; >> >> This breaks PCI on powerpc qemu. Part of the PCI probe reads a resource >> that's zero sized, which used to succeed but now fails due to the size >> check above. >> >> The diff below fixes it for me. > > I fixed it up with your change. Thanks. cheers
diff --git a/drivers/of/address.c b/drivers/of/address.c index 7e59283a4472..df854bb427ce 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -198,6 +198,25 @@ static u64 of_bus_pci_map(__be32 *addr, const __be32 *range, int na, int ns, #endif /* CONFIG_PCI */ +static int __of_address_resource_bounds(struct resource *r, u64 start, u64 size) +{ + u64 end = start; + + if (overflows_type(start, r->start)) + return -EOVERFLOW; + if (size == 0) + return -EOVERFLOW; + if (check_add_overflow(end, size - 1, &end)) + return -EOVERFLOW; + if (overflows_type(end, r->end)) + return -EOVERFLOW; + + r->start = start; + r->end = end; + + return 0; +} + /* * of_pci_range_to_resource - Create a resource from an of_pci_range * @range: the PCI range that describes the resource @@ -216,6 +235,7 @@ static u64 of_bus_pci_map(__be32 *addr, const __be32 *range, int na, int ns, int of_pci_range_to_resource(struct of_pci_range *range, struct device_node *np, struct resource *res) { + u64 start; int err; res->flags = range->flags; res->parent = res->child = res->sibling = NULL; @@ -232,18 +252,11 @@ int of_pci_range_to_resource(struct of_pci_range *range, err = -EINVAL; goto invalid_range; } - res->start = port; + start = port; } else { - if ((sizeof(resource_size_t) < 8) && - upper_32_bits(range->cpu_addr)) { - err = -EINVAL; - goto invalid_range; - } - - res->start = range->cpu_addr; + start = range->cpu_addr; } - res->end = res->start + range->size - 1; - return 0; + return __of_address_resource_bounds(res, start, range->size); invalid_range: res->start = (resource_size_t)OF_BAD_ADDR; @@ -259,8 +272,8 @@ EXPORT_SYMBOL(of_pci_range_to_resource); * @res: pointer to a valid resource that will be updated to * reflect the values contained in the range. * - * Returns ENOENT if the entry is not found or EINVAL if the range cannot be - * converted to resource. + * Returns -ENOENT if the entry is not found or -EOVERFLOW if the range + * cannot be converted to resource. */ int of_range_to_resource(struct device_node *np, int index, struct resource *res) { @@ -1062,16 +1075,10 @@ 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; - return 0; + return __of_address_resource_bounds(r, taddr, size); } /**
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. Refactor the diff overflow checks into a helper function. Also extend the checks to validate each calculation step. Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de> --- drivers/of/address.c | 45 ++++++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 19 deletions(-) --- base-commit: 20e5e74b91e99d57059d783e910d08a65573458f change-id: 20240906-of-address-overflow-06b16d785ba7 Best regards,