From: Russell Currey <hidden> Date: 2020-02-26 06:46:01
Repost from v4 because I somehow missed half of the final patch.
Picking up from Christophe's last series, including the following changes:
- [6/8] Cast "data" to unsigned long instead of int to fix build
- [8/8] New, to fix an issue reported by Jordan Niethe
Christophe's last series is here:
https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=156428
Christophe Leroy (4):
powerpc/mm: Implement set_memory() routines
powerpc/kprobes: Mark newly allocated probes as RO
powerpc/mm: implement set_memory_attr()
powerpc/32: use set_memory_attr()
Russell Currey (4):
powerpc/mm/ptdump: debugfs handler for W+X checks at runtime
powerpc: Set ARCH_HAS_STRICT_MODULE_RWX
powerpc/configs: Enable STRICT_MODULE_RWX in skiroot_defconfig
powerpc/mm: Disable set_memory() routines when strict RWX isn't
enabled
arch/powerpc/Kconfig | 2 +
arch/powerpc/Kconfig.debug | 6 +-
arch/powerpc/configs/skiroot_defconfig | 1 +
arch/powerpc/include/asm/set_memory.h | 34 +++++++
arch/powerpc/kernel/kprobes.c | 17 +++-
arch/powerpc/mm/Makefile | 2 +-
arch/powerpc/mm/pageattr.c | 117 +++++++++++++++++++++++++
arch/powerpc/mm/pgtable_32.c | 95 +++-----------------
arch/powerpc/mm/ptdump/ptdump.c | 21 ++++-
9 files changed, 202 insertions(+), 93 deletions(-)
create mode 100644 arch/powerpc/include/asm/set_memory.h
create mode 100644 arch/powerpc/mm/pageattr.c
--
2.25.1
From: Russell Currey <hidden> Date: 2020-02-26 06:47:53
From: Christophe Leroy <redacted>
The set_memory_{ro/rw/nx/x}() functions are required for STRICT_MODULE_RWX,
and are generally useful primitives to have. This implementation is
designed to be completely generic across powerpc's many MMUs.
It's possible that this could be optimised to be faster for specific
MMUs, but the focus is on having a generic and safe implementation for
now.
This implementation does not handle cases where the caller is attempting
to change the mapping of the page it is executing from, or if another
CPU is concurrently using the page being altered. These cases likely
shouldn't happen, but a more complex implementation with MMU-specific code
could safely handle them, so that is left as a TODO for now.
Signed-off-by: Russell Currey <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/set_memory.h | 32 ++++++++++++
arch/powerpc/mm/Makefile | 2 +-
arch/powerpc/mm/pageattr.c | 74 +++++++++++++++++++++++++++
4 files changed, 108 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/include/asm/set_memory.h
create mode 100644 arch/powerpc/mm/pageattr.c
@@ -0,0 +1,74 @@+// SPDX-License-Identifier: GPL-2.0++/*+*MMU-genericset_memoryimplementationforpowerpc+*+*Copyright2019,IBMCorporation.+*/++#include<linux/mm.h>+#include<linux/set_memory.h>++#include<asm/mmu.h>+#include<asm/page.h>+#include<asm/pgtable.h>+++/*+*Updatestheattributesofapageinthreesteps:+*+*1.invalidatethepagetableentry+*2.flushtheTLB+*3.installthenewentrywiththeupdatedattributes+*+*Thisisunsafeifthecallerisattemptingtochangethemappingofthe+*pageitisexecutingfrom,orifanotherCPUisconcurrentlyusingthe+*pagebeingaltered.+*+*TODOmaketheimplementationresistanttothis.+*/+staticintchange_page_attr(pte_t*ptep,unsignedlongaddr,void*data)+{+longaction=(long)data;+pte_tpte;++spin_lock(&init_mm.page_table_lock);++/* invalidate the PTE so it's safe to modify */+pte=ptep_get_and_clear(&init_mm,addr,ptep);+flush_tlb_kernel_range(addr,addr+PAGE_SIZE);++/* modify the PTE bits as desired, then apply */+switch(action){+caseSET_MEMORY_RO:+pte=pte_wrprotect(pte);+break;+caseSET_MEMORY_RW:+pte=pte_mkwrite(pte);+break;+caseSET_MEMORY_NX:+pte=pte_exprotect(pte);+break;+caseSET_MEMORY_X:+pte=pte_mkexec(pte);+break;+default:+break;+}++set_pte_at(&init_mm,addr,ptep,pte);+spin_unlock(&init_mm.page_table_lock);++return0;+}++intchange_memory_attr(unsignedlongaddr,intnumpages,longaction)+{+unsignedlongstart=ALIGN_DOWN(addr,PAGE_SIZE);+unsignedlongsz=numpages*PAGE_SIZE;++if(!numpages)+return0;++returnapply_to_page_range(&init_mm,start,sz,change_page_attr,(void*)action);+}
From: Russell Currey <hidden> Date: 2020-02-26 06:49:25
From: Christophe Leroy <redacted>
With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be one
W+X page at boot by default. This can be tested with
CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the
kernel log during boot.
powerpc doesn't implement its own alloc() for kprobes like other
architectures do, but we couldn't immediately mark RO anyway since we do
a memcpy to the page we allocate later. After that, nothing should be
allowed to modify the page, and write permissions are removed well
before the kprobe is armed.
The memcpy() would fail if >1 probes were allocated, so use
patch_instruction() instead which is safe for RO.
Reviewed-by: Daniel Axtens <redacted>
Signed-off-by: Russell Currey <redacted>
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/kernel/kprobes.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
From: Russell Currey <hidden> Date: 2020-02-26 06:51:04
Very rudimentary, just
echo 1 > [debugfs]/check_wx_pages
and check the kernel log. Useful for testing strict module RWX.
Updated the Kconfig entry to reflect this.
Also fixed a typo.
Signed-off-by: Russell Currey <redacted>
---
arch/powerpc/Kconfig.debug | 6 ++++--
arch/powerpc/mm/ptdump/ptdump.c | 21 ++++++++++++++++++++-
2 files changed, 24 insertions(+), 3 deletions(-)
@@ -370,7 +370,7 @@ config PPC_PTDUMPIfyouareunsure,sayN.configPPC_DEBUG_WX-bool"Warn on W+X mappings at boot"+bool"Warn on W+X mappings at boot & enable manual checks at runtime"depends onPPC_PTDUMP&&STRICT_KERNEL_RWXhelpGenerateawarningifanyW+Xmappingsarefoundatboot.
From: Russell Currey <hidden> Date: 2020-02-26 06:52:59
To enable strict module RWX on powerpc, set:
CONFIG_STRICT_MODULE_RWX=y
You should also have CONFIG_STRICT_KERNEL_RWX=y set to have any real
security benefit.
ARCH_HAS_STRICT_MODULE_RWX is set to require ARCH_HAS_STRICT_KERNEL_RWX.
This is due to a quirk in arch/Kconfig and arch/powerpc/Kconfig that
makes STRICT_MODULE_RWX *on by default* in configurations where
STRICT_KERNEL_RWX is *unavailable*.
Since this doesn't make much sense, and module RWX without kernel RWX
doesn't make much sense, having the same dependencies as kernel RWX
works around this problem.
Signed-off-by: Russell Currey <redacted>
---
arch/powerpc/Kconfig | 1 +
1 file changed, 1 insertion(+)
From: Russell Currey <hidden> Date: 2020-02-26 06:54:53
From: Christophe Leroy <redacted>
Use set_memory_attr() instead of the PPC32 specific change_page_attr()
change_page_attr() was checking that the address was not mapped by
blocks and was handling highmem, but that's unneeded because the
affected pages can't be in highmem and block mapping verification
is already done by the callers.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/mm/pgtable_32.c | 95 ++++--------------------------------
1 file changed, 10 insertions(+), 85 deletions(-)
@@ -121,99 +122,20 @@ void __init mapin_ram(void)}}-/* Scan the real Linux page tables and return a PTE pointer for-*avirtualaddressinacontext.-*Returnstrue(1)ifPTEwasfound,zerootherwise.Thepointerto-*thePTEpointerisunmodifiedifPTEisnotfound.-*/-staticint-get_pteptr(structmm_struct*mm,unsignedlongaddr,pte_t**ptep,pmd_t**pmdp)-{-pgd_t*pgd;-pud_t*pud;-pmd_t*pmd;-pte_t*pte;-intretval=0;--pgd=pgd_offset(mm,addr&PAGE_MASK);-if(pgd){-pud=pud_offset(pgd,addr&PAGE_MASK);-if(pud&&pud_present(*pud)){-pmd=pmd_offset(pud,addr&PAGE_MASK);-if(pmd_present(*pmd)){-pte=pte_offset_map(pmd,addr&PAGE_MASK);-if(pte){-retval=1;-*ptep=pte;-if(pmdp)-*pmdp=pmd;-/* XXX caller needs to do pte_unmap, yuck */-}-}-}-}-return(retval);-}--staticint__change_page_attr_noflush(structpage*page,pgprot_tprot)-{-pte_t*kpte;-pmd_t*kpmd;-unsignedlongaddress;--BUG_ON(PageHighMem(page));-address=(unsignedlong)page_address(page);--if(v_block_mapped(address))-return0;-if(!get_pteptr(&init_mm,address,&kpte,&kpmd))-return-EINVAL;-__set_pte_at(&init_mm,address,kpte,mk_pte(page,prot),0);-pte_unmap(kpte);--return0;-}--/*-*Changethepageattributesofanpageinthelinearmapping.-*-*THISDOESNOTHINGWITHBATMAPPINGS,DEBUGUSEONLY-*/-staticintchange_page_attr(structpage*page,intnumpages,pgprot_tprot)-{-inti,err=0;-unsignedlongflags;-structpage*start=page;--local_irq_save(flags);-for(i=0;i<numpages;i++,page++){-err=__change_page_attr_noflush(page,prot);-if(err)-break;-}-wmb();-local_irq_restore(flags);-flush_tlb_kernel_range((unsignedlong)page_address(start),-(unsignedlong)page_address(page));-returnerr;-}-voidmark_initmem_nx(void){-structpage*page=virt_to_page(_sinittext);unsignedlongnumpages=PFN_UP((unsignedlong)_einittext)-PFN_DOWN((unsignedlong)_sinittext);if(v_block_mapped((unsignedlong)_stext+1))mmu_mark_initmem_nx();else-change_page_attr(page,numpages,PAGE_KERNEL);+set_memory_attr((unsignedlong)_sinittext,numpages,PAGE_KERNEL);}#ifdef CONFIG_STRICT_KERNEL_RWXvoidmark_rodata_ro(void){-structpage*page;unsignedlongnumpages;if(v_block_mapped((unsignedlong)_sinittext)){
@@ -222,20 +144,18 @@ void mark_rodata_ro(void)return;}-page=virt_to_page(_stext);numpages=PFN_UP((unsignedlong)_etext)-PFN_DOWN((unsignedlong)_stext);-change_page_attr(page,numpages,PAGE_KERNEL_ROX);+set_memory_attr((unsignedlong)_stext,numpages,PAGE_KERNEL_ROX);/**mark.rodataasreadonly.Use__init_beginratherthan__end_rodata*tocoverNOTESandEXCEPTION_TABLE.*/-page=virt_to_page(__start_rodata);numpages=PFN_UP((unsignedlong)__init_begin)-PFN_DOWN((unsignedlong)__start_rodata);-change_page_attr(page,numpages,PAGE_KERNEL_RO);+set_memory_attr((unsignedlong)__start_rodata,numpages,PAGE_KERNEL_RO);// mark_initmem_nx() should have already run by nowptdump_check_wx();
From: Russell Currey <hidden> Date: 2020-02-26 06:56:44
There are a couple of reasons that the set_memory() functions are
problematic when STRICT_KERNEL_RWX isn't enabled:
- The linear mapping is a different size and apply_to_page_range()
may modify a giant section, breaking everything
- patch_instruction() doesn't know to work around a page being marked
RO, and will subsequently crash
The latter can be replicated by building a kernel with the set_memory()
patches but with STRICT_KERNEL_RWX off and running ftracetest.
Reported-by: Jordan Niethe <redacted>
Signed-off-by: Russell Currey <redacted>
---
v5: Apply to both set_memory_attr() and change_memory_attr()
v4: New
arch/powerpc/mm/pageattr.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
From: Andrew Donnellan <hidden> Date: 2020-02-26 06:58:27
On 26/2/20 5:35 pm, Russell Currey wrote:
There are a couple of reasons that the set_memory() functions are
problematic when STRICT_KERNEL_RWX isn't enabled:
- The linear mapping is a different size and apply_to_page_range()
may modify a giant section, breaking everything
- patch_instruction() doesn't know to work around a page being marked
RO, and will subsequently crash
The latter can be replicated by building a kernel with the set_memory()
patches but with STRICT_KERNEL_RWX off and running ftracetest.
Reported-by: Jordan Niethe <redacted>
Signed-off-by: Russell Currey <redacted>
Can we squash this in earlier in the series for the sake of bisectability?
--
Andrew Donnellan OzLabs, ADL Canberra
ajd@linux.ibm.com IBM Australia Limited
From: Russell Currey <hidden> Date: 2020-02-26 07:00:00
skiroot_defconfig is the only powerpc defconfig with STRICT_KERNEL_RWX
enabled, and if you want memory protection for kernel text you'd want it
for modules too, so enable STRICT_MODULE_RWX there.
Acked-by: Joel Stanley <redacted>
Signed-off-by: Russell Currey <redacted>
---
arch/powerpc/configs/skiroot_defconfig | 1 +
1 file changed, 1 insertion(+)
From: Russell Currey <hidden> Date: 2020-02-26 07:01:47
From: Christophe Leroy <redacted>
In addition to the set_memory_xx() functions which allows to change
the memory attributes of not (yet) used memory regions, implement a
set_memory_attr() function to:
- set the final memory protection after init on currently used
kernel regions.
- enable/disable kernel memory regions in the scope of DEBUG_PAGEALLOC.
Unlike the set_memory_xx() which can act in three step as the regions
are unused, this function must modify 'on the fly' as the kernel is
executing from them. At the moment only PPC32 will use it and changing
page attributes on the fly is not an issue.
Signed-off-by: Christophe Leroy <redacted>
Reported-by: kbuild test robot <redacted>
[ruscur: cast "data" to unsigned long instead of int]
Signed-off-by: Russell Currey <redacted>
---
v4: cast "data" to unsigned long instead of int
arch/powerpc/include/asm/set_memory.h | 2 ++
arch/powerpc/mm/pageattr.c | 33 +++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
@@ -29,4 +29,6 @@ static inline int set_memory_x(unsigned long addr, int numpages)returnchange_memory_attr(addr,numpages,SET_MEMORY_X);}+intset_memory_attr(unsignedlongaddr,intnumpages,pgprot_tprot);+#endif
@@ -72,3 +72,36 @@ int change_memory_attr(unsigned long addr, int numpages, long action)returnapply_to_page_range(&init_mm,start,sz,change_page_attr,(void*)action);}++/*+*Settheattributesofapage:+*+*ThisfunctionisusedbyPPC32attheendofinittosetfinalkernelmemory+*protection.Itincludeschangingthemapingofthepageitisexecutingfrom+*anddatapagesitisusing.+*/+staticintset_page_attr(pte_t*ptep,unsignedlongaddr,void*data)+{+pgprot_tprot=__pgprot((unsignedlong)data);++spin_lock(&init_mm.page_table_lock);++set_pte_at(&init_mm,addr,ptep,pte_modify(*ptep,prot));+flush_tlb_kernel_range(addr,addr+PAGE_SIZE);++spin_unlock(&init_mm.page_table_lock);++return0;+}++intset_memory_attr(unsignedlongaddr,intnumpages,pgprot_tprot)+{+unsignedlongstart=ALIGN_DOWN(addr,PAGE_SIZE);+unsignedlongsz=numpages*PAGE_SIZE;++if(!numpages)+return0;++returnapply_to_page_range(&init_mm,start,sz,set_page_attr,+(void*)pgprot_val(prot));+}
There are a couple of reasons that the set_memory() functions are
problematic when STRICT_KERNEL_RWX isn't enabled:
- The linear mapping is a different size and apply_to_page_range()
may modify a giant section, breaking everything
- patch_instruction() doesn't know to work around a page being marked
RO, and will subsequently crash
The latter can be replicated by building a kernel with the set_memory()
patches but with STRICT_KERNEL_RWX off and running ftracetest.
I agree with Andrew, those changes should go into patch 1.
Reported-by: Jordan Niethe <redacted>
Signed-off-by: Russell Currey <redacted>
---
v5: Apply to both set_memory_attr() and change_memory_attr()
@@ -64,13 +64,18 @@ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)intchange_memory_attr(unsignedlongaddr,intnumpages,longaction){-unsignedlongstart=ALIGN_DOWN(addr,PAGE_SIZE);-unsignedlongsz=numpages*PAGE_SIZE;+unsignedlongstart,size;++if(!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))+return0;if(!numpages)return0;-returnapply_to_page_range(&init_mm,start,sz,change_page_attr,(void*)action);+start=ALIGN_DOWN(addr,PAGE_SIZE);+size=numpages*PAGE_SIZE;++returnapply_to_page_range(&init_mm,start,size,change_page_attr,(void*)action);
You don't need to move start and sz initialisation, neither you need to
change the name of sz to size.
If you want to rename sz to size, do it in the initial patch, but take
care of the length of the lines. IIRC I used a short name to have the
line fit on a single line with no more than 90 chars.
Christophe
quoted hunk
}
/*
@@ -96,12 +101,17 @@ static int set_page_attr(pte_t *ptep, unsigned long addr, void *data) int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot) {- unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);- unsigned long sz = numpages * PAGE_SIZE;+ unsigned long start, size;++ if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))+ return 0; if (!numpages) return 0;- return apply_to_page_range(&init_mm, start, sz, set_page_attr,+ start = ALIGN_DOWN(addr, PAGE_SIZE);+ size = numpages * PAGE_SIZE;++ return apply_to_page_range(&init_mm, start, size, set_page_attr, (void *)pgprot_val(prot)); }
On Wed, Feb 26, 2020 at 05:35:46PM +1100, Russell Currey wrote:
Very rudimentary, just
echo 1 > [debugfs]/check_wx_pages
and check the kernel log. Useful for testing strict module RWX.
Updated the Kconfig entry to reflect this.
Oh, I like this! This would be handy to have on all architectures.
Reviewed-by: Kees Cook <redacted>
-Kees
quoted hunk
Also fixed a typo.
Signed-off-by: Russell Currey <redacted>
---
arch/powerpc/Kconfig.debug | 6 ++++--
arch/powerpc/mm/ptdump/ptdump.c | 21 ++++++++++++++++++++-
2 files changed, 24 insertions(+), 3 deletions(-)
@@ -370,7 +370,7 @@ config PPC_PTDUMPIfyouareunsure,sayN.configPPC_DEBUG_WX-bool"Warn on W+X mappings at boot"+bool"Warn on W+X mappings at boot & enable manual checks at runtime"depends onPPC_PTDUMP&&STRICT_KERNEL_RWXhelpGenerateawarningifanyW+Xmappingsarefoundatboot.
*thread necromancy*
Is this patch still something folks are working on? It'd be nice to be
able to trigger this check at runtime.
-Kees
On Wed, Feb 26, 2020 at 05:35:46PM +1100, Russell Currey wrote:
quoted hunk
Very rudimentary, just
echo 1 > [debugfs]/check_wx_pages
and check the kernel log. Useful for testing strict module RWX.
Updated the Kconfig entry to reflect this.
Also fixed a typo.
Signed-off-by: Russell Currey <redacted>
---
arch/powerpc/Kconfig.debug | 6 ++++--
arch/powerpc/mm/ptdump/ptdump.c | 21 ++++++++++++++++++++-
2 files changed, 24 insertions(+), 3 deletions(-)
@@ -370,7 +370,7 @@ config PPC_PTDUMPIfyouareunsure,sayN.configPPC_DEBUG_WX-bool"Warn on W+X mappings at boot"+bool"Warn on W+X mappings at boot & enable manual checks at runtime"depends onPPC_PTDUMP&&STRICT_KERNEL_RWXhelpGenerateawarningifanyW+Xmappingsarefoundatboot.
*thread necromancy*
Is this patch still something folks are working on? It'd be nice to be
able to trigger this check at runtime.
The series was taken over by Jordan.
v15 of the series was accepted, but that particular patch was dropped in
v11 (see
https://patchwork.ozlabs.org/project/linuxppc-dev/cover/20210429031602.2606654-1-jniethe5@gmail.com/)
The comment was:
Some patches were dropped from this revision:
powerpc/mm/ptdump: debugfs handler for W+X checks at runtime
- Will use Christophe's generic ptdump series
Not sure what happened after that.
Christophe
-Kees
On Wed, Feb 26, 2020 at 05:35:46PM +1100, Russell Currey wrote:
quoted
Very rudimentary, just
echo 1 > [debugfs]/check_wx_pages
and check the kernel log. Useful for testing strict module RWX.
Updated the Kconfig entry to reflect this.
Also fixed a typo.
Signed-off-by: Russell Currey <redacted>
---
arch/powerpc/Kconfig.debug | 6 ++++--
arch/powerpc/mm/ptdump/ptdump.c | 21 ++++++++++++++++++++-
2 files changed, 24 insertions(+), 3 deletions(-)
@@ -370,7 +370,7 @@ config PPC_PTDUMPIfyouareunsure,sayN.configPPC_DEBUG_WX-bool"Warn on W+X mappings at boot"+bool"Warn on W+X mappings at boot & enable manual checks at runtime"depends onPPC_PTDUMP&&STRICT_KERNEL_RWXhelpGenerateawarningifanyW+Xmappingsarefoundatboot.