| Message ID | 20220706164606.68528-1-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] ARM: Fix MAX_DMA_ADDRESS overflow | expand |
Hi Florian, On Wed, Jul 6, 2022 at 6:46 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > Commit 26f09e9b3a06 ("mm/memblock: add memblock memory allocation apis") > added a check to determine whether arm_dma_zone_size is exceeding the > amount of kernel virtual address space available between the upper 4GB > virtual address limit and PAGE_OFFSET in order to provide a suitable > definition of MAX_DMA_ADDRESS that should fit within the 32-bit virtual > address space. The quantity used for comparison was off by a missing > trailing 0, leading to MAX_DMA_ADDRESS to be overflowing a 32-bit > quantity. > > This was caught with the bcm2711 platforms which defines a dma_zone_size > of 1GB, and using a PAGE_OFFSET of 0xc000_0000 (CONFIG_VMSPLIT_3G) with > CONFIG_DEBUG_VIRTUAL enabled would lead to MAX_DMA_ADDRESS being > 0x1_0000_0000 which overflows the unsigned long type used throughout > __pa() and __virt_addr_valid(). Because the virtual address passed to > __virt_addr_valid() would now be 0, the function would loudly warn, thus > making the platform unable to boot properly. > > Fixes: 26f09e9b3a06 ("mm/memblock: add memblock memory allocation apis") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > Changes in v2: > > - simplify the patch and drop the first patch that attempted to fix an > off by one in the calculation. Thanks for the update! > --- a/arch/arm/include/asm/dma.h > +++ b/arch/arm/include/asm/dma.h > @@ -10,7 +10,7 @@ > #else > #define MAX_DMA_ADDRESS ({ \ > extern phys_addr_t arm_dma_zone_size; \ > - arm_dma_zone_size && arm_dma_zone_size < (0x10000000 - PAGE_OFFSET) ? \ ^^^^^^^^^^ 0x10000000ULL, as the constant doesn't fit in 32-bit. However, both gcc (9.4.0) and sparse don't seem to complain about the missing suffix (anymore?). > + arm_dma_zone_size && arm_dma_zone_size < (0x100000000 - PAGE_OFFSET) ? \ > (PAGE_OFFSET + arm_dma_zone_size) : 0xffffffffUL; }) > #endif 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 Wed, Jul 6, 2022 at 9:44 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Jul 6, 2022 at 6:46 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > Commit 26f09e9b3a06 ("mm/memblock: add memblock memory allocation apis") > > added a check to determine whether arm_dma_zone_size is exceeding the > > amount of kernel virtual address space available between the upper 4GB > > virtual address limit and PAGE_OFFSET in order to provide a suitable > > definition of MAX_DMA_ADDRESS that should fit within the 32-bit virtual > > address space. The quantity used for comparison was off by a missing > > trailing 0, leading to MAX_DMA_ADDRESS to be overflowing a 32-bit > > quantity. > > > > This was caught with the bcm2711 platforms which defines a dma_zone_size > > of 1GB, and using a PAGE_OFFSET of 0xc000_0000 (CONFIG_VMSPLIT_3G) with > > CONFIG_DEBUG_VIRTUAL enabled would lead to MAX_DMA_ADDRESS being > > 0x1_0000_0000 which overflows the unsigned long type used throughout > > __pa() and __virt_addr_valid(). Because the virtual address passed to > > __virt_addr_valid() would now be 0, the function would loudly warn, thus > > making the platform unable to boot properly. > > > > Fixes: 26f09e9b3a06 ("mm/memblock: add memblock memory allocation apis") > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > --- > > Changes in v2: > > > > - simplify the patch and drop the first patch that attempted to fix an > > off by one in the calculation. > > Thanks for the update! > > > --- a/arch/arm/include/asm/dma.h > > +++ b/arch/arm/include/asm/dma.h > > @@ -10,7 +10,7 @@ > > #else > > #define MAX_DMA_ADDRESS ({ \ > > extern phys_addr_t arm_dma_zone_size; \ > > - arm_dma_zone_size && arm_dma_zone_size < (0x10000000 - PAGE_OFFSET) ? \ > ^^^^^^^^^^ > 0x10000000ULL, as the constant doesn't fit in 32-bit. > However, both gcc (9.4.0) and sparse don't seem to complain about > the missing suffix (anymore?). > > > + arm_dma_zone_size && arm_dma_zone_size < (0x100000000 - PAGE_OFFSET) ? \ Obviously my comment applies to the _new_ line, not to the removed line... > > (PAGE_OFFSET + arm_dma_zone_size) : 0xffffffffUL; }) > > #endif 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 7/6/22 12:44, Geert Uytterhoeven wrote: > Hi Florian, > > On Wed, Jul 6, 2022 at 6:46 PM Florian Fainelli <f.fainelli@gmail.com> wrote: >> Commit 26f09e9b3a06 ("mm/memblock: add memblock memory allocation apis") >> added a check to determine whether arm_dma_zone_size is exceeding the >> amount of kernel virtual address space available between the upper 4GB >> virtual address limit and PAGE_OFFSET in order to provide a suitable >> definition of MAX_DMA_ADDRESS that should fit within the 32-bit virtual >> address space. The quantity used for comparison was off by a missing >> trailing 0, leading to MAX_DMA_ADDRESS to be overflowing a 32-bit >> quantity. >> >> This was caught with the bcm2711 platforms which defines a dma_zone_size >> of 1GB, and using a PAGE_OFFSET of 0xc000_0000 (CONFIG_VMSPLIT_3G) with >> CONFIG_DEBUG_VIRTUAL enabled would lead to MAX_DMA_ADDRESS being >> 0x1_0000_0000 which overflows the unsigned long type used throughout >> __pa() and __virt_addr_valid(). Because the virtual address passed to >> __virt_addr_valid() would now be 0, the function would loudly warn, thus >> making the platform unable to boot properly. >> >> Fixes: 26f09e9b3a06 ("mm/memblock: add memblock memory allocation apis") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> Changes in v2: >> >> - simplify the patch and drop the first patch that attempted to fix an >> off by one in the calculation. > > Thanks for the update! > >> --- a/arch/arm/include/asm/dma.h >> +++ b/arch/arm/include/asm/dma.h >> @@ -10,7 +10,7 @@ >> #else >> #define MAX_DMA_ADDRESS ({ \ >> extern phys_addr_t arm_dma_zone_size; \ >> - arm_dma_zone_size && arm_dma_zone_size < (0x10000000 - PAGE_OFFSET) ? \ > ^^^^^^^^^^ > 0x10000000ULL, as the constant doesn't fit in 32-bit. > However, both gcc (9.4.0) and sparse don't seem to complain about > the missing suffix (anymore?). Thanks, I will the ULL suffix in v3.
Hi Florian, On Wed, Jul 6, 2022 at 10:27 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > On 7/6/22 12:44, Geert Uytterhoeven wrote: > > On Wed, Jul 6, 2022 at 6:46 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > >> Commit 26f09e9b3a06 ("mm/memblock: add memblock memory allocation apis") > >> added a check to determine whether arm_dma_zone_size is exceeding the > >> amount of kernel virtual address space available between the upper 4GB > >> virtual address limit and PAGE_OFFSET in order to provide a suitable > >> definition of MAX_DMA_ADDRESS that should fit within the 32-bit virtual > >> address space. The quantity used for comparison was off by a missing > >> trailing 0, leading to MAX_DMA_ADDRESS to be overflowing a 32-bit > >> quantity. > >> > >> This was caught with the bcm2711 platforms which defines a dma_zone_size > >> of 1GB, and using a PAGE_OFFSET of 0xc000_0000 (CONFIG_VMSPLIT_3G) with > >> CONFIG_DEBUG_VIRTUAL enabled would lead to MAX_DMA_ADDRESS being > >> 0x1_0000_0000 which overflows the unsigned long type used throughout > >> __pa() and __virt_addr_valid(). Because the virtual address passed to > >> __virt_addr_valid() would now be 0, the function would loudly warn, thus > >> making the platform unable to boot properly. > >> > >> Fixes: 26f09e9b3a06 ("mm/memblock: add memblock memory allocation apis") > >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- > >> Changes in v2: > >> > >> - simplify the patch and drop the first patch that attempted to fix an > >> off by one in the calculation. > > > > Thanks for the update! > > > >> --- a/arch/arm/include/asm/dma.h > >> +++ b/arch/arm/include/asm/dma.h > >> @@ -10,7 +10,7 @@ > >> #else > >> #define MAX_DMA_ADDRESS ({ \ > >> extern phys_addr_t arm_dma_zone_size; \ > >> - arm_dma_zone_size && arm_dma_zone_size < (0x10000000 - PAGE_OFFSET) ? \ > > ^^^^^^^^^^ > > 0x10000000ULL, as the constant doesn't fit in 32-bit. > > However, both gcc (9.4.0) and sparse don't seem to complain about > > the missing suffix (anymore?). > > Thanks, I will the ULL suffix in v3. I just remembered the suffix is not needed (but doesn't hurt), because hexadecimal constants automatically have the right unsigned type. 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
diff --git a/arch/arm/include/asm/dma.h b/arch/arm/include/asm/dma.h index a81dda65c576..1ffa75beb709 100644 --- a/arch/arm/include/asm/dma.h +++ b/arch/arm/include/asm/dma.h @@ -10,7 +10,7 @@ #else #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 && arm_dma_zone_size < (0x100000000 - PAGE_OFFSET) ? \ (PAGE_OFFSET + arm_dma_zone_size) : 0xffffffffUL; }) #endif
Commit 26f09e9b3a06 ("mm/memblock: add memblock memory allocation apis") added a check to determine whether arm_dma_zone_size is exceeding the amount of kernel virtual address space available between the upper 4GB virtual address limit and PAGE_OFFSET in order to provide a suitable definition of MAX_DMA_ADDRESS that should fit within the 32-bit virtual address space. The quantity used for comparison was off by a missing trailing 0, leading to MAX_DMA_ADDRESS to be overflowing a 32-bit quantity. This was caught with the bcm2711 platforms which defines a dma_zone_size of 1GB, and using a PAGE_OFFSET of 0xc000_0000 (CONFIG_VMSPLIT_3G) with CONFIG_DEBUG_VIRTUAL enabled would lead to MAX_DMA_ADDRESS being 0x1_0000_0000 which overflows the unsigned long type used throughout __pa() and __virt_addr_valid(). Because the virtual address passed to __virt_addr_valid() would now be 0, the function would loudly warn, thus making the platform unable to boot properly. Fixes: 26f09e9b3a06 ("mm/memblock: add memblock memory allocation apis") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- Changes in v2: - simplify the patch and drop the first patch that attempted to fix an off by one in the calculation. arch/arm/include/asm/dma.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)