MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G

[Florian Fainelli]

On 3/23/22 13:28, Geert Uytterhoeven wrote:
> Hi Florian,
> 
> On Wed, Mar 23, 2022 at 8:52 PM Florian Fainelli <f.fainelli at gmail.com> wrote:
>> On 3/23/22 09:53, Geert Uytterhoeven wrote:
>>> On Wed, Mar 23, 2022 at 4:02 PM Linus Walleij <linus.walleij at linaro.org> wrote:
>>>> On Mon, Mar 21, 2022 at 4:46 AM Florian Fainelli <f.fainelli at gmail.com> wrote:
>>>>> All of the virt_to_phys() and related functions either take a pointer
>>>>> size argument (const volatile void *) or an unsigned long argument and
>>>>> these are virtual addresses so unable to go over 32-bit anyway.
>>>>
>>>> Oh I ran into that too, in some different context that I since forgot.
>>>> A macro that works the same on pointers and unsigned long but with
>>>> slightly different semantics :P
>>>>
>>>> I don't know what is the proper thing to do here. Let's involve Arnd
>>>> and Ard and Geert!
>>>>
>>>>> Since MAX_DMA_ADDRESS is intended to be "This is the maximum virtual
>>>>> address which can be DMA'd from.", should we make sure that we clamp it
>>>>> below 32-bit in case it overflows?
>>>>
>>>> Hmmmm.... I don't know what that would mean in practice?
>>>
>>>>> While debugging numerous KASAN splats with CONFIG_DEBUG_VIRTUAL on ARM
>>>>> 32-bit with a Raspberry Pi 4B 4GB, it finally clicked that the problem
>>>>> is with the use of __virt_to_phys(MAX_DMA_ADDRESS). Since that platform
>>>>> has CONFIG_ZONE_DMA enabled, we end-up with:
>>>>>
>>>>>      #define MAX_DMA_ADDRESS ({ \
>>>>>              extern phys_addr_t arm_dma_zone_size; \
>>>>>              arm_dma_zone_size && arm_dma_zone_size < (0x10000000 -
>>>>> PAGE_OFFSET) ? \
>>>>>                      (PAGE_OFFSET + arm_dma_zone_size) : 0xffffffffUL; })
>>>
>>> I guess that should be "PAGE_OFFSET + (arm_dma_zone_size - 1)"?
>>
>> Yes, we are definitively off by one here, so this is a good catch and
>> this will work for bcm2711 and any platform whereby PAGE_OFFSET +
>> arm_dma_zone_size < 0xffff_ffff.
>>
>> There are a few that will still overflow that quantity:
>>
>> arch/arm/mach-highbank/highbank.c:      .dma_zone_size  = (4ULL * SZ_1G),
>> arch/arm/mach-keystone/keystone.c:      .dma_zone_size  = SZ_2G,
>> arch/arm/mach-omap2/board-generic.c:    .dma_zone_size  = SZ_2G,
>> arch/arm/mach-omap2/board-generic.c:    .dma_zone_size  = SZ_2G,
>> arch/arm/mach-omap2/board-generic.c:    .dma_zone_size  = SZ_2G,
> 
> Better show some context:
> 
>      $ git grep -W "\<dma_zone_size.*SZ_.G" -- arch/arm
>      arch/arm/mach-bcm/bcm2711.c=DT_MACHINE_START(BCM2711, "BCM2711")
>      arch/arm/mach-bcm/bcm2711.c-#ifdef CONFIG_ZONE_DMA
>      arch/arm/mach-bcm/bcm2711.c:    .dma_zone_size  = SZ_1G,
> 
> So this is the problematic one?

The one that prompted this email yes, definitively problematic :D

> 
>      arch/arm/mach-bcm/bcm2711.c-#endif
>      arch/arm/mach-bcm/bcm2711.c-    .dt_compat = bcm2711_compat,
>      arch/arm/mach-bcm/bcm2711.c-    .smp = smp_ops(bcm2836_smp_ops),
>      --
>      arch/arm/mach-highbank/highbank.c=DT_MACHINE_START(HIGHBANK, "Highbank")
>      arch/arm/mach-highbank/highbank.c-#if defined(CONFIG_ZONE_DMA) &&
> defined(CONFIG_ARM_LPAE)
>      arch/arm/mach-highbank/highbank.c:      .dma_zone_size  = (4ULL * SZ_1G),
> 
> This is fine, as LPAE implies physaddr_t is 64-bit.
> 
>      arch/arm/mach-highbank/highbank.c-#endif
> 
> The omap ones are fine for the same reason (LPAE).

Well it is fine except that MAX_DMA_ADDRESS is supposed to be a 
*virtual* address so it will be 32-bit only even with LPAE, if it was a 
physical one, we would be fine. By adding PAGE_OFFSET to get a virtual 
address, we are definitively going above 32-bits.
-- 
Florian