* MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G
@ 2022-03-21 3:46 Florian Fainelli
2022-03-23 15:02 ` Linus Walleij
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2022-03-21 3:46 UTC (permalink / raw)
To: linux-arm-kernel, Russell King, Nicolas Pitre, linus.walleij
Hi all,
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; })
and with arch/arm/mach-bcm/bcm2711.c defining a dma_zone_size of SZ_1G
and PAGE_OFFSET = 0xC000_0000 we end up with MAX_DMA_ADDRES of
0x1_0000_0000 which overflows the virtual address size that is
represented with an unsigned long.
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.
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?
The splats can be silenced with this too:
diff --git a/arch/arm/mm/physaddr.c b/arch/arm/mm/physaddr.c
index cf75819e4c13..abf071c7c6e9 100644
--- a/arch/arm/mm/physaddr.c
+++ b/arch/arm/mm/physaddr.c
@@ -29,7 +29,7 @@ static inline bool __virt_addr_valid(unsigned long x)
* actual physical address. Enough code relies on
__pa(MAX_DMA_ADDRESS)
* that we just need to work around it and always return true.
*/
- if (x == MAX_DMA_ADDRESS)
+ if (x == (unsigned long)MAX_DMA_ADDRESS)
return true;
return false;
but this does not permit differentiating a 0 virtual address from
MAX_DMA_ADDRESS having overflowed.
Thanks!
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G
2022-03-21 3:46 MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G Florian Fainelli
@ 2022-03-23 15:02 ` Linus Walleij
2022-03-23 16:53 ` Geert Uytterhoeven
0 siblings, 1 reply; 6+ messages in thread
From: Linus Walleij @ 2022-03-23 15:02 UTC (permalink / raw)
To: Florian Fainelli, Arnd Bergmann, Ard Biesheuvel,
Geert Uytterhoeven
Cc: linux-arm-kernel, Russell King, Nicolas Pitre
On Mon, Mar 21, 2022 at 4:46 AM Florian Fainelli <f.fainelli@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?
Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G
2022-03-23 15:02 ` Linus Walleij
@ 2022-03-23 16:53 ` Geert Uytterhoeven
2022-03-23 19:52 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2022-03-23 16:53 UTC (permalink / raw)
To: Linus Walleij
Cc: Florian Fainelli, Arnd Bergmann, Ard Biesheuvel, linux-arm-kernel,
Russell King, Nicolas Pitre
Hi Linus,
On Wed, Mar 23, 2022 at 4:02 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Mar 21, 2022 at 4:46 AM Florian Fainelli <f.fainelli@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)"?
> >
> > and with arch/arm/mach-bcm/bcm2711.c defining a dma_zone_size of SZ_1G
> > and PAGE_OFFSET = 0xC000_0000 we end up with MAX_DMA_ADDRES of
> > 0x1_0000_0000 which overflows the virtual address size that is
> > represented with an unsigned long.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G
2022-03-23 16:53 ` Geert Uytterhoeven
@ 2022-03-23 19:52 ` Florian Fainelli
2022-03-23 20:28 ` Geert Uytterhoeven
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2022-03-23 19:52 UTC (permalink / raw)
To: Geert Uytterhoeven, Linus Walleij, Rob Herring
Cc: Arnd Bergmann, Ard Biesheuvel, linux-arm-kernel, Russell King,
Nicolas Pitre
On 3/23/22 09:53, Geert Uytterhoeven wrote:
> Hi Linus,
>
> On Wed, Mar 23, 2022 at 4:02 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, Mar 21, 2022 at 4:46 AM Florian Fainelli <f.fainelli@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,
I don't see anything that would prevent them from using a VMSPLIT_3G
configuration.
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G
2022-03-23 19:52 ` Florian Fainelli
@ 2022-03-23 20:28 ` Geert Uytterhoeven
2022-03-23 20:36 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2022-03-23 20:28 UTC (permalink / raw)
To: Florian Fainelli
Cc: Linus Walleij, Rob Herring, Arnd Bergmann, Ard Biesheuvel,
linux-arm-kernel, Russell King, Nicolas Pitre
Hi Florian,
On Wed, Mar 23, 2022 at 8:52 PM Florian Fainelli <f.fainelli@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@linaro.org> wrote:
> >> On Mon, Mar 21, 2022 at 4:46 AM Florian Fainelli <f.fainelli@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?
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).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G
2022-03-23 20:28 ` Geert Uytterhoeven
@ 2022-03-23 20:36 ` Florian Fainelli
0 siblings, 0 replies; 6+ messages in thread
From: Florian Fainelli @ 2022-03-23 20:36 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Linus Walleij, Rob Herring, Arnd Bergmann, Ard Biesheuvel,
linux-arm-kernel, Russell King, Nicolas Pitre
On 3/23/22 13:28, Geert Uytterhoeven wrote:
> Hi Florian,
>
> On Wed, Mar 23, 2022 at 8:52 PM Florian Fainelli <f.fainelli@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@linaro.org> wrote:
>>>> On Mon, Mar 21, 2022 at 4:46 AM Florian Fainelli <f.fainelli@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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-23 20:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-03-21 3:46 MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G Florian Fainelli
2022-03-23 15:02 ` Linus Walleij
2022-03-23 16:53 ` Geert Uytterhoeven
2022-03-23 19:52 ` Florian Fainelli
2022-03-23 20:28 ` Geert Uytterhoeven
2022-03-23 20:36 ` Florian Fainelli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).