* 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).