| Message ID | 41c9bb67-3f80-510f-9904-a568a865b0ea@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MAX_DMA_ADDRESS overflow with non-zero arm_dma_zone_size and VMSPLIT_3G | expand |
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
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
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.
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
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.
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;
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: return false; but this does not permit differentiating a 0 virtual address from MAX_DMA_ADDRESS having overflowed. Thanks!