Re: [PATCH 2/2] ARM: Clamp MAX_DMA_ADDRESS to 32-bit

From: Florian Fainelli <f.fainelli@gmail.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>,
	Santosh Shilimkar <ssantosh@kernel.org>
Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Russell King <linux@armlinux.org.uk>,
	Nicolas Pitre <nico@fluxnic.net>,
	Catalin Marinas <catalin.marinas@arm.com>,
	open list <linux-kernel@vger.kernel.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [PATCH 2/2] ARM: Clamp MAX_DMA_ADDRESS to 32-bit
Date: Thu, 24 Mar 2022 15:39:36 -0700	[thread overview]
Message-ID: <145fa83e-c4c8-ec0b-4626-8f99655174ba@gmail.com> (raw)
In-Reply-To: <CAMuHMdWyDLoOit-VYPzLXxLz6W300xpo_wrqj=Pr9sZSwk_OwQ@mail.gmail.com>

On 3/24/22 13:31, Geert Uytterhoeven wrote:
> Hi Florian,
> 
> On Thu, Mar 24, 2022 at 6:54 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>> MAX_DMA_ADDRESS is a virtual address, therefore it needs to fit within a
>> 32-bit unsigned quantity. Platforms defining a DMA zone size in
>> their machine descriptor can easily overflow this quantity depending on
>> the DMA zone size and/or the PAGE_OFFSET setting.
>>
>> In most cases this is harmless, however in the case of a
>> CONFIG_DEBUG_VIRTUAL enabled, __virt_addr_valid() will be unable to
>> return that MAX_DMA_ADDRESS is valid because the value passed to that
>> function is an unsigned long which has already overflowed.
>>
>> Fixes: e377cd8221eb ("ARM: 8640/1: Add support for CONFIG_DEBUG_VIRTUAL")
>> Fixes: 2fb3ec5c9503 ("ARM: Replace platform definition of ISA_DMA_THRESHOLD/MAX_DMA_ADDRESS")
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> 
>> --- a/arch/arm/include/asm/dma.h
>> +++ b/arch/arm/include/asm/dma.h
>> @@ -8,10 +8,12 @@
>>   #ifndef CONFIG_ZONE_DMA
>>   #define MAX_DMA_ADDRESS        0xffffffffUL
>>   #else
>> +#include <linux/minmax.h>
>>   #define MAX_DMA_ADDRESS        ({ \
>>          extern phys_addr_t arm_dma_zone_size; \
>>          arm_dma_zone_size && arm_dma_zone_size < (0x10000000 - PAGE_OFFSET) ? \
> 
> "arm_dma_zone_size < (0x10000000 - PAGE_OFFSET)" looks
> like an overflow-avoiding version of
> "PAGE_OFFSET + arm_dma_zone_size < 0x10000000".
> However, PAGE_OFFSET is always larger than 0x10000000,
> so "0x10000000 - PAGE_OFFSET" is a rather large number?

Yes it is a large number, though I am not too sure what the intention of 
this check was in the first place, whether it denoted the largest known 
DMA size of any machine at the time, or if it has any relationship to 
lowmem (in which case it does not account for its exact value since it 
can be changed indirectly via vmalloc= on the command line).

We ought to be just fine with keeping only:

min_t(phys_addr_t, (PAGE_OFFSET + arm_dma_zone_size - 1), 0xffffffffUL);

Santosh, what did 0x10000000 mean when you added it?
-- 
Florian