linux-kernel - Re: [PATCH v3 1/2] dmaengine: tegra210-adma: Fix build error due to 64-by-32 division

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <52n364alceto6tgitbnjbfgtrk2lpe5ipztxi4abnuikjwgnvk@i6irrkj6fqbb>
Date: Tue, 4 Feb 2025 18:03:22 +0100
From: Thierry Reding <thierry.reding@...il.com>
To: Mohan Kumar D <mkumard@...dia.com>
Cc: vkoul@...nel.org, jonathanh@...dia.com, dmaengine@...r.kernel.org, 
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org, stable@...r.kernel.org, 
	kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v3 1/2] dmaengine: tegra210-adma: Fix build error due to
 64-by-32 division

On Tue, Feb 04, 2025 at 10:13:09PM +0530, Mohan Kumar D wrote:
> 
> On 04-02-2025 21:06, Thierry Reding wrote:
> > On Thu, Jan 16, 2025 at 09:50:32PM +0530, Mohan Kumar D wrote:
> > > Kernel test robot reported the build errors on 32-bit platforms due to
> > > plain 64-by-32 division. Following build erros were reported.
> > > 
> > >     "ERROR: modpost: "__udivdi3" [drivers/dma/tegra210-adma.ko] undefined!
> > >      ld: drivers/dma/tegra210-adma.o: in function `tegra_adma_probe':
> > >      tegra210-adma.c:(.text+0x12cf): undefined reference to `__udivdi3'"
> > > 
> > > This can be fixed by using lower_32_bits() for the adma address space as
> > > the offset is constrained to the lower 32 bits
> > > 
> > > Fixes: 68811c928f88 ("dmaengine: tegra210-adma: Support channel page")
> > > Cc: stable@...r.kernel.org
> > > Reported-by: kernel test robot <lkp@...el.com>
> > > Closes: https://lore.kernel.org/oe-kbuild-all/202412250204.GCQhdKe3-lkp@intel.com/
> > > Signed-off-by: Mohan Kumar D <mkumard@...dia.com>
> > > ---
> > >   drivers/dma/tegra210-adma.c | 14 +++++++++++---
> > >   1 file changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/dma/tegra210-adma.c b/drivers/dma/tegra210-adma.c
> > > index 6896da8ac7ef..258220c9cb50 100644
> > > --- a/drivers/dma/tegra210-adma.c
> > > +++ b/drivers/dma/tegra210-adma.c
> > > @@ -887,7 +887,8 @@ static int tegra_adma_probe(struct platform_device *pdev)
> > >   	const struct tegra_adma_chip_data *cdata;
> > >   	struct tegra_adma *tdma;
> > >   	struct resource *res_page, *res_base;
> > > -	int ret, i, page_no;
> > > +	unsigned int page_no, page_offset;
> > > +	int ret, i;
> > >   	cdata = of_device_get_match_data(&pdev->dev);
> > >   	if (!cdata) {
> > > @@ -914,9 +915,16 @@ static int tegra_adma_probe(struct platform_device *pdev)
> > >   		res_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "global");
> > >   		if (res_base) {
> > > -			page_no = (res_page->start - res_base->start) / cdata->ch_base_offset;
> > > -			if (page_no <= 0)
> > > +			if (WARN_ON(lower_32_bits(res_page->start) <=
> > > +						lower_32_bits(res_base->start)))
> > Don't we technically also want to check that
> > 
> > 	res_page->start <= res_base->start
> > 
> > because otherwise people might put in something that's completely out of
> > range? I guess maybe you could argue that the DT is then just broken,
> > but since we're checking anyway, might as well check for all corner
> > cases.
> > 
> > Thierry
> ADMA Address range for all Tegra chip falls within 32bit range. Do you think
> still we need to have this extra check which seems like redundant for now.

No, you're right. If this is all within the lower 32 bit range, this
should be plenty enough. It might be worth to make it a bit more
explicit and store these values in variables and add a comment as to
why we only need the 32 bits. That would also make the code a bit
easier to read by making the lines shorter.

	// memory regions are guaranteed to be within the lower 4 GiB
	u32 base = lower_32_bits(res_base->start);
	u32 page = lower_32_bits(res_page->start);

	if (WARN_ON(page <= base))
		...

etc.

Hm... on the other hand. Do we know that it's always going to stay that
way? What if we ever get a chip that has a very different address map?

Maybe we can do a combination of Arnd's patch and this. In conjunction
with your second patch here, this could become something along these
lines:

	u64 offset, page;

	if (WARN_ON(res_page->start <= res_base->start))
		return -EINVAL;

	offset = res_page->start - res_base->start;
	page = div_u64(offset, cdata->ch_base_offset);

	if (WARN_ON(page == 0 || page > cdata->max_page))
		return -EINVAL;

Thierry

> > 
> > > +				return -EINVAL;
> > > +
> > > +			page_offset = lower_32_bits(res_page->start) -
> > > +						lower_32_bits(res_base->start);
> > > +			page_no = page_offset / cdata->ch_base_offset;
> > > +			if (page_no == 0)
> > >   				return -EINVAL;
> > > +
> > >   			tdma->ch_page_no = page_no - 1;
> > >   			tdma->base_addr = devm_ioremap_resource(&pdev->dev, res_base);
> > >   			if (IS_ERR(tdma->base_addr))
> > > -- 
> > > 2.25.1
> > > 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)