diff mbox

[1/2] arm64: Fix overlapping VA allocations

Message ID 1421091408-10660-2-git-send-email-mark.rutland@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Mark Rutland Jan. 12, 2015, 7:36 p.m. UTC 
32MiB of space immediately below MODULES_VADDR is allocated as PCI I/O
space, yet the final 8KiB of this space is also allocated for the
fixmap, allowing for potential clashes between the two.

Additionally, the size of the PCI I/O space is assumed to be 16MiB by
mem_init and the page table dumping code, resulting the I/O space being
erroneously reported as 16MiB long.

This patch changes the definition of the PCI I/O space allocation to
live in asm/memory.h, along with the other VA space allocations. As the
fixmap allocation depends on the number of fixmap entries, this is moved
below the PCI I/O space allocation. Both the fixmap and PCI I/O space
are guarded with a following 2MB of padding. Sites assuming the I/O
space was 16MiB are moved over use new PCI_IO_{START,END} definitions.

As a useful side effect, the use of the new PCI_IO_{START,END}
definitions prevents a build issue in the dumping code due to a (now
redundant) missing include of io.h for PCI_IOBASE, reported by Mark
Brown.

Signed-off-by: Mark Rutland <mark.rutland@arm.com>
Reported-by: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Laura Abbott <lauraa@codeaurora.org>
Cc: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Steve Capper <steve.capper@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/io.h     | 5 +++--
 arch/arm64/include/asm/memory.h | 4 +++-
 arch/arm64/mm/dump.c            | 4 ++--
 arch/arm64/mm/init.c            | 3 ++-
 4 files changed, 10 insertions(+), 6 deletions(-)

Comments

Catalin Marinas Jan. 13, 2015, 3:34 p.m. UTC | #1 
On Mon, Jan 12, 2015 at 07:36:47PM +0000, Mark Rutland wrote:
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -26,6 +26,7 @@
>  
>  #include <asm/byteorder.h>
>  #include <asm/barrier.h>
> +#include <asm/memory.h>
>  #include <asm/pgtable.h>
>  #include <asm/early_ioremap.h>
>  #include <asm/alternative.h>
> @@ -145,8 +146,8 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>   *  I/O port access primitives.
>   */
>  #define arch_has_dev_port()	(1)
> -#define IO_SPACE_LIMIT		(SZ_32M - 1)
> -#define PCI_IOBASE		((void __iomem *)(MODULES_VADDR - SZ_32M))
> +#define IO_SPACE_LIMIT		(PCI_IO_END - PCI_IO_START - 1)
> +#define PCI_IOBASE		((void __iomem *)PCI_IO_START)

I've seen at least couple of places where IO_SPACE_LIMIT is used as a
mark. So I would prefer it to be something like (power-of-two - 1)
rather than some random (size - 1).

> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -45,7 +45,9 @@
>  #define PAGE_OFFSET		(UL(0xffffffffffffffff) << (VA_BITS - 1))
>  #define MODULES_END		(PAGE_OFFSET)
>  #define MODULES_VADDR		(MODULES_END - SZ_64M)
> -#define FIXADDR_TOP		(MODULES_VADDR - SZ_2M - PAGE_SIZE)
> +#define PCI_IO_END		(MODULES_VADDR - SZ_2M)
> +#define PCI_IO_START		(PCI_IO_END - SZ_32M)
> +#define FIXADDR_TOP		(PCI_IO_START - SZ_2M)
>  #define TASK_SIZE_64		(UL(1) << VA_BITS)

So you could make PCI_IO_START MODULES_VADDR - SZ_16M and FIXADDR_TOP
just below it (or above as it was before, I don't really care).

>  #ifdef CONFIG_COMPAT
> diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
> index cf33f33..203a6cf 100644
> --- a/arch/arm64/mm/dump.c
> +++ b/arch/arm64/mm/dump.c
> @@ -52,8 +52,8 @@ static struct addr_marker address_markers[] = {
>  	{ 0,			"vmemmap start" },
>  	{ 0,			"vmemmap end" },
>  #endif
> -	{ (unsigned long) PCI_IOBASE,		"PCI I/O start" },
> -	{ (unsigned long) PCI_IOBASE + SZ_16M,	"PCI I/O end" },
> +	{ PCI_IO_START,		"PCI I/O start" },
> +	{ PCI_IO_END,		"PCI I/O end" },
>  	{ FIXADDR_START,	"Fixmap start" },
>  	{ FIXADDR_TOP,		"Fixmap end" },
>  	{ MODULES_VADDR,	"Modules start" },
> 
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index bac492c..9f2406d 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -35,6 +35,7 @@
>  #include <linux/efi.h>
>  
>  #include <asm/fixmap.h>
> +#include <asm/memory.h>
>  #include <asm/sections.h>
>  #include <asm/setup.h>
>  #include <asm/sizes.h>
> @@ -291,7 +292,7 @@ void __init mem_init(void)
>  		  MLM((unsigned long)virt_to_page(PAGE_OFFSET),
>  		      (unsigned long)virt_to_page(high_memory)),
>  #endif
> -		  MLM((unsigned long)PCI_IOBASE, (unsigned long)PCI_IOBASE + SZ_16M),
> +		  MLM(PCI_IO_START, PCI_IO_END),
>  		  MLK(FIXADDR_START, FIXADDR_TOP),
>  		  MLM(MODULES_VADDR, MODULES_END),
>  		  MLM(PAGE_OFFSET, (unsigned long)high_memory),

If you change the order of FIX_ADDR_TOP with the PCI_IO_START, so you
should change the printed order as well.
Mark Rutland Jan. 14, 2015, 10:42 a.m. UTC | #2 
On Tue, Jan 13, 2015 at 03:34:49PM +0000, Catalin Marinas wrote:
> On Mon, Jan 12, 2015 at 07:36:47PM +0000, Mark Rutland wrote:
> > --- a/arch/arm64/include/asm/io.h
> > +++ b/arch/arm64/include/asm/io.h
> > @@ -26,6 +26,7 @@
> >  
> >  #include <asm/byteorder.h>
> >  #include <asm/barrier.h>
> > +#include <asm/memory.h>
> >  #include <asm/pgtable.h>
> >  #include <asm/early_ioremap.h>
> >  #include <asm/alternative.h>
> > @@ -145,8 +146,8 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
> >   *  I/O port access primitives.
> >   */
> >  #define arch_has_dev_port()	(1)
> > -#define IO_SPACE_LIMIT		(SZ_32M - 1)
> > -#define PCI_IOBASE		((void __iomem *)(MODULES_VADDR - SZ_32M))
> > +#define IO_SPACE_LIMIT		(PCI_IO_END - PCI_IO_START - 1)
> > +#define PCI_IOBASE		((void __iomem *)PCI_IO_START)
> 
> I've seen at least couple of places where IO_SPACE_LIMIT is used as a
> mark. So I would prefer it to be something like (power-of-two - 1)
> rather than some random (size - 1).

I couldn't spot instances in core code (maybe I missed them), just
arch/arm and arch/hexagon:

[mark@leverpostej:~/src/linux]% git grep IO_SPACE_LIMIT -- arch | grep '&'
arch/arm/include/asm/io.h:#define __io(a)               __typesafe_io(PCI_IO_VIRT_BASE + ((a) & IO_SPACE_LIMIT))
arch/arm/include/asm/io.h:#define __io(a)               __typesafe_io((a) & IO_SPACE_LIMIT)
arch/hexagon/include/asm/io.h:  return readb(_IO_BASE + (port & IO_SPACE_LIMIT));
arch/hexagon/include/asm/io.h:  return readw(_IO_BASE + (port & IO_SPACE_LIMIT));
arch/hexagon/include/asm/io.h:  return readl(_IO_BASE + (port & IO_SPACE_LIMIT));
arch/hexagon/include/asm/io.h:  writeb(data, _IO_BASE + (port & IO_SPACE_LIMIT));
arch/hexagon/include/asm/io.h:  writew(data, _IO_BASE + (port & IO_SPACE_LIMIT));
arch/hexagon/include/asm/io.h:  writel(data, _IO_BASE + (port & IO_SPACE_LIMIT));

Are PCI I/O addresses expected to wrap in this manner? To me it seems
that masking addresses in this way just hides a bug if addresses are
provided that don't fit inside the I/O space -- we'll generate an
address inside the I/O space, but it could still be wrong.

If we do require IO_SPACE_LIMIT to function as a mask, then I think that
we should add an explicit check to asm/io.h for that:

/*
 * IO_SPACE_LIMIT needs to function as a mask.
 */
BUILD_BUG_ON_NOT_POWER_OF_2(IO_SPACE_LIMIT + 1);

That should highlight potential problems if someone updates the I/O
space definitions in future. If core expects that IO_SPACE_LIMIT is
usable as a mask then that should probably live in asm-generic/io.h.

> > --- a/arch/arm64/include/asm/memory.h
> > +++ b/arch/arm64/include/asm/memory.h
> > @@ -45,7 +45,9 @@
> >  #define PAGE_OFFSET		(UL(0xffffffffffffffff) << (VA_BITS - 1))
> >  #define MODULES_END		(PAGE_OFFSET)
> >  #define MODULES_VADDR		(MODULES_END - SZ_64M)
> > -#define FIXADDR_TOP		(MODULES_VADDR - SZ_2M - PAGE_SIZE)
> > +#define PCI_IO_END		(MODULES_VADDR - SZ_2M)
> > +#define PCI_IO_START		(PCI_IO_END - SZ_32M)
> > +#define FIXADDR_TOP		(PCI_IO_START - SZ_2M)
> >  #define TASK_SIZE_64		(UL(1) << VA_BITS)
> 
> So you could make PCI_IO_START MODULES_VADDR - SZ_16M and FIXADDR_TOP
> just below it (or above as it was before, I don't really care).

I'd very much prefer that we have the the PCI_IO_END defintion and
define PCI_IO_START in terms of that. That makes it easier to ensure
that the boot-time VA space layout dump and the page table dumper agree
with the actual VA space layout.

Are you asking for the I/O space to be shrunk to 16MB?

I can drop the 2MB padding if that's what you're suggesting?

> >  #ifdef CONFIG_COMPAT
> > diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
> > index cf33f33..203a6cf 100644
> > --- a/arch/arm64/mm/dump.c
> > +++ b/arch/arm64/mm/dump.c
> > @@ -52,8 +52,8 @@ static struct addr_marker address_markers[] = {
> >  	{ 0,			"vmemmap start" },
> >  	{ 0,			"vmemmap end" },
> >  #endif
> > -	{ (unsigned long) PCI_IOBASE,		"PCI I/O start" },
> > -	{ (unsigned long) PCI_IOBASE + SZ_16M,	"PCI I/O end" },
> > +	{ PCI_IO_START,		"PCI I/O start" },
> > +	{ PCI_IO_END,		"PCI I/O end" },
> >  	{ FIXADDR_START,	"Fixmap start" },
> >  	{ FIXADDR_TOP,		"Fixmap end" },
> >  	{ MODULES_VADDR,	"Modules start" },
> > 
> > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> > index bac492c..9f2406d 100644
> > --- a/arch/arm64/mm/init.c
> > +++ b/arch/arm64/mm/init.c
> > @@ -35,6 +35,7 @@
> >  #include <linux/efi.h>
> >  
> >  #include <asm/fixmap.h>
> > +#include <asm/memory.h>
> >  #include <asm/sections.h>
> >  #include <asm/setup.h>
> >  #include <asm/sizes.h>
> > @@ -291,7 +292,7 @@ void __init mem_init(void)
> >  		  MLM((unsigned long)virt_to_page(PAGE_OFFSET),
> >  		      (unsigned long)virt_to_page(high_memory)),
> >  #endif
> > -		  MLM((unsigned long)PCI_IOBASE, (unsigned long)PCI_IOBASE + SZ_16M),
> > +		  MLM(PCI_IO_START, PCI_IO_END),
> >  		  MLK(FIXADDR_START, FIXADDR_TOP),
> >  		  MLM(MODULES_VADDR, MODULES_END),
> >  		  MLM(PAGE_OFFSET, (unsigned long)high_memory),
> 
> If you change the order of FIX_ADDR_TOP with the PCI_IO_START, so you
> should change the printed order as well.

Done.

Thanks,
Mark.
Will Deacon Jan. 14, 2015, 10:53 a.m. UTC | #3 
On Wed, Jan 14, 2015 at 10:42:56AM +0000, Mark Rutland wrote:
> On Tue, Jan 13, 2015 at 03:34:49PM +0000, Catalin Marinas wrote:
> > On Mon, Jan 12, 2015 at 07:36:47PM +0000, Mark Rutland wrote:
> > > --- a/arch/arm64/include/asm/io.h
> > > +++ b/arch/arm64/include/asm/io.h
> > > @@ -26,6 +26,7 @@
> > >  
> > >  #include <asm/byteorder.h>
> > >  #include <asm/barrier.h>
> > > +#include <asm/memory.h>
> > >  #include <asm/pgtable.h>
> > >  #include <asm/early_ioremap.h>
> > >  #include <asm/alternative.h>
> > > @@ -145,8 +146,8 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
> > >   *  I/O port access primitives.
> > >   */
> > >  #define arch_has_dev_port()	(1)
> > > -#define IO_SPACE_LIMIT		(SZ_32M - 1)
> > > -#define PCI_IOBASE		((void __iomem *)(MODULES_VADDR - SZ_32M))
> > > +#define IO_SPACE_LIMIT		(PCI_IO_END - PCI_IO_START - 1)
> > > +#define PCI_IOBASE		((void __iomem *)PCI_IO_START)
> > 
> > I've seen at least couple of places where IO_SPACE_LIMIT is used as a
> > mark. So I would prefer it to be something like (power-of-two - 1)
> > rather than some random (size - 1).
> 
> I couldn't spot instances in core code (maybe I missed them), just
> arch/arm and arch/hexagon:

drivers/pci/probe.c: mask64 = PCI_BASE_ADDRESS_IO_MASK & (u32)IO_SPACE_LIMIT;
include/asm-generic/io.h: return PCI_IOBASE + (port & IO_SPACE_LIMIT);

Will
diff mbox

Patch

diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 949c406..c180c9d 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -26,6 +26,7 @@ 
 
 #include <asm/byteorder.h>
 #include <asm/barrier.h>
+#include <asm/memory.h>
 #include <asm/pgtable.h>
 #include <asm/early_ioremap.h>
 #include <asm/alternative.h>
@@ -145,8 +146,8 @@  static inline u64 __raw_readq(const volatile void __iomem *addr)
  *  I/O port access primitives.
  */
 #define arch_has_dev_port()	(1)
-#define IO_SPACE_LIMIT		(SZ_32M - 1)
-#define PCI_IOBASE		((void __iomem *)(MODULES_VADDR - SZ_32M))
+#define IO_SPACE_LIMIT		(PCI_IO_END - PCI_IO_START - 1)
+#define PCI_IOBASE		((void __iomem *)PCI_IO_START)
 
 /*
  * String version of I/O memory access operations.
diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
index 6486b2b..a183577 100644
--- a/arch/arm64/include/asm/memory.h
+++ b/arch/arm64/include/asm/memory.h
@@ -45,7 +45,9 @@ 
 #define PAGE_OFFSET		(UL(0xffffffffffffffff) << (VA_BITS - 1))
 #define MODULES_END		(PAGE_OFFSET)
 #define MODULES_VADDR		(MODULES_END - SZ_64M)
-#define FIXADDR_TOP		(MODULES_VADDR - SZ_2M - PAGE_SIZE)
+#define PCI_IO_END		(MODULES_VADDR - SZ_2M)
+#define PCI_IO_START		(PCI_IO_END - SZ_32M)
+#define FIXADDR_TOP		(PCI_IO_START - SZ_2M)
 #define TASK_SIZE_64		(UL(1) << VA_BITS)
 
 #ifdef CONFIG_COMPAT
diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
index cf33f33..203a6cf 100644
--- a/arch/arm64/mm/dump.c
+++ b/arch/arm64/mm/dump.c
@@ -52,8 +52,8 @@  static struct addr_marker address_markers[] = {
 	{ 0,			"vmemmap start" },
 	{ 0,			"vmemmap end" },
 #endif
-	{ (unsigned long) PCI_IOBASE,		"PCI I/O start" },
-	{ (unsigned long) PCI_IOBASE + SZ_16M,	"PCI I/O end" },
+	{ PCI_IO_START,		"PCI I/O start" },
+	{ PCI_IO_END,		"PCI I/O end" },
 	{ FIXADDR_START,	"Fixmap start" },
 	{ FIXADDR_TOP,		"Fixmap end" },
 	{ MODULES_VADDR,	"Modules start" },
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index bac492c..9f2406d 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -35,6 +35,7 @@ 
 #include <linux/efi.h>
 
 #include <asm/fixmap.h>
+#include <asm/memory.h>
 #include <asm/sections.h>
 #include <asm/setup.h>
 #include <asm/sizes.h>
@@ -291,7 +292,7 @@  void __init mem_init(void)
 		  MLM((unsigned long)virt_to_page(PAGE_OFFSET),
 		      (unsigned long)virt_to_page(high_memory)),
 #endif
-		  MLM((unsigned long)PCI_IOBASE, (unsigned long)PCI_IOBASE + SZ_16M),
+		  MLM(PCI_IO_START, PCI_IO_END),
 		  MLK(FIXADDR_START, FIXADDR_TOP),
 		  MLM(MODULES_VADDR, MODULES_END),
 		  MLM(PAGE_OFFSET, (unsigned long)high_memory),