On Fri, Jan 17, 2025 at 12:23:53PM +0530, Basharath Hussain Khaja wrote:
> >> >> Thomas Weißschuh <thomas.weisssc...@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.weisssc...@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.
> >>
> >>
> >> This commit is breaking Ethernet functionality on the TI AM57xx platform
> >> due to
> >> zero byte SRAM block size allocation during initialization. Prior to this
> >> patch, zero byte block sizes were handled properly.
> >
> > What driver and where exactly?
>
> We found an issue while developing the driver [1] and more
> specifically in [2] (lines 313-327), but it looks like this is a
> generic issue which can block 1 byte of memory, when a zero size
> request has been initiated for the reserved region.
>
> 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 && 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;
> }
>
> Though we have the start address handling already in place above, we
> do see an issue with the end address, because there is an
> unconditional +1 afterwards in resource_size() API below which is
> responsible for reserving the extra byte
>
> static inline resource_size_t resource_size(const struct resource *res)
> {
> return res->end - res->start + 1;
> }
Now the report makes more sense.
> We have 4 ways of fixing it.
>
> Option 1: Modify the function to handle the size zero case
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index c1f1c810e810..8db6ae9a12b8 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -204,6 +204,12 @@ static int __of_address_resource_bounds(struct resource
> *r, u64 start, u64 size)
>
> if (overflows_type(start, r->start))
> return -EOVERFLOW;
> + if (!size) {
> + r->start = start;
> + r->end = end - 1;
> +
> + return 0;
> + }
> if (size && check_add_overflow(end, size - 1, &end))
> return -EOVERFLOW;
> if (overflows_type(end, r->end))
>
> This seems to be the simplest solution.
Fixing it in __of_address_resource_bounds() looks correct to me.
The proposed solution doesn't look as clean as I'd like though,
this is highly subjective, though.
What about the following (untested)?
static int __of_address_resource_bounds(struct resource *r, u64 start, u64 size)
{
if (overflows_type(start, r->start))
return -EOVERFLOW;
r->start = start;
r->end = start;
if (!size)
r->end -= 1; /* May underflow for empty resources. */
else if (check_add_overflow(r->end, size - 1, &r->end))
return -EOVERFLOW;
return 0;
}
A kunit test looks to be in order in any case, to make sure all the
edgecases are handled.
> Option 2: Handle in resource_size().
> static inline resource_size_t resource_size(const struct resource *res)
> {
> return (res->end == res->start) ? 0 : (res->end - res->start + 1);
> }
> There are plenty of places where we are using this API and there is an
> assumption that the end address should always be start + size -1. We are a
> bit unsure about the side effects of this change.
>
> Option 3: Handle in sram_reserve_region().
> We can avoid calling the resource_size() API and handle size zero as a
> special case. We are a bit unsure about the side effects of this change as
> well.
>
> Option 4: Handle this in dts [2] with non zero size. Estimate the approximate
> size and update that value in dts file with extra buffer. However, as
> indicated in [2] in lines 313-327, the size is not known apriori and the
> actual size is only known in runtime. So if we set some size for this buffer,
> then this will always be blocked and may or may not be used subsequently.
>
> [1] https://lore.kernel.org/all/20250109105600.41297-1-bashar...@couthit.com/
> [2]
> https://github.com/torvalds/linux/blob/master/arch/arm/boot/dts/ti/omap/dra7.dtsi
>
>
> Thanks & Best Regards,
> Basharath
