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: <c5e7e8a3-2e8e-4d68-8e06-a7a3f7fc451e@nvidia.com>
Date: Tue, 4 Feb 2025 17:18:46 +0000
From: Jon Hunter <jonathanh@...dia.com>
To: Thierry Reding <thierry.reding@...il.com>,
 Mohan Kumar D <mkumard@...dia.com>
Cc: vkoul@...nel.org, 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 04/02/2025 17:03, Thierry Reding wrote:
> 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?

You mean a DMA register space that crosses a 4GB address boundary? I 
would hope not but maybe I should not assume that!

> 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);


We were trying to avoid the div_u64 because at some point we want to 
convert the result to 32-bits to avoid any further 64-bit math and we 
really don't need 64-bits for the page number.

Jon

--
nvpublic