arm64: mm: use correct mapping granularity under DEBUG_RODATA

PATCH New Ard Biesheuvel 4 participants 8 messages Nov 16, 2015 – Nov 17, 2015 arm64

Share & tools

Ard Biesheuvel

author

2015-11-16 10:18 UTC #0 lore raw

PATCH 1/1 arm64: mm: use correct mapping granularity under DEBUG_RODATA

When booting a 64k pages kernel that is built with CONFIG_DEBUG_RODATA and resides at an offset that is not a multiple of 512 MB, the rounding that occurs in __map_memblock() and fixup_executable() results in incorrect regions being mapped.

The following snippet from /sys/kernel/debug/kernel_page_tables shows how, when the kernel is loaded 2 MB above the base of DRAM at 0x40000000, the first 2 MB of memory (which may be inaccessible from non-secure EL1 or just reserved by the firmware) is inadvertently mapped into the end of the module region.

---[ Modules start ]---
0xfffffdffffe00000-0xfffffe0000000000 2M RW NX ... UXN MEM/NORMAL
---[ Modules end ]---
---[ Kernel Mapping ]---
0xfffffe0000000000-0xfffffe0000090000 576K RW NX ... UXN MEM/NORMAL

  0xfffffe0000090000-0xfffffe0000200000  1472K ro x  ... UXN MEM/NORMAL
  0xfffffe0000200000-0xfffffe0000800000     6M ro x  ... UXN MEM/NORMAL
  0xfffffe0000800000-0xfffffe0000810000    64K ro x  ... UXN MEM/NORMAL

0xfffffe0000810000-0xfffffe0000a00000 1984K RW NX ... UXN MEM/NORMAL 0xfffffe0000a00000-0xfffffe00ffe00000 4084M RW NX ... UXN MEM/NORMAL

The same issue is likely to occur on 16k pages kernels whose load address is not a multiple of 32 MB (i.e., SECTION_SIZE). So round to SWAPPER_BLOCK_SIZE instead of SECTION_SIZE.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/mm/mmu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

arch/arm64/mm/mmu.c +6 -6

index e3f563c81c48..32ddd893da9a 100644
@@ -362,8 +362,8 @@ static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
 	 * for now. This will get more fine grained later once all memory
 	 * is mapped
 	 */
-	unsigned long kernel_x_start = round_down(__pa(_stext), SECTION_SIZE);
-	unsigned long kernel_x_end = round_up(__pa(__init_end), SECTION_SIZE);
+	unsigned long kernel_x_start = round_down(__pa(_stext), SWAPPER_BLOCK_SIZE);
+	unsigned long kernel_x_end = round_up(__pa(__init_end), SWAPPER_BLOCK_SIZE);
 
 	if (end < kernel_x_start) {
 		create_mapping(start, __phys_to_virt(start),

@@ -451,18 +451,18 @@ static void __init fixup_executable(void)
 {
 #ifdef CONFIG_DEBUG_RODATA
 	/* now that we are actually fully mapped, make the start/end more fine grained */
-	if (!IS_ALIGNED((unsigned long)_stext, SECTION_SIZE)) {
+	if (!IS_ALIGNED((unsigned long)_stext, SWAPPER_BLOCK_SIZE)) {
 		unsigned long aligned_start = round_down(__pa(_stext),
-							SECTION_SIZE);
+							 SWAPPER_BLOCK_SIZE);
 
 		create_mapping(aligned_start, __phys_to_virt(aligned_start),
 				__pa(_stext) - aligned_start,
 				PAGE_KERNEL);
 	}
 
-	if (!IS_ALIGNED((unsigned long)__init_end, SECTION_SIZE)) {
+	if (!IS_ALIGNED((unsigned long)__init_end, SWAPPER_BLOCK_SIZE)) {
 		unsigned long aligned_end = round_up(__pa(__init_end),
-							SECTION_SIZE);
+							  SWAPPER_BLOCK_SIZE);
 		create_mapping(__pa(__init_end), (unsigned long)__init_end,
 				aligned_end - __pa(__init_end),
 				PAGE_KERNEL);

Signature · 1 line

1.9.1

Mark Rutland

reviewer

2015-11-16 11:57 UTC #1 lore raw

PATCH 1/1 arm64: mm: use correct mapping granularity under DEBUG_RODATA

Hi Ard,

Quoted 10 lines · Ard Biesheuvel

When booting a 64k pages kernel that is built with CONFIG_DEBUG_RODATA and resides at an offset that is not a multiple of 512 MB, the rounding that occurs in __map_memblock() and fixup_executable() results in incorrect regions being mapped.
The following snippet from /sys/kernel/debug/kernel_page_tables shows how, when the kernel is loaded 2 MB above the base of DRAM at 0x40000000, the first 2 MB of memory (which may be inaccessible from non-secure EL1 or just reserved by the firmware) is inadvertently mapped into the end of the module region.

I assume for the above that you mean the kernel is loaded TEXT_OFFSET above these addresses. It's only a nit, but it might make things harder for otehrs to reason about in future.

Perhaps re-word in terms of PHYS_OFFSET/memstart_addr? e.g. "when PHYS_OFFSET is not a multiple of SECTION_SIZE ...".

Quoted 8 lines

---[ Modules start ]---
0xfffffdffffe00000-0xfffffe0000000000 2M RW NX ... UXN MEM/NORMAL
---[ Modules end ]---
---[ Kernel Mapping ]---
0xfffffe0000000000-0xfffffe0000090000 576K RW NX ... UXN MEM/NORMAL
  0xfffffe0000090000-0xfffffe0000200000  1472K ro x  ... UXN MEM/NORMAL
  0xfffffe0000200000-0xfffffe0000800000     6M ro x  ... UXN MEM/NORMAL
  0xfffffe0000800000-0xfffffe0000810000    64K ro x  ... UXN MEM/NORMAL

Quoted 8 lines

0xfffffe0000810000-0xfffffe0000a00000 1984K RW NX ... UXN MEM/NORMAL 0xfffffe0000a00000-0xfffffe00ffe00000 4084M RW NX ... UXN MEM/NORMAL
The same issue is likely to occur on 16k pages kernels whose load address is not a multiple of 32 MB (i.e., SECTION_SIZE). So round to SWAPPER_BLOCK_SIZE instead of SECTION_SIZE.
Signed-off-by Ard Biesheuvel <[email protected]>

This looks good to me. With or without the rewording above:

Acked-by Mark Rutland <[email protected]>

Does this need to be backported to stable?

Mark.

Quoted 45 lines

---
 arch/arm64/mm/mmu.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

arch/arm64/mm/mmu.c +6 -6

index e3f563c81c48..32ddd893da9a 100644
@@ -362,8 +362,8 @@ static void __init __map_memblock(phys_addr_t start, phys_addr_t end)
 	 * for now. This will get more fine grained later once all memory
 	 * is mapped
 	 */
-	unsigned long kernel_x_start = round_down(__pa(_stext), SECTION_SIZE);
-	unsigned long kernel_x_end = round_up(__pa(__init_end), SECTION_SIZE);
+	unsigned long kernel_x_start = round_down(__pa(_stext), SWAPPER_BLOCK_SIZE);
+	unsigned long kernel_x_end = round_up(__pa(__init_end), SWAPPER_BLOCK_SIZE);
 
 	if (end < kernel_x_start) {
 		create_mapping(start, __phys_to_virt(start),

@@ -451,18 +451,18 @@ static void __init fixup_executable(void)
 {
 #ifdef CONFIG_DEBUG_RODATA
 	/* now that we are actually fully mapped, make the start/end more fine grained */
-	if (!IS_ALIGNED((unsigned long)_stext, SECTION_SIZE)) {
+	if (!IS_ALIGNED((unsigned long)_stext, SWAPPER_BLOCK_SIZE)) {
 		unsigned long aligned_start = round_down(__pa(_stext),
-							SECTION_SIZE);
+							 SWAPPER_BLOCK_SIZE);
 
 		create_mapping(aligned_start, __phys_to_virt(aligned_start),
 				__pa(_stext) - aligned_start,
 				PAGE_KERNEL);
 	}
 
-	if (!IS_ALIGNED((unsigned long)__init_end, SECTION_SIZE)) {
+	if (!IS_ALIGNED((unsigned long)__init_end, SWAPPER_BLOCK_SIZE)) {
 		unsigned long aligned_end = round_up(__pa(__init_end),
-							SECTION_SIZE);
+							  SWAPPER_BLOCK_SIZE);
 		create_mapping(__pa(__init_end), (unsigned long)__init_end,
 				aligned_end - __pa(__init_end),
 				PAGE_KERNEL);

Signature · 1 line

1.9.1

Ard Biesheuvel

author

2015-11-16 12:00 UTC #2 lore raw

PATCH 1/1 arm64: mm: use correct mapping granularity under DEBUG_RODATA

Quoted 14 lines · Mark Rutland

Hi Ard,
Quoted 10 lines · Ard Biesheuvel
When booting a 64k pages kernel that is built with CONFIG_DEBUG_RODATA and resides at an offset that is not a multiple of 512 MB, the rounding that occurs in __map_memblock() and fixup_executable() results in incorrect regions being mapped.
The following snippet from /sys/kernel/debug/kernel_page_tables shows how, when the kernel is loaded 2 MB above the base of DRAM at 0x40000000, the first 2 MB of memory (which may be inaccessible from non-secure EL1 or just reserved by the firmware) is inadvertently mapped into the end of the module region.

Quoted 7 lines

I assume for the above that you mean the kernel is loaded TEXT_OFFSET above these addresses. It's only a nit, but it might make things harder for otehrs to reason about in future.
Perhaps re-word in terms of PHYS_OFFSET/memstart_addr? e.g. "when PHYS_OFFSET is not a multiple of SECTION_SIZE ...".

The clearer the better, so yes, let's reword it.

Quoted 15 lines

Quoted 15 lines
---[ Modules start ]---
0xfffffdffffe00000-0xfffffe0000000000 2M RW NX ... UXN MEM/NORMAL
---[ Modules end ]---
---[ Kernel Mapping ]---
0xfffffe0000000000-0xfffffe0000090000 576K RW NX ... UXN MEM/NORMAL
  0xfffffe0000090000-0xfffffe0000200000  1472K ro x  ... UXN MEM/NORMAL
  0xfffffe0000200000-0xfffffe0000800000     6M ro x  ... UXN MEM/NORMAL
  0xfffffe0000800000-0xfffffe0000810000    64K ro x  ... UXN MEM/NORMAL
0xfffffe0000810000-0xfffffe0000a00000 1984K RW NX ... UXN MEM/NORMAL 0xfffffe0000a00000-0xfffffe00ffe00000 4084M RW NX ... UXN MEM/NORMAL
The same issue is likely to occur on 16k pages kernels whose load address is not a multiple of 32 MB (i.e., SECTION_SIZE). So round to SWAPPER_BLOCK_SIZE instead of SECTION_SIZE.

Quoted 8 lines

Quoted 1 line
Signed-off-by Ard Biesheuvel <[email protected]>
This looks good to me. With or without the rewording above:
Acked-by Mark Rutland <[email protected]>
Does this need to be backported to stable?

It should be fixed in stable, I think, but SWAPPER_BLOCK_SIZE is new in v4.4 so we'd need another patch anyway.

Signature · 1 line

Ard.

Mark Rutland

reviewer

2015-11-16 12:16 UTC #3 lore raw

PATCH 1/1 arm64: mm: use correct mapping granularity under DEBUG_RODATA

Quoted 4 lines

Quoted 1 line
Does this need to be backported to stable?
It should be fixed in stable, I think, but SWAPPER_BLOCK_SIZE is new in v4.4 so we'd need another patch anyway.

I was hoping 87d1587bef394cd8 ("arm64: Move swapper pagetable definitions") would backport easily. I just had a go and that's not the case, and the conflict is non-trivial.

Regardless, we should probably mark this with a fixes tag so it doesn't get lost:

Fixes da141706aea52c1a ("arm64: add better page protections to arm64")

Mark.

Ard Biesheuvel

author

2015-11-16 12:18 UTC #4 lore raw

PATCH 1/1 arm64: mm: use correct mapping granularity under DEBUG_RODATA

Quoted 5 lines · Mark Rutland

Quoted 4 lines
Quoted 1 line
Does this need to be backported to stable?
It should be fixed in stable, I think, but SWAPPER_BLOCK_SIZE is new in v4.4 so we'd need another patch anyway.

Quoted 9 lines

I was hoping 87d1587bef394cd8 ("arm64: Move swapper pagetable definitions") would backport easily. I just had a go and that's not the case, and the conflict is non-trivial.
Regardless, we should probably mark this with a fixes tag so it doesn't get lost:
Fixes da141706aea52c1a ("arm64: add better page protections to arm64")

Indeed.

For the backported version, we can just add a #define SWAPPER_BLOCK_SIZE to mmu.c

Signature · 1 line

Ard.

Catalin Marinas

maintainer

2015-11-17 11:11 UTC #5 lore raw

PATCH 1/1 arm64: mm: use correct mapping granularity under DEBUG_RODATA

Quoted 13 lines · Ard Biesheuvel

Quoted 12 lines · Mark Rutland
Quoted 4 lines
Quoted 1 line
Does this need to be backported to stable?
It should be fixed in stable, I think, but SWAPPER_BLOCK_SIZE is new in v4.4 so we'd need another patch anyway.
I was hoping 87d1587bef394cd8 ("arm64: Move swapper pagetable definitions") would backport easily. I just had a go and that's not the case, and the conflict is non-trivial.
Regardless, we should probably mark this with a fixes tag so it doesn't get lost:

Quoted 6 lines

Quoted 1 line
Fixes da141706aea52c1a ("arm64: add better page protections to arm64")
Indeed.
For the backported version, we can just add a #define SWAPPER_BLOCK_SIZE to mmu.c

Patch applied with a fixes tag. I assume you'll prepare a fix for stable when Greg pings us.

Thanks.

Signature · 1 line

Catalin

Ard Biesheuvel

author

2015-11-17 11:14 UTC #6 lore raw

PATCH 1/1 arm64: mm: use correct mapping granularity under DEBUG_RODATA

Quoted 16 lines · Catalin Marinas

Quoted 15 lines · Ard Biesheuvel
Quoted 13 lines · Mark Rutland
Quoted 4 lines
Quoted 1 line
Does this need to be backported to stable?
It should be fixed in stable, I think, but SWAPPER_BLOCK_SIZE is new in v4.4 so we'd need another patch anyway.
I was hoping 87d1587bef394cd8 ("arm64: Move swapper pagetable definitions") would backport easily. I just had a go and that's not the case, and the conflict is non-trivial.
Regardless, we should probably mark this with a fixes tag so it doesn't get lost:
Fixes da141706aea52c1a ("arm64: add better page protections to arm64")

Quoted 8 lines

Quoted 4 lines
Indeed.
For the backported version, we can just add a #define SWAPPER_BLOCK_SIZE to mmu.c
Patch applied with a fixes tag. I assume you'll prepare a fix for stable when Greg pings us.

Indeed. We should pay attention, though, since the patch most likely applies cleanly to older kernels, and will only break at build time for lack of a definition of SWAPPER_BLOCK_SIZE.

Signature · 1 line

Ard.

Laura Abbott

2015-11-16 23:14 UTC #7 lore raw

PATCH 1/1 arm64: mm: use correct mapping granularity under DEBUG_RODATA

In reply to Ard Biesheuvel

Quoted 22 lines · Ard Biesheuvel

When booting a 64k pages kernel that is built with CONFIG_DEBUG_RODATA and resides at an offset that is not a multiple of 512 MB, the rounding that occurs in __map_memblock() and fixup_executable() results in incorrect regions being mapped.
The following snippet from /sys/kernel/debug/kernel_page_tables shows how, when the kernel is loaded 2 MB above the base of DRAM at 0x40000000, the first 2 MB of memory (which may be inaccessible from non-secure EL1 or just reserved by the firmware) is inadvertently mapped into the end of the module region.
   ---[ Modules start ]---
   0xfffffdffffe00000-0xfffffe0000000000     2M RW NX ... UXN MEM/NORMAL
   ---[ Modules end ]---
   ---[ Kernel Mapping ]---
   0xfffffe0000000000-0xfffffe0000090000   576K RW NX ... UXN MEM/NORMAL
   0xfffffe0000090000-0xfffffe0000200000  1472K ro x  ... UXN MEM/NORMAL
   0xfffffe0000200000-0xfffffe0000800000     6M ro x  ... UXN MEM/NORMAL
   0xfffffe0000800000-0xfffffe0000810000    64K ro x  ... UXN MEM/NORMAL
   0xfffffe0000810000-0xfffffe0000a00000  1984K RW NX ... UXN MEM/NORMAL
   0xfffffe0000a00000-0xfffffe00ffe00000  4084M RW NX ... UXN MEM/NORMAL

Quoted 5 lines

The same issue is likely to occur on 16k pages kernels whose load address is not a multiple of 32 MB (i.e., SECTION_SIZE). So round to SWAPPER_BLOCK_SIZE instead of SECTION_SIZE.
Signed-off-by Ard Biesheuvel <[email protected]>

Acked-by Laura Abbott <[email protected]>

Thanks,
Laura