| Message ID | 20220324175417.1014562-3-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: MAX_DMA_ADDRESS fixes | expand |
On Thu, Mar 24, 2022 at 6:54 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > MAX_DMA_ADDRESS is a virtual address, therefore it needs to fit within a > 32-bit unsigned quantity. Platforms defining a DMA zone size in > their machine descriptor can easily overflow this quantity depending on > the DMA zone size and/or the PAGE_OFFSET setting. > > In most cases this is harmless, however in the case of a > CONFIG_DEBUG_VIRTUAL enabled, __virt_addr_valid() will be unable to > return that MAX_DMA_ADDRESS is valid because the value passed to that > function is an unsigned long which has already overflowed. > > Fixes: e377cd8221eb ("ARM: 8640/1: Add support for CONFIG_DEBUG_VIRTUAL") > Fixes: 2fb3ec5c9503 ("ARM: Replace platform definition of ISA_DMA_THRESHOLD/MAX_DMA_ADDRESS") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi Florian, On Thu, Mar 24, 2022 at 6:54 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > MAX_DMA_ADDRESS is a virtual address, therefore it needs to fit within a > 32-bit unsigned quantity. Platforms defining a DMA zone size in > their machine descriptor can easily overflow this quantity depending on > the DMA zone size and/or the PAGE_OFFSET setting. > > In most cases this is harmless, however in the case of a > CONFIG_DEBUG_VIRTUAL enabled, __virt_addr_valid() will be unable to > return that MAX_DMA_ADDRESS is valid because the value passed to that > function is an unsigned long which has already overflowed. > > Fixes: e377cd8221eb ("ARM: 8640/1: Add support for CONFIG_DEBUG_VIRTUAL") > Fixes: 2fb3ec5c9503 ("ARM: Replace platform definition of ISA_DMA_THRESHOLD/MAX_DMA_ADDRESS") > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- a/arch/arm/include/asm/dma.h > +++ b/arch/arm/include/asm/dma.h > @@ -8,10 +8,12 @@ > #ifndef CONFIG_ZONE_DMA > #define MAX_DMA_ADDRESS 0xffffffffUL > #else > +#include <linux/minmax.h> > #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 < (0x10000000 - PAGE_OFFSET)" looks like an overflow-avoiding version of "PAGE_OFFSET + arm_dma_zone_size < 0x10000000". However, PAGE_OFFSET is always larger than 0x10000000, so "0x10000000 - PAGE_OFFSET" is a rather large number? > - (PAGE_OFFSET + arm_dma_zone_size - 1) : 0xffffffffUL; }) > + min_t(phys_addr_t, (PAGE_OFFSET + arm_dma_zone_size - 1), 0xffffffffUL) : \ > + 0xffffffffUL; }) > #endif > > #ifdef CONFIG_ISA_DMA_API 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/24/22 13:31, Geert Uytterhoeven wrote: > Hi Florian, > > On Thu, Mar 24, 2022 at 6:54 PM Florian Fainelli <f.fainelli@gmail.com> wrote: >> MAX_DMA_ADDRESS is a virtual address, therefore it needs to fit within a >> 32-bit unsigned quantity. Platforms defining a DMA zone size in >> their machine descriptor can easily overflow this quantity depending on >> the DMA zone size and/or the PAGE_OFFSET setting. >> >> In most cases this is harmless, however in the case of a >> CONFIG_DEBUG_VIRTUAL enabled, __virt_addr_valid() will be unable to >> return that MAX_DMA_ADDRESS is valid because the value passed to that >> function is an unsigned long which has already overflowed. >> >> Fixes: e377cd8221eb ("ARM: 8640/1: Add support for CONFIG_DEBUG_VIRTUAL") >> Fixes: 2fb3ec5c9503 ("ARM: Replace platform definition of ISA_DMA_THRESHOLD/MAX_DMA_ADDRESS") >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > >> --- a/arch/arm/include/asm/dma.h >> +++ b/arch/arm/include/asm/dma.h >> @@ -8,10 +8,12 @@ >> #ifndef CONFIG_ZONE_DMA >> #define MAX_DMA_ADDRESS 0xffffffffUL >> #else >> +#include <linux/minmax.h> >> #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 < (0x10000000 - PAGE_OFFSET)" looks > like an overflow-avoiding version of > "PAGE_OFFSET + arm_dma_zone_size < 0x10000000". > However, PAGE_OFFSET is always larger than 0x10000000, > so "0x10000000 - PAGE_OFFSET" is a rather large number? Yes it is a large number, though I am not too sure what the intention of this check was in the first place, whether it denoted the largest known DMA size of any machine at the time, or if it has any relationship to lowmem (in which case it does not account for its exact value since it can be changed indirectly via vmalloc= on the command line). We ought to be just fine with keeping only: min_t(phys_addr_t, (PAGE_OFFSET + arm_dma_zone_size - 1), 0xffffffffUL); Santosh, what did 0x10000000 mean when you added it?
On Thu, Mar 24, 2022 at 09:31:56PM +0100, Geert Uytterhoeven wrote: > Hi Florian, > > On Thu, Mar 24, 2022 at 6:54 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > MAX_DMA_ADDRESS is a virtual address, therefore it needs to fit within a > > 32-bit unsigned quantity. Platforms defining a DMA zone size in > > their machine descriptor can easily overflow this quantity depending on > > the DMA zone size and/or the PAGE_OFFSET setting. > > > > In most cases this is harmless, however in the case of a > > CONFIG_DEBUG_VIRTUAL enabled, __virt_addr_valid() will be unable to > > return that MAX_DMA_ADDRESS is valid because the value passed to that > > function is an unsigned long which has already overflowed. > > > > Fixes: e377cd8221eb ("ARM: 8640/1: Add support for CONFIG_DEBUG_VIRTUAL") > > Fixes: 2fb3ec5c9503 ("ARM: Replace platform definition of ISA_DMA_THRESHOLD/MAX_DMA_ADDRESS") > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > > > --- a/arch/arm/include/asm/dma.h > > +++ b/arch/arm/include/asm/dma.h > > @@ -8,10 +8,12 @@ > > #ifndef CONFIG_ZONE_DMA > > #define MAX_DMA_ADDRESS 0xffffffffUL > > #else > > +#include <linux/minmax.h> > > #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 < (0x10000000 - PAGE_OFFSET)" looks > like an overflow-avoiding version of > "PAGE_OFFSET + arm_dma_zone_size < 0x10000000". > However, PAGE_OFFSET is always larger than 0x10000000, > so "0x10000000 - PAGE_OFFSET" is a rather large number? This, to me, looks like it should have been: arm_dma_zone_size && arm_dma_zone_size < (0x100000000 - PAGE_OFFSET) ? \ since that is the only thing that would make sense - it's the virtual space remaining between PAGE_OFFSET and the top of addressable space. However, 0x100000000 isn't correct - since the vectors live at 0xffff0000, and we have the vmalloc space etc.
diff --git a/arch/arm/include/asm/dma.h b/arch/arm/include/asm/dma.h index f244ee68e814..ea47420babd4 100644 --- a/arch/arm/include/asm/dma.h +++ b/arch/arm/include/asm/dma.h @@ -8,10 +8,12 @@ #ifndef CONFIG_ZONE_DMA #define MAX_DMA_ADDRESS 0xffffffffUL #else +#include <linux/minmax.h> #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 - 1) : 0xffffffffUL; }) + min_t(phys_addr_t, (PAGE_OFFSET + arm_dma_zone_size - 1), 0xffffffffUL) : \ + 0xffffffffUL; }) #endif #ifdef CONFIG_ISA_DMA_API
MAX_DMA_ADDRESS is a virtual address, therefore it needs to fit within a 32-bit unsigned quantity. Platforms defining a DMA zone size in their machine descriptor can easily overflow this quantity depending on the DMA zone size and/or the PAGE_OFFSET setting. In most cases this is harmless, however in the case of a CONFIG_DEBUG_VIRTUAL enabled, __virt_addr_valid() will be unable to return that MAX_DMA_ADDRESS is valid because the value passed to that function is an unsigned long which has already overflowed. Fixes: e377cd8221eb ("ARM: 8640/1: Add support for CONFIG_DEBUG_VIRTUAL") Fixes: 2fb3ec5c9503 ("ARM: Replace platform definition of ISA_DMA_THRESHOLD/MAX_DMA_ADDRESS") Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- arch/arm/include/asm/dma.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)