[PATCH 0/2] ARM: MAX_DMA_ADDRESS fixes

[PATCH 0/2] ARM: MAX_DMA_ADDRESS fixes

[Florian Fainelli]

This patch series clamps the MAX_DMA_ADDRESS to 32-bit in order to fit
within a virtual address, and also fixes the off by one which was
suggested by Geert.

Florian Fainelli (2):
  ARM: Fix off by one in checking DMA zone size
  ARM: Clamp MAX_DMA_ADDRESS to 32-bit

 arch/arm/include/asm/dma.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH 1/2] ARM: Fix off by one in checking DMA zone size

[Florian Fainelli]

The maximum DMA address is PAGE_OFFSET + arm_dma_zone_size - 1, fix this
by doing the appropriate subtraction.

Fixes: 650320181a08 ("ARM: change ARM_DMA_ZONE_SIZE into a variable")
Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/include/asm/dma.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/dma.h b/arch/arm/include/asm/dma.h
index a81dda65c576..f244ee68e814 100644
--- a/arch/arm/include/asm/dma.h
+++ b/arch/arm/include/asm/dma.h
@@ -11,7 +11,7 @@
 #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; })
+		(PAGE_OFFSET + arm_dma_zone_size - 1) : 0xffffffffUL; })
 #endif
 
 #ifdef CONFIG_ISA_DMA_API
-- 
2.25.1

Re: [PATCH 1/2] ARM: Fix off by one in checking DMA zone size

[Linus Walleij]

On Thu, Mar 24, 2022 at 6:54 PM Florian Fainelli <f.fainelli@gmail.com> wrote:

> The maximum DMA address is PAGE_OFFSET + arm_dma_zone_size - 1, fix this
> by doing the appropriate subtraction.
>
> Fixes: 650320181a08 ("ARM: change ARM_DMA_ZONE_SIZE into a variable")
> Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 1/2] ARM: Fix off by one in checking DMA zone size

[Russell King (Oracle)]

On Thu, Mar 24, 2022 at 10:54:16AM -0700, Florian Fainelli wrote:
> The maximum DMA address is PAGE_OFFSET + arm_dma_zone_size - 1, fix this
> by doing the appropriate subtraction.

The question is... is MAX_DMA_ADDRESS inclusive or exclusive. The answer
is rather indeterminant, unfortunately.

drivers/net/wan/cosa.c: if (b + len >= MAX_DMA_ADDRESS)

So, if the buffer address + buffer length is equal to MAX_DMA_ADDRESS,
then the buffer is not DMA-able. To me this looks completely wrong.
It's also completely wrong because 'b' is a "unsigned long" which
means on 32-bit, this can wrap.

drivers/parport/parport_pc.c:
	unsigned long start = (unsigned long) buf;
	unsigned long end = (unsigned long) buf + length - 1;

	if (end < MAX_DMA_ADDRESS) {

So, "end" is the last byte of the buffer. If MAX_DMA_ADDRESS is the last
byte of the virtual address space that supports DMA, then if the two are
equal, we do not use DMA. This seems wrong to me, and points to it being
an _exclusive_ value - it's the last byte plus one.

there's a bunch of memblock allocation functions that use
__pa(MAX_DMA_ADDRESS) as the "min_addr" and if this is a minimum
address, then surely this means that it is also an exclusive value.

So, I came to the conclusion that MAX_DMA_ADDRESS is supposed to be
exclusive. In other words, where PAGE_OFFSET + sizeof(ram) if
sizeof(ram) is the size in bytes of the RAM, limited to the maximum
addressable virtual address that RAM can be mapped to.

I don't see an inclusive value being correct, at least not for the cases
I've looekd at.


-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/2] ARM: Fix off by one in checking DMA zone size

[Arnd Bergmann]

On Fri, Apr 29, 2022 at 5:15 PM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Thu, Mar 24, 2022 at 10:54:16AM -0700, Florian Fainelli wrote:
> > The maximum DMA address is PAGE_OFFSET + arm_dma_zone_size - 1, fix this
> > by doing the appropriate subtraction.
>
> The question is... is MAX_DMA_ADDRESS inclusive or exclusive. The answer
> is rather indeterminant, unfortunately.
>
> drivers/net/wan/cosa.c: if (b + len >= MAX_DMA_ADDRESS)
>
> So, if the buffer address + buffer length is equal to MAX_DMA_ADDRESS,
> then the buffer is not DMA-able. To me this looks completely wrong.
> It's also completely wrong because 'b' is a "unsigned long" which
> means on 32-bit, this can wrap.

This driver is on its way out and will be removed in 5.19.

> there's a bunch of memblock allocation functions that use
> __pa(MAX_DMA_ADDRESS) as the "min_addr" and if this is a minimum
> address, then surely this means that it is also an exclusive value.
>
> So, I came to the conclusion that MAX_DMA_ADDRESS is supposed to be
> exclusive. In other words, where PAGE_OFFSET + sizeof(ram) if
> sizeof(ram) is the size in bytes of the RAM, limited to the maximum
> addressable virtual address that RAM can be mapped to.
>
> I don't see an inclusive value being correct, at least not for the cases
> I've looekd at.

Looking at the other definitions, I see it's usually either ULONG_MAX (which
is inclusive) or an exclusive limit, same as the existing version on arm:

arch/alpha/include/asm/io.h:#define IDENT_ADDR     0xffff800000000000UL
arch/alpha/include/asm/dma.h:#define MAX_DMA_ADDRESS
(alpha_mv.mv_pci_tbi ?  \
arch/alpha/include/asm/dma.h-                            ~0UL :
IDENT_ADDR + 0x01000000)
arch/arc/include/asm/dma.h:#define MAX_DMA_ADDRESS 0xC0000000
arch/hexagon/include/asm/dma.h:#define MAX_DMA_ADDRESS  (PAGE_OFFSET)
arch/ia64/mm/init.c:unsigned long MAX_DMA_ADDRESS = PAGE_OFFSET + 0x100000000UL;
arch/m68k/include/asm/dma.h:#define MAX_DMA_ADDRESS PAGE_OFFSET
arch/mips/include/asm/dma.h:#define MAX_DMA_ADDRESS             PAGE_OFFSET
arch/mips/include/asm/dma.h:#define MAX_DMA_ADDRESS
(PAGE_OFFSET + 0x01000000)
arch/parisc/include/asm/dma.h:#define MAX_DMA_ADDRESS (~0UL)
arch/powerpc/include/asm/dma.h:#define MAX_DMA_ADDRESS          (~0UL)
arch/s390/include/asm/dma.h:#define MAX_DMA_ADDRESS         0x80000000
arch/sparc/include/asm/dma.h:#define MAX_DMA_ADDRESS  (~0UL)
arch/um/include/asm/dma.h:#define MAX_DMA_ADDRESS (uml_physmem)
arch/x86/include/asm/dma.h:#define MAX_DMA_ADDRESS      (PAGE_OFFSET +
0x1000000)
arch/x86/include/asm/dma.h:#define MAX_DMA_PFN   ((16UL * 1024 * 1024)
>> PAGE_SHIFT)
arch/x86/include/asm/dma.h:#define MAX_DMA_ADDRESS ((unsigned
long)__va(MAX_DMA_PFN << PAGE_SHIFT))

The exceptions are:
arch/xtensa/include/asm/kmem_layout.h:#define XCHAL_KIO_SIZE
         0x10000000
arch/xtensa/include/asm/dma.h:#define MAX_DMA_ADDRESS
(PAGE_OFFSET + XCHAL_KIO_SIZE - 1)
arch/microblaze/include/asm/dma.h:#define MAX_DMA_ADDRESS
(CONFIG_KERNEL_START + memory_size - 1)

        Arnd

[PATCH 2/2] ARM: Clamp MAX_DMA_ADDRESS to 32-bit

[Florian Fainelli]

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(-)

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
-- 
2.25.1

Re: [PATCH 2/2] ARM: Clamp MAX_DMA_ADDRESS to 32-bit

[Linus Walleij]

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

Re: [PATCH 2/2] ARM: Clamp MAX_DMA_ADDRESS to 32-bit

[Geert Uytterhoeven]

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

Re: [PATCH 2/2] ARM: Clamp MAX_DMA_ADDRESS to 32-bit

[Florian Fainelli]

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?
-- 
Florian

Re: [PATCH 2/2] ARM: Clamp MAX_DMA_ADDRESS to 32-bit

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 0/2] ARM: MAX_DMA_ADDRESS fixes

[Florian Fainelli]

On 3/24/22 10:54, Florian Fainelli wrote:
> This patch series clamps the MAX_DMA_ADDRESS to 32-bit in order to fit
> within a virtual address, and also fixes the off by one which was
> suggested by Geert.
> 
> Florian Fainelli (2):
>    ARM: Fix off by one in checking DMA zone size
>    ARM: Clamp MAX_DMA_ADDRESS to 32-bit

Russell, do you have any feedback on these two patches?
-- 
Florian