x86, mm: set NX across entire PMD at boot

When setting up permissions on kernel memory at boot, the end of the PMD that was split from bss remained executable. It should be NX like the rest. This performs a PMD alignment instead of a PAGE alignment to get the correct span of memory, and should be freed.

Before:
---[ High Kernel Mapping ]---
...

0xffffffff8202d000-0xffffffff82200000  1868K     RW       GLB NX pte
0xffffffff82200000-0xffffffff82c00000    10M     RW   PSE GLB NX pmd
0xffffffff82c00000-0xffffffff82df5000  2004K     RW       GLB NX pte
0xffffffff82df5000-0xffffffff82e00000    44K     RW       GLB x  pte

0xffffffff82e00000-0xffffffffc0000000 978M pmd

After:
---[ High Kernel Mapping ]---
...

0xffffffff8202d000-0xffffffff82200000  1868K     RW       GLB NX pte
0xffffffff82200000-0xffffffff82c00000    10M     RW   PSE GLB NX pmd
0xffffffff82c00000-0xffffffff82df5000  2004K     RW       GLB NX pte
0xffffffff82df5000-0xffffffff82e00000    44K     RW           NX pte

0xffffffff82e00000-0xffffffffc0000000 978M pmd

---
v2:
- added call to free_init_pages(), as suggested by tglx

---
 arch/x86/mm/init_64.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 4cb8763868fc..0d498c922668 100644
@@ -1124,6 +1124,7 @@ void mark_rodata_ro(void)
 	unsigned long text_end = PFN_ALIGN(&__stop___ex_table);
 	unsigned long rodata_end = PFN_ALIGN(&__end_rodata);
 	unsigned long all_end = PFN_ALIGN(&_end);
+	unsigned long pmd_end = roundup(all_end, PMD_SIZE);
 
 	printk(KERN_INFO "Write protecting the kernel read-only data: %luk\n",
 	       (end - start) >> 10);

@@ -1135,7 +1136,7 @@ void mark_rodata_ro(void)
 	 * The rodata/data/bss/brk section (but not the kernel text!)
 	 * should also be not-executable.
 	 */
-	set_memory_nx(rodata_start, (all_end - rodata_start) >> PAGE_SHIFT);
+	set_memory_nx(rodata_start, (pmd_end - rodata_start) >> PAGE_SHIFT);
 
 	rodata_test();
 

@@ -1147,6 +1148,7 @@ void mark_rodata_ro(void)
 	set_memory_ro(start, (end-start) >> PAGE_SHIFT);
 #endif
 
+	free_init_pages("unused kernel", all_end, pmd_end);
 	free_init_pages("unused kernel",
 			(unsigned long) __va(__pa_symbol(text_end)),
 			(unsigned long) __va(__pa_symbol(rodata_start)));

1.9.1


-- 
Kees Cook
Chrome OS Security

....
should use attached one instead.

1. should use _brk_end instead of &end, as we only use partial of brk.
2. [_brk_end, pm_end) page range is already converted. aka is not wasted.

---
 arch/x86/mm/init_64.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/mm/init_64.c
===================================================================
@@ -1124,7 +1124,8 @@ void mark_rodata_ro(void)
     unsigned long end = (unsigned long) &__end_rodata_hpage_align;
     unsigned long text_end = PFN_ALIGN(&__stop___ex_table);
     unsigned long rodata_end = PFN_ALIGN(&__end_rodata);
-    unsigned long all_end = PFN_ALIGN(&_end);
+    unsigned long all_end = PFN_ALIGN(_brk_end);
+    unsigned long pmd_end = roundup(all_end, PMD_SIZE);
 
     printk(KERN_INFO "Write protecting the kernel read-only data: %luk\n",
            (end - start) >> 10);

@@ -1136,7 +1137,7 @@ void mark_rodata_ro(void)
      * The rodata/data/bss/brk section (but not the kernel text!)
      * should also be not-executable.
      */
-    set_memory_nx(rodata_start, (all_end - rodata_start) >> PAGE_SHIFT);
+    set_memory_nx(rodata_start, (pmd_end - rodata_start) >> PAGE_SHIFT);
 
     rodata_test();
 

@@ -1148,6 +1149,7 @@ void mark_rodata_ro(void)
     set_memory_ro(start, (end-start) >> PAGE_SHIFT);
 #endif
 
+    /* all_end to pmd_end is handled via free_all_bootmem() */
     free_init_pages("unused kernel",
             (unsigned long) __va(__pa_symbol(text_end)),
             (unsigned long) __va(__pa_symbol(rodata_start)));

1. should use _brk_end instead of &end, as we only use partial of brk.
2. [_brk_end, pm_end) page range is already converted. aka is not wasted.

---
 arch/x86/mm/init_64.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/mm/init_64.c
===================================================================
@@ -1124,7 +1124,8 @@ void mark_rodata_ro(void)
 	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
 	unsigned long text_end = PFN_ALIGN(&__stop___ex_table);
 	unsigned long rodata_end = PFN_ALIGN(&__end_rodata);
-	unsigned long all_end = PFN_ALIGN(&_end);
+	unsigned long all_end = PFN_ALIGN(_brk_end);
+	unsigned long pmd_end = roundup(all_end, PMD_SIZE);
 
 	printk(KERN_INFO "Write protecting the kernel read-only data: %luk\n",
 	       (end - start) >> 10);

@@ -1136,7 +1137,7 @@ void mark_rodata_ro(void)
 	 * The rodata/data/bss/brk section (but not the kernel text!)
 	 * should also be not-executable.
 	 */
-	set_memory_nx(rodata_start, (all_end - rodata_start) >> PAGE_SHIFT);
+	set_memory_nx(rodata_start, (pmd_end - rodata_start) >> PAGE_SHIFT);
 
 	rodata_test();
 

@@ -1148,6 +1149,7 @@ void mark_rodata_ro(void)
 	set_memory_ro(start, (end-start) >> PAGE_SHIFT);
 #endif
 
+	/* all_end to pmd_end is handled via free_all_bootmem() */
 	free_init_pages("unused kernel",
 			(unsigned long) __va(__pa_symbol(text_end)),
 			(unsigned long) __va(__pa_symbol(rodata_start)));

Are you sure? For me, _brk_end isn't far enough:

[    1.475572] all_end: 0xffffffff82df5000
[    1.476736] _brk_end: 0xffffffff82dd6000

0xffffffff82200000-0xffffffff82c00000    10M     RW   PSE GLB NX pmd
0xffffffff82c00000-0xffffffff82df5000  2004K     RW       GLB NX pte
0xffffffff82df5000-0xffffffff82e00000    44K     RW           NX pte

Kees Cook
Chrome OS Security

Yes. _brk_end should be small then &_end.

Need to remove GLB ?

Yinghai

Please check attached one that clean up the highmap tail...

Subject: [RFC PATCH] x86, 64bit: cleanup highmap tail near partial 2M range

1. should use _brk_end instead of &end, as we only use partial of brk.
2. [_brk_end, pm_end) page range is already converted mem. and is not wasted.
3. add cleanup_highmap_tail for [_brk_end, pm_end).

Kernel Layout:

[    0.000000]   .text: [0x01000000-0x0200d5c8]
[    0.000000] .rodata: [0x02200000-0x02a1cfff]
[    0.000000]   .data: [0x02c00000-0x02e50e7f]
[    0.000000]   .init: [0x02e52000-0x03212fff]
[    0.000000]    .bss: [0x03221000-0x0437bfff]
[    0.000000]    .brk: [0x0437c000-0x043a1fff]

Actually used brk:

[    0.272959] memblock_reserve: [0x0000000437c000-0x00000004382fff]

flags 0x0 BRK

Before patch:
---[ High Kernel Mapping ]---

0xffffffff80000000-0xffffffff81000000          16M                           pmd
0xffffffff81000000-0xffffffff82200000          18M     ro         PSE GLB x  pmd
0xffffffff82200000-0xffffffff82c00000          10M     ro         PSE GLB NX pmd
0xffffffff82c00000-0xffffffff82e00000           2M     RW         PSE GLB NX pmd
0xffffffff82e00000-0xffffffff83000000           2M     RW             GLB NX pte
0xffffffff83000000-0xffffffff83200000           2M     RW         PSE GLB NX pmd
0xffffffff83200000-0xffffffff83400000           2M     RW             GLB NX pte
0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff843a2000        1672K     RW             GLB NX pte
0xffffffff843a2000-0xffffffff84400000         376K     RW             GLB x  pte
0xffffffff84400000-0xffffffffa0000000         444M                           pmd
After patch:
---[ High Kernel Mapping ]---
0xffffffff80000000-0xffffffff81000000          16M                           pmd
0xffffffff81000000-0xffffffff82200000          18M     ro         PSE GLB x  pmd
0xffffffff82200000-0xffffffff82c00000          10M     ro         PSE GLB NX pmd
0xffffffff82c00000-0xffffffff82e00000           2M     RW         PSE GLB NX pmd
0xffffffff82e00000-0xffffffff83000000           2M     RW             GLB NX pte
0xffffffff83000000-0xffffffff83200000           2M     RW         PSE GLB NX pmd
0xffffffff83200000-0xffffffff83400000           2M     RW             GLB NX pte
0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff84383000        1548K     RW             GLB NX pte
0xffffffff84383000-0xffffffff84400000         500K                           pte
0xffffffff84400000-0xffffffffa0000000         444M                           pmd

---
 arch/x86/mm/init_64.c  |   23 ++++++++++++++++++++++-
 arch/x86/mm/pageattr.c |    2 +-
 2 files changed, 23 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/mm/init_64.c
===================================================================
@@ -375,6 +375,7 @@ void __init init_extra_mapping_uc(unsign
     __init_extra_mapping(phys, size, PAGE_KERNEL_LARGE_NOCACHE);
 }
 
+static pmd_t *last_pmd;
 /*
  * The head.S code sets up the kernel high mapping:
  *

@@ -408,9 +409,26 @@ void __init cleanup_highmap(void)
             continue;
         if (vaddr < (unsigned long) _text || vaddr > end)
             set_pmd(pmd, __pmd(0));
+        else
+            last_pmd = pmd;
     }
 }
 
+static void __init cleanup_highmap_tail(unsigned long addr)
+{
+    int i;
+    pte_t *pte;
+
+    if (!last_pmd || pmd_none(*last_pmd))
+        return;
+
+    pte = (pte_t *)pmd_page_vaddr(*last_pmd);
+    pte += pte_index(addr);
+
+    for (i = pte_index(addr); i < PTRS_PER_PTE; i++, pte++)
+        set_pte(pte, __pte(0));
+}
+
 static unsigned long __meminit
 phys_pte_init(pte_t *pte_page, unsigned long addr, unsigned long end,
           pgprot_t prot)

@@ -1124,7 +1142,8 @@ void mark_rodata_ro(void)
     unsigned long end = (unsigned long) &__end_rodata_hpage_align;
     unsigned long text_end = PFN_ALIGN(&__stop___ex_table);
     unsigned long rodata_end = PFN_ALIGN(&__end_rodata);
-    unsigned long all_end = PFN_ALIGN(&_end);
+    unsigned long all_end = PFN_ALIGN(_brk_end);
+    unsigned long pmd_end = roundup(all_end, PMD_SIZE);
 
     printk(KERN_INFO "Write protecting the kernel read-only data: %luk\n",
            (end - start) >> 10);

@@ -1137,6 +1156,8 @@ void mark_rodata_ro(void)
      * should also be not-executable.
      */
     set_memory_nx(rodata_start, (all_end - rodata_start) >> PAGE_SHIFT);
+    if (all_end < pmd_end)
+        cleanup_highmap_tail(all_end);
 
     rodata_test();
 

Index: linux-2.6/arch/x86/mm/pageattr.c
===================================================================
@@ -100,7 +100,7 @@ static inline unsigned long highmap_star
 
 static inline unsigned long highmap_end_pfn(void)
 {
-    return __pa_symbol(roundup(_brk_end, PMD_SIZE)) >> PAGE_SHIFT;
+    return __pa_symbol(PFN_ALIGN(_brk_end)) >> PAGE_SHIFT;
 }
 
 #endif

Subject: [RFC PATCH] x86, 64bit: cleanup highmap tail near partial 2M range

1. should use _brk_end instead of &end, as we only use partial of brk.
2. [_brk_end, pm_end) page range is already converted mem. and is not wasted.
3. add cleanup_highmap_tail for [_brk_end, pm_end).

Kernel Layout:

[    0.000000]   .text: [0x01000000-0x0200d5c8]
[    0.000000] .rodata: [0x02200000-0x02a1cfff]
[    0.000000]   .data: [0x02c00000-0x02e50e7f]
[    0.000000]   .init: [0x02e52000-0x03212fff]
[    0.000000]    .bss: [0x03221000-0x0437bfff]
[    0.000000]    .brk: [0x0437c000-0x043a1fff]

Actually used brk:

[    0.272959] memblock_reserve: [0x0000000437c000-0x00000004382fff] flags 0x0 BRK

Before patch:
---[ High Kernel Mapping ]---

0xffffffff80000000-0xffffffff81000000          16M                           pmd
0xffffffff81000000-0xffffffff82200000          18M     ro         PSE GLB x  pmd
0xffffffff82200000-0xffffffff82c00000          10M     ro         PSE GLB NX pmd
0xffffffff82c00000-0xffffffff82e00000           2M     RW         PSE GLB NX pmd
0xffffffff82e00000-0xffffffff83000000           2M     RW             GLB NX pte
0xffffffff83000000-0xffffffff83200000           2M     RW         PSE GLB NX pmd
0xffffffff83200000-0xffffffff83400000           2M     RW             GLB NX pte
0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff843a2000        1672K     RW             GLB NX pte
0xffffffff843a2000-0xffffffff84400000         376K     RW             GLB x  pte
0xffffffff84400000-0xffffffffa0000000         444M                           pmd
After patch:
---[ High Kernel Mapping ]---
0xffffffff80000000-0xffffffff81000000          16M                           pmd
0xffffffff81000000-0xffffffff82200000          18M     ro         PSE GLB x  pmd
0xffffffff82200000-0xffffffff82c00000          10M     ro         PSE GLB NX pmd
0xffffffff82c00000-0xffffffff82e00000           2M     RW         PSE GLB NX pmd
0xffffffff82e00000-0xffffffff83000000           2M     RW             GLB NX pte
0xffffffff83000000-0xffffffff83200000           2M     RW         PSE GLB NX pmd
0xffffffff83200000-0xffffffff83400000           2M     RW             GLB NX pte
0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff84383000        1548K     RW             GLB NX pte
0xffffffff84383000-0xffffffff84400000         500K                           pte
0xffffffff84400000-0xffffffffa0000000         444M                           pmd

---
 arch/x86/mm/init_64.c  |   23 ++++++++++++++++++++++-
 arch/x86/mm/pageattr.c |    2 +-
 2 files changed, 23 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/mm/init_64.c
===================================================================
@@ -375,6 +375,7 @@ void __init init_extra_mapping_uc(unsign
 	__init_extra_mapping(phys, size, PAGE_KERNEL_LARGE_NOCACHE);
 }
 
+static pmd_t *last_pmd;
 /*
  * The head.S code sets up the kernel high mapping:
  *

@@ -408,9 +409,26 @@ void __init cleanup_highmap(void)
 			continue;
 		if (vaddr < (unsigned long) _text || vaddr > end)
 			set_pmd(pmd, __pmd(0));
+		else
+			last_pmd = pmd;
 	}
 }
 
+static void __init cleanup_highmap_tail(unsigned long addr)
+{
+	int i;
+	pte_t *pte;
+
+	if (!last_pmd || pmd_none(*last_pmd))
+		return;
+
+	pte = (pte_t *)pmd_page_vaddr(*last_pmd);
+	pte += pte_index(addr);
+
+	for (i = pte_index(addr); i < PTRS_PER_PTE; i++, pte++)
+		set_pte(pte, __pte(0));
+}
+
 static unsigned long __meminit
 phys_pte_init(pte_t *pte_page, unsigned long addr, unsigned long end,
 	      pgprot_t prot)

@@ -1124,7 +1142,8 @@ void mark_rodata_ro(void)
 	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
 	unsigned long text_end = PFN_ALIGN(&__stop___ex_table);
 	unsigned long rodata_end = PFN_ALIGN(&__end_rodata);
-	unsigned long all_end = PFN_ALIGN(&_end);
+	unsigned long all_end = PFN_ALIGN(_brk_end);
+	unsigned long pmd_end = roundup(all_end, PMD_SIZE);
 
 	printk(KERN_INFO "Write protecting the kernel read-only data: %luk\n",
 	       (end - start) >> 10);

@@ -1137,6 +1156,8 @@ void mark_rodata_ro(void)
 	 * should also be not-executable.
 	 */
 	set_memory_nx(rodata_start, (all_end - rodata_start) >> PAGE_SHIFT);
+	if (all_end < pmd_end)
+		cleanup_highmap_tail(all_end);
 
 	rodata_test();
 

Index: linux-2.6/arch/x86/mm/pageattr.c
===================================================================
@@ -100,7 +100,7 @@ static inline unsigned long highmap_star
 
 static inline unsigned long highmap_end_pfn(void)
 {
-	return __pa_symbol(roundup(_brk_end, PMD_SIZE)) >> PAGE_SHIFT;
+	return __pa_symbol(PFN_ALIGN(_brk_end)) >> PAGE_SHIFT;
 }
 
 #endif

I'm not quite sure about the xen path.

I missed that.

Please check this one that address three problems that you pointed out.

Subject: [PATCH v2] x86, 64bit: cleanup highmap tail near partial 2M range

1. should use _brk_end instead of &_end in mark_rodata_ro(). _brk_end can move up to &_end, i.e. to __brk_limit. It's safe to use _brk_end when mark_rodata_ro() is called because extend_brk() is gone already at that point.
2. [_brk_end, pm_end) page range is already converted mem. and is not wasted.
3. add cleanup_highmap_tail for [_brk_end, pm_end).

Kernel Layout:

[    0.000000]    .brk: [0x0437c000-0x043a1fff]

Actually used brk:

[    0.272959] memblock_reserve: [0x0000000437c000-0x00000004382fff]

flags 0x0 BRK

Before patch:
---[ High Kernel Mapping ]---
...

0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff843a2000        1672K     RW             GLB NX pte
0xffffffff843a2000-0xffffffff84400000         376K     RW             GLB x  pte

0xffffffff84400000-0xffffffffa0000000 444M pmd
After patch:
---[ High Kernel Mapping ]---
...
0xffffffff83400000-0xffffffff84200000 14M RW PSE GLB NX pmd
0xffffffff84200000-0xffffffff84383000 1548K RW GLB NX pte

0xffffffff84383000-0xffffffff84400000         500K                           pte
0xffffffff84400000-0xffffffffa0000000         444M                           pmd

-v2: according to tglx
caculate the pmd postion instead of passing last_pmd. cleanup_highmap_tail could not have __init, as it is called in mark_rodata_ro and mark_rodata_ro is called after free_initmem.
highmap_end_pfn should keep PMD_SIZE alignment on !CONFIG_DEBUG_RODATA

---
 arch/x86/mm/init_64.c  |   22 +++++++++++++++++++++-
 arch/x86/mm/pageattr.c |    4 ++++
 2 files changed, 25 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/mm/init_64.c
===================================================================
@@ -411,6 +411,23 @@ void __init cleanup_highmap(void)
     }
 }
 
+static void cleanup_highmap_tail(unsigned long addr)
+{
+    int i;
+    pgd_t *pgd;
+    pud_t *pud;
+    pmd_t *pmd;
+    pte_t *pte;
+
+    pgd = pgd_offset_k(addr);
+    pud = (pud_t *)pgd_page_vaddr(*pgd) + pud_index(addr);
+    pmd = (pmd_t *)pud_page_vaddr(*pud) + pmd_index(addr);
+    pte = (pte_t *)pmd_page_vaddr(*pmd) + pte_index(addr);
+
+    for (i = pte_index(addr); i < PTRS_PER_PTE; i++, pte++)
+        set_pte(pte, __pte(0));
+}
+
 static unsigned long __meminit
 phys_pte_init(pte_t *pte_page, unsigned long addr, unsigned long end,
           pgprot_t prot)

@@ -1124,7 +1141,8 @@ void mark_rodata_ro(void)
     unsigned long end = (unsigned long) &__end_rodata_hpage_align;
     unsigned long text_end = PFN_ALIGN(&__stop___ex_table);
     unsigned long rodata_end = PFN_ALIGN(&__end_rodata);
-    unsigned long all_end = PFN_ALIGN(&_end);
+    unsigned long all_end = PFN_ALIGN(_brk_end);
+    unsigned long pmd_end = roundup(all_end, PMD_SIZE);
 
     printk(KERN_INFO "Write protecting the kernel read-only data: %luk\n",
            (end - start) >> 10);

@@ -1137,6 +1155,8 @@ void mark_rodata_ro(void)
      * should also be not-executable.
      */
     set_memory_nx(rodata_start, (all_end - rodata_start) >> PAGE_SHIFT);
+    if (all_end < pmd_end)
+        cleanup_highmap_tail(all_end);
 
     rodata_test();
 

Index: linux-2.6/arch/x86/mm/pageattr.c
===================================================================
@@ -100,7 +100,11 @@ static inline unsigned long highmap_star
 
 static inline unsigned long highmap_end_pfn(void)
 {
+#ifdef CONFIG_DEBUG_RODATA
+    return __pa_symbol(PFN_ALIGN(_brk_end)) >> PAGE_SHIFT;
+#else
     return __pa_symbol(roundup(_brk_end, PMD_SIZE)) >> PAGE_SHIFT;
+#endif
 }
 
 #endif

Subject: [PATCH v2] x86, 64bit: cleanup highmap tail near partial 2M range

1. should use _brk_end instead of &_end in mark_rodata_ro(). _brk_end can move up to &_end, i.e. to __brk_limit. It's safe to use _brk_end when mark_rodata_ro() is called because extend_brk() is gone already at that point.
2. [_brk_end, pm_end) page range is already converted mem. and is not wasted.
3. add cleanup_highmap_tail for [_brk_end, pm_end).

Kernel Layout:

[    0.000000]    .brk: [0x0437c000-0x043a1fff]

Actually used brk:

[    0.272959] memblock_reserve: [0x0000000437c000-0x00000004382fff] flags 0x0 BRK

Before patch:
---[ High Kernel Mapping ]---
...

0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff843a2000        1672K     RW             GLB NX pte
0xffffffff843a2000-0xffffffff84400000         376K     RW             GLB x  pte

0xffffffff84400000-0xffffffffa0000000 444M pmd
After patch:
---[ High Kernel Mapping ]---
...
0xffffffff83400000-0xffffffff84200000 14M RW PSE GLB NX pmd
0xffffffff84200000-0xffffffff84383000 1548K RW GLB NX pte

0xffffffff84383000-0xffffffff84400000         500K                           pte
0xffffffff84400000-0xffffffffa0000000         444M                           pmd

-v2: according to tglx
caculate the pmd postion instead of passing last_pmd. cleanup_highmap_tail could not have __init, as it is called in mark_rodata_ro and mark_rodata_ro is called after free_initmem.
highmap_end_pfn should keep PMD_SIZE alignment on !CONFIG_DEBUG_RODATA

---
 arch/x86/mm/init_64.c  |   22 +++++++++++++++++++++-
 arch/x86/mm/pageattr.c |    4 ++++
 2 files changed, 25 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/mm/init_64.c
===================================================================
@@ -411,6 +411,23 @@ void __init cleanup_highmap(void)
 	}
 }
 
+static void cleanup_highmap_tail(unsigned long addr)
+{
+	int i;
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+	pte_t *pte;
+
+	pgd = pgd_offset_k(addr);
+	pud = (pud_t *)pgd_page_vaddr(*pgd) + pud_index(addr);
+	pmd = (pmd_t *)pud_page_vaddr(*pud) + pmd_index(addr);
+	pte = (pte_t *)pmd_page_vaddr(*pmd) + pte_index(addr);
+
+	for (i = pte_index(addr); i < PTRS_PER_PTE; i++, pte++)
+		set_pte(pte, __pte(0));
+}
+
 static unsigned long __meminit
 phys_pte_init(pte_t *pte_page, unsigned long addr, unsigned long end,
 	      pgprot_t prot)

@@ -1124,7 +1141,8 @@ void mark_rodata_ro(void)
 	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
 	unsigned long text_end = PFN_ALIGN(&__stop___ex_table);
 	unsigned long rodata_end = PFN_ALIGN(&__end_rodata);
-	unsigned long all_end = PFN_ALIGN(&_end);
+	unsigned long all_end = PFN_ALIGN(_brk_end);
+	unsigned long pmd_end = roundup(all_end, PMD_SIZE);
 
 	printk(KERN_INFO "Write protecting the kernel read-only data: %luk\n",
 	       (end - start) >> 10);

@@ -1137,6 +1155,8 @@ void mark_rodata_ro(void)
 	 * should also be not-executable.
 	 */
 	set_memory_nx(rodata_start, (all_end - rodata_start) >> PAGE_SHIFT);
+	if (all_end < pmd_end)
+		cleanup_highmap_tail(all_end);
 
 	rodata_test();
 

Index: linux-2.6/arch/x86/mm/pageattr.c
===================================================================
@@ -100,7 +100,11 @@ static inline unsigned long highmap_star
 
 static inline unsigned long highmap_end_pfn(void)
 {
+#ifdef CONFIG_DEBUG_RODATA
+	return __pa_symbol(PFN_ALIGN(_brk_end)) >> PAGE_SHIFT;
+#else
 	return __pa_symbol(roundup(_brk_end, PMD_SIZE)) >> PAGE_SHIFT;
+#endif
 }
 
 #endif

Wrong. _brk_end can move up to _end, i.e. to __brk_limit.

But it's safe to use _brk_end when mark_rodata_ro() is called because extend_brk() is gone already at that point.

Thanks,

tglx

Confused. What is wrong? _brk_end is not smaller than &_end?

Yinghai

_brk_end is adjusted at boot time via extend_brk() up to __brk_limit, which is the same as _end. We usually do not use all of that space. So it's expected that _brk_end < _end.

Yes, I wanted to get rid of the high mapping for anything between _brk_end and _end, and I brought you on the wrong track with my suggestion to call free_init_pages(). Sorry about that.

That happened because I missed the completely non obvious fact, that only the effective brk section is reserved for the kernel via reserve_brk(). So the area between _brk_end and _end is already reusable. Though that reuse works only by chance and not by design and is completely undocumented as everything else in that area.

So the initial patch to get rid of the X mapping is of course to just extend the area to the PMD. A little bit different to your initial patch, but essentially the same.

- unsigned long all_end = PFN_ALIGN(&_end);
+ unsigned long all_end = roundup((unsigned long) &_end, PMD_SIZE);

I'm going to apply your V1 patch with the above roundup() simplification. If a page of that area gets used later on then we are going to split up the PMD anyway.

But still we want to get rid of that highmap between _brk_end and _end, but there is absolutely no reason to come up with extra silly functions for that.

So the obvious solution is to let setup_arch() reserve the memory up to _end instead of _bss_stop, get rid of the extra reservation in reserve_brk() and then let free_initmem() release the area between _brk_end and _end. No extra hackery, no side effects, just works.

I spent quite some time to stare into that and I wonder about the following related issues:

1) Why is the mark_rodata_ro() business a debug configuration, i.e CONFIG_DEBUG_RODATA?

This does not make any sense at all. We really want RO and NX on by default and AFAICT distros are turning that on anyway for obvious reasons.

The only idiocity I found so far is the kgdb Documentation which recommends to turn it off. Sigh.

So that should be changed to:

config ARCH_HAS_RONX
bool

config DISABLE_RONX
def_bool !ARCH_HAS_RONX || !KGDB_ENABLE_SECURITY_HOLES

plus

      config ARCH_WANTS_KGDB_SECURITY_HOLES
      	  bool

      config KGDB_ENABLE_SECURITY_HOLES
      	  bool "WTF?"

depends on BROKEN && ARCH_WANTS_KGDB_SECURITY_HOLES

2) What is actually the modules counterpart for mark_rodata_ro()?

CONFIG_DEBUG_SET_MODULE_RONX

Of course not enabled by default, but enabled by distros again.

See #1.

Now what's interesting aside of the general fuckup is that CONFIG_DEBUG_RODATA is supported by:

arch/x86 and arch/parisc

But CONFIG_DEBUG_SET_MODULE_RONX is supported by:

     arch/arm/ arch/arm64 arch/s390 and arch/x86

This does not make any sense at all.

Do arm/arm64/s390 have other means to make RO/NX work or are they just doing it for modules? And how is that supposed to work with KGDB if it is not aware of modules sections being RO/NX? KGDB has only extra magic for CONFIG_DEBUG_RODATA, but not for CONFIG_DEBUG_SET_MODULE_RONX.

Now for extended fun the x86 help text for that option says:

... Such protection may interfere with run-time code patching and dynamic kernel tracing - and they might also protect against certain classes of kernel exploits.
If in doubt, say "N".

Patently wrong. More sigh.

3) Why is mark_rodata_ro() called AFTER free_initmem() and therefor cannot be marked __init ?

Just because ...

Thanks,

tglx

:)

where is info for everything else?

should use _brk_end instead of &_end for the roundup?

So where get the highmap to be removed?
free_init_pages via free_initmem()?

with the code change like you suggested,

---
 arch/x86/kernel/setup.c |    6 +-----
 arch/x86/mm/init.c      |    6 ++++++
 arch/x86/mm/init_64.c   |    2 +-
 3 files changed, 8 insertions(+), 6 deletions(-)

Index: linux-2.6/arch/x86/kernel/setup.c
===================================================================
@@ -286,10 +286,6 @@ static void __init cleanup_highmap(void)
 
 static void __init reserve_brk(void)
 {
-    if (_brk_end > _brk_start)
-        memblock_reserve(__pa_symbol(_brk_start),
-                 _brk_end - _brk_start);
-
     /* Mark brk area as locked down and no longer taking any
        new allocations */
     _brk_start = 0;

@@ -857,7 +853,7 @@ dump_kernel_offset(struct notifier_block
 void __init setup_arch(char **cmdline_p)
 {
     memblock_reserve(__pa_symbol(_text),
-             (unsigned long)__bss_stop - (unsigned long)_text);
+             (unsigned long)_end - (unsigned long)_text);
 
     early_reserve_initrd();
 

Index: linux-2.6/arch/x86/mm/init_64.c
===================================================================
@@ -1122,7 +1122,7 @@ void mark_rodata_ro(void)
     unsigned long end = (unsigned long) &__end_rodata_hpage_align;
     unsigned long text_end = PFN_ALIGN(&__stop___ex_table);
     unsigned long rodata_end = PFN_ALIGN(&__end_rodata);
-    unsigned long all_end = PFN_ALIGN(&_end);
+    unsigned long all_end = roundup((unsigned long)&_end, PMD_SIZE);
 
     printk(KERN_INFO "Write protecting the kernel read-only data: %luk\n",
            (end - start) >> 10);

Index: linux-2.6/arch/x86/mm/init.c
===================================================================
@@ -637,9 +637,15 @@ void free_init_pages(char *what, unsigne
 
 void free_initmem(void)
 {
+    unsigned long all_end = roundup((unsigned long)&_end, PMD_SIZE);
+
     free_init_pages("unused kernel",
             (unsigned long)(&__init_begin),
             (unsigned long)(&__init_end));
+
+#ifdef CONFIG_X86_64
+    free_init_pages("unused brk", PFN_ALIGN(_brk_end), all_end);
+#endif
 }
 
 #ifdef CONFIG_BLK_DEV_INITRD

got

[    7.942636] Freeing unused brk memory: 500K (ffffffff84383000 -

ffffffff84400000)

---[ High Kernel Mapping ]---

0xffffffff80000000-0xffffffff81000000          16M                           pmd
0xffffffff81000000-0xffffffff82200000          18M     ro         PSE GLB x  pmd
0xffffffff82200000-0xffffffff82c00000          10M     ro         PSE GLB NX pmd
0xffffffff82c00000-0xffffffff82e00000           2M     RW         PSE GLB NX pmd
0xffffffff82e00000-0xffffffff83000000           2M     RW             GLB NX pte
0xffffffff83000000-0xffffffff83200000           2M     RW         PSE GLB NX pmd
0xffffffff83200000-0xffffffff83400000           2M     RW             GLB NX pte
0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff84400000           2M     RW             GLB NX pte

0xffffffff84400000-0xffffffffa0000000 444M pmd

the _brk_end to &_end highmap is still there

Also if you want to remove highmap between _brk_end and _end, Do you want highmap for

                        [text_end, rodata_start)
                        [rodata_end, _sdata),

as mark_rodata_ro is calling

        free_init_pages("unused kernel",
                        (unsigned long) __va(__pa_symbol(text_end)),
                        (unsigned long) __va(__pa_symbol(rodata_start)));
        free_init_pages("unused kernel",
                        (unsigned long) __va(__pa_symbol(rodata_end)),
                        (unsigned long) __va(__pa_symbol(_sdata)));

Thanks

Yinghai

That's fine by me. Yinghai Lu seems to have potentially better solutions, but my head is spinning after reading more of this thread. :) I just want to make sure that at the end of the day, there are no RW+x mappings.

This is a historically badly named config item. Once arm and arm64 land their CONFIG_DEBUG_RODATA implementations, I was going to suggest having this renamed.

I have a series for arm that is waiting to be picked up by rmk:
https://patchwork.ozlabs.org/patch/400383/

Laura has been working on a series for arm64:
http://comments.gmane.org/gmane.linux.ports.arm.kernel/366777

So what you're seeing is us being in the middle of providing support for it. It just happens that CONFIG_DEBUG_SET_MODULE_RONX is a bit easier to implement, so it was completed first.

Like the CONFIG naming, I hope to start cleaning these defaults up once arm and arm64 are landed.

That's worth examining. Since most of the logic that does the ro/nx work needs to stick around for things like ftrace, kgdb, etc, it's possible there wouldn't be much savings from making mark_rodata_ro as __init. I'll add this to my list to check.

Thanks!

-Kees

Kees Cook
Chrome OS Security

It should've been in linux-next via my tree for the last two weeks or so.

FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Fantastic! Thank you!

-Kees

Kees Cook
Chrome OS Security

Please check v3 that cleanup highmap in the middle.

Before patch:
---[ High Kernel Mapping ]---

0xffffffff80000000-0xffffffff81000000          16M                           pmd
0xffffffff81000000-0xffffffff82200000          18M     ro         PSE GLB x  pmd
0xffffffff82200000-0xffffffff82c00000          10M     ro         PSE GLB NX pmd
0xffffffff82c00000-0xffffffff82e00000           2M     RW         PSE GLB NX pmd
0xffffffff82e00000-0xffffffff83000000           2M     RW             GLB NX pte
0xffffffff83000000-0xffffffff83200000           2M     RW         PSE GLB NX pmd
0xffffffff83200000-0xffffffff83400000           2M     RW             GLB NX pte
0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff843a2000        1672K     RW             GLB NX pte
0xffffffff843a2000-0xffffffff84400000         376K     RW             GLB x  pte

0xffffffff84400000-0xffffffffa0000000 444M pmd

After patch:
---[ High Kernel Mapping ]---

0xffffffff80000000-0xffffffff81000000          16M                           pmd
0xffffffff81000000-0xffffffff82000000          16M     ro         PSE GLB x  pmd
0xffffffff82000000-0xffffffff82011000          68K     ro             GLB x  pte
0xffffffff82011000-0xffffffff82200000        1980K                           pte

0xffffffff82200000-0xffffffff82a00000 8M ro PSE GLB NX pmd
0xffffffff82a00000-0xffffffff82a1d000 116K ro GLB NX pte
0xffffffff82a1d000-0xffffffff82c00000 1932K pte
0xffffffff82c00000-0xffffffff82e00000 2M RW PSE GLB NX pmd
0xffffffff82e00000-0xffffffff82e52000 328K RW GLB NX pte

0xffffffff82e52000-0xffffffff83000000        1720K                           pte
0xffffffff83000000-0xffffffff83200000           2M                           pmd
0xffffffff83200000-0xffffffff83213000          76K                           pte
0xffffffff83213000-0xffffffff83400000        1972K     RW             GLB NX pte
0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff84383000        1548K     RW             GLB NX pte
0xffffffff84383000-0xffffffff84400000         500K                           pte
0xffffffff84400000-0xffffffffa0000000         444M                           pmd

Thanks

Yinghai

Subject: [PATCH 1/2] x86, 64bit: add pfn_range_is_highmapped()

will use it to support holes in highmap.

---
 arch/x86/include/asm/pgtable_64.h |    2 ++
 arch/x86/mm/init_64.c             |   22 ++++++++++++++++++++++
 arch/x86/mm/pageattr.c            |   16 +---------------
 3 files changed, 25 insertions(+), 15 deletions(-)

Index: linux-2.6/arch/x86/mm/init_64.c
===================================================================
@@ -375,6 +375,23 @@ void __init init_extra_mapping_uc(unsign
 	__init_extra_mapping(phys, size, PAGE_KERNEL_LARGE_NOCACHE);
 }
 
+/* three holes at most*/
+#define NR_RANGE 4
+static struct range pfn_highmapped[NR_RANGE];
+static int nr_pfn_highmapped;
+
+int pfn_range_is_highmapped(unsigned long start_pfn, unsigned long end_pfn)
+{
+	int i;
+
+	for (i = 0; i < nr_pfn_highmapped; i++)
+		if ((start_pfn >= pfn_highmapped[i].start) &&
+		    (end_pfn <= pfn_highmapped[i].end))
+			return 1;
+
+	return 0;
+}
+
 /*
  * The head.S code sets up the kernel high mapping:
  *

@@ -409,6 +426,11 @@ void __init cleanup_highmap(void)
 		if (vaddr < (unsigned long) _text || vaddr > end)
 			set_pmd(pmd, __pmd(0));
 	}
+
+	nr_pfn_highmapped = add_range(pfn_highmapped, NR_RANGE,
+			nr_pfn_highmapped,
+			__pa_symbol(_text) >> PAGE_SHIFT,
+			__pa_symbol(roundup(_brk_end, PMD_SIZE)) >> PAGE_SHIFT);
 }
 
 static unsigned long __meminit

Index: linux-2.6/arch/x86/mm/pageattr.c
===================================================================
@@ -91,20 +91,6 @@ void arch_report_meminfo(struct seq_file
 static inline void split_page_count(int level) { }
 #endif
 
-#ifdef CONFIG_X86_64
-
-static inline unsigned long highmap_start_pfn(void)
-{
-	return __pa_symbol(_text) >> PAGE_SHIFT;
-}
-
-static inline unsigned long highmap_end_pfn(void)
-{
-	return __pa_symbol(roundup(_brk_end, PMD_SIZE)) >> PAGE_SHIFT;
-}
-
-#endif
-
 #ifdef CONFIG_DEBUG_PAGEALLOC
 # define debug_pagealloc 1
 #else

@@ -1242,7 +1228,7 @@ static int cpa_process_alias(struct cpa_
 	 * to touch the high mapped kernel as well:
 	 */
 	if (!within(vaddr, (unsigned long)_text, _brk_end) &&
-	    within(cpa->pfn, highmap_start_pfn(), highmap_end_pfn())) {
+	    pfn_range_is_highmapped(cpa->pfn, 1)) {
 		unsigned long temp_cpa_vaddr = (cpa->pfn << PAGE_SHIFT) +
 					       __START_KERNEL_map - phys_base;
 		alias_cpa = *cpa;

Index: linux-2.6/arch/x86/include/asm/pgtable_64.h
===================================================================
@@ -167,6 +167,8 @@ static inline int pgd_large(pgd_t pgd) {
 extern int kern_addr_valid(unsigned long addr);
 extern void cleanup_highmap(void);
 
+int pfn_range_is_highmapped(unsigned long start_pfn, unsigned long end_pfn);
+
 #define HAVE_ARCH_UNMAPPED_AREA
 #define HAVE_ARCH_UNMAPPED_AREA_TOPDOWN
 

Subject: [PATCH 2/2] x86, 64bit: cleanup highmap late for not needed range

1. should use _brk_end instead of &_end in mark_rodata_ro(). _brk_end can move up to &_end, i.e. to __brk_limit. It's safe to use _brk_end when mark_rodata_ro() is called because extend_brk() is gone already at that point.
2. add cleanup_highmap_late for range for initmem, around rodata, and [_brk_end, pmd_end]

Kernel Layout:

[    0.000000]   .text: [0x01000000-0x0200d608]
[    0.000000] .rodata: [0x02200000-0x02a1cfff]
[    0.000000]   .data: [0x02c00000-0x02e50e7f]
[    0.000000]   .init: [0x02e52000-0x03212fff]
[    0.000000]    .bss: [0x03221000-0x0437bfff]
[    0.000000]    .brk: [0x0437c000-0x043a1fff]

Actually used brk:

[    0.272959] memblock_reserve: [0x0000000437c000-0x00000004382fff] flags 0x0 BRK

Before patch:
---[ High Kernel Mapping ]---

0xffffffff80000000-0xffffffff81000000          16M                           pmd
0xffffffff81000000-0xffffffff82200000          18M     ro         PSE GLB x  pmd
0xffffffff82200000-0xffffffff82c00000          10M     ro         PSE GLB NX pmd
0xffffffff82c00000-0xffffffff82e00000           2M     RW         PSE GLB NX pmd
0xffffffff82e00000-0xffffffff83000000           2M     RW             GLB NX pte
0xffffffff83000000-0xffffffff83200000           2M     RW         PSE GLB NX pmd
0xffffffff83200000-0xffffffff83400000           2M     RW             GLB NX pte
0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff843a2000        1672K     RW             GLB NX pte
0xffffffff843a2000-0xffffffff84400000         376K     RW             GLB x  pte

0xffffffff84400000-0xffffffffa0000000 444M pmd

After patch:
---[ High Kernel Mapping ]---

0xffffffff80000000-0xffffffff81000000          16M                           pmd
0xffffffff81000000-0xffffffff82000000          16M     ro         PSE GLB x  pmd
0xffffffff82000000-0xffffffff82011000          68K     ro             GLB x  pte
0xffffffff82011000-0xffffffff82200000        1980K                           pte

0xffffffff82200000-0xffffffff82a00000 8M ro PSE GLB NX pmd
0xffffffff82a00000-0xffffffff82a1d000 116K ro GLB NX pte
0xffffffff82a1d000-0xffffffff82c00000 1932K pte
0xffffffff82c00000-0xffffffff82e00000 2M RW PSE GLB NX pmd
0xffffffff82e00000-0xffffffff82e52000 328K RW GLB NX pte

0xffffffff82e52000-0xffffffff83000000        1720K                           pte
0xffffffff83000000-0xffffffff83200000           2M                           pmd
0xffffffff83200000-0xffffffff83213000          76K                           pte
0xffffffff83213000-0xffffffff83400000        1972K     RW             GLB NX pte
0xffffffff83400000-0xffffffff84200000          14M     RW         PSE GLB NX pmd
0xffffffff84200000-0xffffffff84383000        1548K     RW             GLB NX pte
0xffffffff84383000-0xffffffff84400000         500K                           pte
0xffffffff84400000-0xffffffffa0000000         444M                           pmd

-v3: remove all not used highmap ranges.

---
 arch/x86/mm/init_64.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

_brk_end is guaranteed to be <= _end. But we really want to use _brk_end here, because if we have the following situation:

_brk_start:     0x03ff0000
_brk_end:       0x03ffff00
_end:           0x04016000

So we have the following PMDs setup:

0x03e00000 pmd rw nx <- covers the top of .bss
and the start of .brk
0x04000000 pmd rw nx <- covers the end of .brk
and some random unused

So if _brk_end is less than 0x04000000, then cleanup_highmem() has zapped the extra PMD already. So we don't want to call set_nx() on that.

If _brk_end is >= 0x04000000 then we cover that last pmd with the set_nx call.

Completely non obvious as anything else in that area.

Thanks,

tglx

Also, what tree is this? "Freeing %s" went away in c88442ec45f30d587b38b935a14acde4e217a926 (and should probably be re-added, which is what I assume has happened.)

And which CONFIG turns on this reporting?

-Kees

Kees Cook
Chrome OS Security

My local patch.

this brk does not shrink yet.

Yinghai