From: Russell Currey <hidden> Date: 2020-02-26 06:26:41
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:30:12
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:32:03
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:33:49
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:35:39
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 06:38:08
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));+}
From: Russell Currey <hidden> Date: 2020-02-26 06:39:44
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:41:32
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>
---
v4: new
arch/powerpc/mm/pageattr.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
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
I don't understand.
- patch_instruction() doesn't know to work around a page being marked
RO, and will subsequently crash
Is patch_instruction() involved at all ?
quoted hunk
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>
---
v4: new
arch/powerpc/mm/pageattr.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
@@ -96,12 +96,17 @@ static int set_page_attr(pte_t *ptep, unsigned long addr, void *data)intset_memory_attr(unsignedlongaddr,intnumpages,pgprot_tprot)
Isn't it change_memory_attr() that is a problem for you ?
{
- 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;
Doing this you break patch 7:
mark_initmem_nx() is called regardless of CONFIG_STRICT_KERNEL_RWX
__kernel_map_pages() depends on CONFIG_DEBUG_PAGEALLOC which doesn't
depend on CONFIG_STRICT_KERNEL_RWX