From: Christopher M. Riedl <hidden> Date: 2020-03-23 05:03:35
When compiled with CONFIG_STRICT_KERNEL_RWX, the kernel must create
temporary mappings when patching itself. These mappings temporarily
override the strict RWX text protections to permit a write. Currently,
powerpc allocates a per-CPU VM area for patching. Patching occurs as
follows:
1. Map page of text to be patched to per-CPU VM area w/
PAGE_KERNEL protection
2. Patch text
3. Remove the temporary mapping
While the VM area is per-CPU, the mapping is actually inserted into the
kernel page tables. Presumably, this could allow another CPU to access
the normally write-protected text - either malicously or accidentally -
via this same mapping if the address of the VM area is known. Ideally,
the mapping should be kept local to the CPU doing the patching (or any
other sensitive operations requiring temporarily overriding memory
protections) [0].
x86 introduced "temporary mm" structs which allow the creation of
mappings local to a particular CPU [1]. This series intends to bring the
notion of a temporary mm to powerpc and harden powerpc by using such a
mapping for patching a kernel with strict RWX permissions.
The first patch introduces the temporary mm struct and API for powerpc
along with a new function to retrieve a current hw breakpoint.
The second patch uses the `poking_init` init hook added by the x86
patches to initialize a temporary mm and patching address. The patching
address is randomized between 0 and DEFAULT_MAP_WINDOW-PAGE_SIZE. The
upper limit is necessary due to how the hash MMU operates - by default
the space above DEFAULT_MAP_WINDOW is not available. For now, both hash
and radix randomize inside this range. The number of possible random
addresses is dependent on PAGE_SIZE and limited by DEFAULT_MAP_WINDOW.
Bits of entropy with 64K page size on BOOK3S_64:
bits-o-entropy = log2(DEFAULT_MAP_WINDOW_USER64 / PAGE_SIZE)
PAGE_SIZE=64K, DEFAULT_MAP_WINDOW_USER64=128TB
bits-o-entropy = log2(128TB / 64K)
bits-o-entropy = 31
Currently, randomization occurs only once during initialization at boot.
The third patch replaces the VM area with the temporary mm in the
patching code. The page for patching has to be mapped PAGE_SHARED with
the hash MMU since hash prevents the kernel from accessing userspace
pages with PAGE_PRIVILEGED bit set. There is on-going work on my side to
explore if this is actually necessary in the hash codepath.
Testing so far is limited to booting on QEMU (power8 and power9 targets)
and a POWER8 VM along with setting some simple xmon breakpoints (which
makes use of code-patching). A POC lkdtm test is in-progress to actually
exploit the existing vulnerability (ie. the mapping during patching is
exposed in kernel page tables and accessible by other CPUS) - this will
accompany a future v1 of this series.
[0]: https://github.com/linuxppc/issues/issues/224
[1]: https://lore.kernel.org/kernel-hardening/20190426232303.28381-1-nadav.amit@gmail.com/
Christopher M. Riedl (3):
powerpc/mm: Introduce temporary mm
powerpc/lib: Initialize a temporary mm for code patching
powerpc/lib: Use a temporary mm for code patching
arch/powerpc/include/asm/debug.h | 1 +
arch/powerpc/include/asm/mmu_context.h | 56 +++++++++-
arch/powerpc/kernel/process.c | 5 +
arch/powerpc/lib/code-patching.c | 140 ++++++++++++++-----------
4 files changed, 137 insertions(+), 65 deletions(-)
--
2.25.1
From: Christopher M. Riedl <hidden> Date: 2020-03-23 05:01:18
When code patching a STRICT_KERNEL_RWX kernel the page containing the
address to be patched is temporarily mapped with permissive memory
protections. Currently, a per-cpu vmalloc patch area is used for this
purpose. While the patch area is per-cpu, the temporary page mapping is
inserted into the kernel page tables for the duration of the patching.
The mapping is exposed to CPUs other than the patching CPU - this is
undesirable from a hardening perspective.
Use the `poking_init` init hook to prepare a temporary mm and patching
address. Initialize the temporary mm by copying the init mm. Choose a
randomized patching address inside the temporary mm userspace address
portion. The next patch uses the temporary mm and patching address for
code patching.
Based on x86 implementation:
commit 4fc19708b165
("x86/alternatives: Initialize temporary mm for patching")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
@@ -39,6 +41,30 @@ int raw_patch_instruction(unsigned int *addr, unsigned int instr)}#ifdef CONFIG_STRICT_KERNEL_RWX++__ro_after_initstructmm_struct*patching_mm;+__ro_after_initunsignedlongpatching_addr;++void__initpoking_init(void)+{+spinlock_t*ptl;/* for protecting pte table */+pte_t*ptep;++patching_mm=copy_init_mm();+BUG_ON(!patching_mm);++/*+*InhashwecannotgoaboveDEFAULT_MAP_WINDOWeasily.+*XXX:Dowewantadditionalbitsofentropyforradix?+*/+patching_addr=(get_random_long()&PAGE_MASK)%+(DEFAULT_MAP_WINDOW-PAGE_SIZE);++ptep=get_locked_pte(patching_mm,patching_addr,&ptl);+BUG_ON(!ptep);+pte_unmap_unlock(ptep,ptl);+}+staticDEFINE_PER_CPU(structvm_struct*,text_poke_area);staticinttext_area_cpu_up(unsignedintcpu)
From: Christopher M. Riedl <hidden> Date: 2020-03-23 05:05:49
Currently, code patching a STRICT_KERNEL_RWX exposes the temporary
mappings to other CPUs. These mappings should be kept local to the CPU
doing the patching. Use the pre-initialized temporary mm and patching
address for this purpose. Also add a check after patching to ensure the
patch succeeded.
Based on x86 implementation:
commit b3fd8e83ada0
("x86/alternatives: Use temporary mm for text poking")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 128 ++++++++++++++-----------------
1 file changed, 57 insertions(+), 71 deletions(-)
@@ -65,99 +66,79 @@ void __init poking_init(void)pte_unmap_unlock(ptep,ptl);}-staticDEFINE_PER_CPU(structvm_struct*,text_poke_area);--staticinttext_area_cpu_up(unsignedintcpu)-{-structvm_struct*area;--area=get_vm_area(PAGE_SIZE,VM_ALLOC);-if(!area){-WARN_ONCE(1,"Failed to create text area for cpu %d\n",-cpu);-return-1;-}-this_cpu_write(text_poke_area,area);--return0;-}--staticinttext_area_cpu_down(unsignedintcpu)-{-free_vm_area(this_cpu_read(text_poke_area));-return0;-}--/*-*Runasalateinitcall.Thisallowsalltheboottimepatchingtobedone-*simplybypatchingthecode,andthenwe'recalledherepriorto-*mark_rodata_ro(),whichhappensafterallinitcallsarerun.Although-*BUG_ON()isrude,inthiscaseitshouldonlyhappenifENOMEM,andwejudge-*itasbeingpreferabletoakernelthatwillcrashlaterwhensomeonetries-*tousepatch_instruction().-*/-staticint__initsetup_text_poke_area(void)-{-BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,-"powerpc/text_poke:online",text_area_cpu_up,-text_area_cpu_down));--return0;-}-late_initcall(setup_text_poke_area);+structpatch_mapping{+spinlock_t*ptl;/* for protecting pte table */+structtemp_mmtemp_mm;+};/**Thiscanbecalledforkerneltextoramodule.*/-staticintmap_patch_area(void*addr,unsignedlongtext_poke_addr)+staticintmap_patch(constvoid*addr,structpatch_mapping*patch_mapping){-unsignedlongpfn;-interr;+structpage*page;+pte_tpte,*ptep;+pgprot_tpgprot;if(is_vmalloc_addr(addr))-pfn=vmalloc_to_pfn(addr);+page=vmalloc_to_page(addr);else-pfn=__pa_symbol(addr)>>PAGE_SHIFT;+page=virt_to_page(addr);-err=map_kernel_page(text_poke_addr,(pfn<<PAGE_SHIFT),PAGE_KERNEL);+if(radix_enabled())+pgprot=__pgprot(pgprot_val(PAGE_KERNEL));+else+pgprot=PAGE_SHARED;-pr_devel("Mapped addr %lx with pfn %lx:%d\n",text_poke_addr,pfn,err);-if(err)+ptep=get_locked_pte(patching_mm,patching_addr,&patch_mapping->ptl);+if(unlikely(!ptep)){+pr_warn("map patch: failed to allocate pte for patching\n");return-1;+}++pte=mk_pte(page,pgprot);+set_pte_at(patching_mm,patching_addr,ptep,pte);++init_temp_mm(&patch_mapping->temp_mm,patching_mm);+use_temporary_mm(&patch_mapping->temp_mm);return0;}-staticinlineintunmap_patch_area(unsignedlongaddr)+staticintunmap_patch(structpatch_mapping*patch_mapping){pte_t*ptep;pmd_t*pmdp;pud_t*pudp;pgd_t*pgdp;-pgdp=pgd_offset_k(addr);+pgdp=pgd_offset(patching_mm,patching_addr);if(unlikely(!pgdp))return-EINVAL;-pudp=pud_offset(pgdp,addr);+pudp=pud_offset(pgdp,patching_addr);if(unlikely(!pudp))return-EINVAL;-pmdp=pmd_offset(pudp,addr);+pmdp=pmd_offset(pudp,patching_addr);if(unlikely(!pmdp))return-EINVAL;-ptep=pte_offset_kernel(pmdp,addr);+ptep=pte_offset_kernel(pmdp,patching_addr);if(unlikely(!ptep))return-EINVAL;-pr_devel("clearing mm %p, pte %p, addr %lx\n",&init_mm,ptep,addr);+/*+*Inhash,pte_clearflushesthetlb+*/+pte_clear(patching_mm,patching_addr,ptep);+unuse_temporary_mm(&patch_mapping->temp_mm);/*-*Inhash,pte_clearflushesthetlb,inradix,wehaveto+*Inradix,wehavetoexplicitlyflushthetlb(no-opinhash)*/-pte_clear(&init_mm,addr,ptep);-flush_tlb_kernel_range(addr,addr+PAGE_SIZE);+local_flush_tlb_mm(patching_mm);+pte_unmap_unlock(ptep,patch_mapping->ptl);return0;}
@@ -167,33 +148,38 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr)interr;unsignedint*patch_addr=NULL;unsignedlongflags;-unsignedlongtext_poke_addr;-unsignedlongkaddr=(unsignedlong)addr;+structpatch_mappingpatch_mapping;/*-*Duringearlyearlybootpatch_instructioniscalled-*whentext_poke_areaisnotready,butwestillneed-*toallowpatching.Wejustdotheplainoldpatching+*Thepatching_mmisinitializedbeforecallingmark_rodata_ro.Prior+*tothis,patch_instructioniscalledwhenwedon'thave(anddon't+*need)thepatching_mmsojustdoplainoldpatching.*/-if(!this_cpu_read(text_poke_area))+if(!patching_mm)returnraw_patch_instruction(addr,instr);local_irq_save(flags);-text_poke_addr=(unsignedlong)__this_cpu_read(text_poke_area)->addr;-if(map_patch_area(addr,text_poke_addr)){-err=-1;+err=map_patch(addr,&patch_mapping);+if(err)gotoout;-}-patch_addr=(unsignedint*)(text_poke_addr)+-((kaddr&~PAGE_MASK)/sizeof(unsignedint));+patch_addr=(unsignedint*)(patching_addr)++(offset_in_page((unsignedlong)addr)/+sizeof(unsignedint));__patch_instruction(addr,instr,patch_addr);-err=unmap_patch_area(text_poke_addr);+err=unmap_patch(&patch_mapping);if(err)-pr_warn("failed to unmap %lx\n",text_poke_addr);+pr_warn("unmap patch: failed to unmap patch\n");++/*+*Somethingiswrongifwhatwejustwrotedoesn'tmatchwhatwe+*thinkwejustwrote.+*XXX:BUG_ON()instead?+*/+WARN_ON(memcmp(addr,&instr,sizeof(instr)));out:local_irq_restore(flags);
From: Christopher M. Riedl <hidden> Date: 2020-03-23 05:08:22
x86 supports the notion of a temporary mm which restricts access to
temporary PTEs to a single CPU. A temporary mm is useful for situations
where a CPU needs to perform sensitive operations (such as patching a
STRICT_KERNEL_RWX kernel) requiring temporary mappings without exposing
said mappings to other CPUs. A side benefit is that other CPU TLBs do
not need to be flushed when the temporary mm is torn down.
Mappings in the temporary mm can be set in the userspace portion of the
address-space.
Interrupts must be disabled while the temporary mm is in use. HW
breakpoints, which may have been set by userspace as watchpoints on
addresses now within the temporary mm, are saved and disabled when
loading the temporary mm. The HW breakpoints are restored when unloading
the temporary mm. All HW breakpoints are indiscriminately disabled while
the temporary mm is in use.
Based on x86 implementation:
commit cefa929c034e
("x86/mm: Introduce temporary mm structs")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/include/asm/debug.h | 1 +
arch/powerpc/include/asm/mmu_context.h | 56 +++++++++++++++++++++++++-
arch/powerpc/kernel/process.c | 5 +++
3 files changed, 61 insertions(+), 1 deletion(-)
On 03/23/2020 04:52 AM, Christopher M. Riedl wrote:
When compiled with CONFIG_STRICT_KERNEL_RWX, the kernel must create
temporary mappings when patching itself. These mappings temporarily
override the strict RWX text protections to permit a write. Currently,
powerpc allocates a per-CPU VM area for patching. Patching occurs as
follows:
1. Map page of text to be patched to per-CPU VM area w/
PAGE_KERNEL protection
2. Patch text
3. Remove the temporary mapping
While the VM area is per-CPU, the mapping is actually inserted into the
kernel page tables. Presumably, this could allow another CPU to access
the normally write-protected text - either malicously or accidentally -
via this same mapping if the address of the VM area is known. Ideally,
the mapping should be kept local to the CPU doing the patching (or any
other sensitive operations requiring temporarily overriding memory
protections) [0].
x86 introduced "temporary mm" structs which allow the creation of
mappings local to a particular CPU [1]. This series intends to bring the
notion of a temporary mm to powerpc and harden powerpc by using such a
mapping for patching a kernel with strict RWX permissions.
The first patch introduces the temporary mm struct and API for powerpc
along with a new function to retrieve a current hw breakpoint.
The second patch uses the `poking_init` init hook added by the x86
patches to initialize a temporary mm and patching address. The patching
address is randomized between 0 and DEFAULT_MAP_WINDOW-PAGE_SIZE. The
upper limit is necessary due to how the hash MMU operates - by default
the space above DEFAULT_MAP_WINDOW is not available. For now, both hash
and radix randomize inside this range. The number of possible random
addresses is dependent on PAGE_SIZE and limited by DEFAULT_MAP_WINDOW.
Bits of entropy with 64K page size on BOOK3S_64:
bits-o-entropy = log2(DEFAULT_MAP_WINDOW_USER64 / PAGE_SIZE)
PAGE_SIZE=64K, DEFAULT_MAP_WINDOW_USER64=128TB
bits-o-entropy = log2(128TB / 64K)
bits-o-entropy = 31
Currently, randomization occurs only once during initialization at boot.
The third patch replaces the VM area with the temporary mm in the
patching code. The page for patching has to be mapped PAGE_SHARED with
the hash MMU since hash prevents the kernel from accessing userspace
pages with PAGE_PRIVILEGED bit set. There is on-going work on my side to
explore if this is actually necessary in the hash codepath.
Testing so far is limited to booting on QEMU (power8 and power9 targets)
and a POWER8 VM along with setting some simple xmon breakpoints (which
makes use of code-patching). A POC lkdtm test is in-progress to actually
exploit the existing vulnerability (ie. the mapping during patching is
exposed in kernel page tables and accessible by other CPUS) - this will
accompany a future v1 of this series.
On 03/23/2020 04:52 AM, Christopher M. Riedl wrote:
quoted
When compiled with CONFIG_STRICT_KERNEL_RWX, the kernel must create
temporary mappings when patching itself. These mappings temporarily
override the strict RWX text protections to permit a write. Currently,
powerpc allocates a per-CPU VM area for patching. Patching occurs as
follows:
1. Map page of text to be patched to per-CPU VM area w/
PAGE_KERNEL protection
2. Patch text
3. Remove the temporary mapping
While the VM area is per-CPU, the mapping is actually inserted into the
kernel page tables. Presumably, this could allow another CPU to access
the normally write-protected text - either malicously or accidentally -
via this same mapping if the address of the VM area is known. Ideally,
the mapping should be kept local to the CPU doing the patching (or any
other sensitive operations requiring temporarily overriding memory
protections) [0].
x86 introduced "temporary mm" structs which allow the creation of
mappings local to a particular CPU [1]. This series intends to bring the
notion of a temporary mm to powerpc and harden powerpc by using such a
mapping for patching a kernel with strict RWX permissions.
The first patch introduces the temporary mm struct and API for powerpc
along with a new function to retrieve a current hw breakpoint.
The second patch uses the `poking_init` init hook added by the x86
patches to initialize a temporary mm and patching address. The patching
address is randomized between 0 and DEFAULT_MAP_WINDOW-PAGE_SIZE. The
upper limit is necessary due to how the hash MMU operates - by default
the space above DEFAULT_MAP_WINDOW is not available. For now, both hash
and radix randomize inside this range. The number of possible random
addresses is dependent on PAGE_SIZE and limited by DEFAULT_MAP_WINDOW.
Bits of entropy with 64K page size on BOOK3S_64:
bits-o-entropy = log2(DEFAULT_MAP_WINDOW_USER64 / PAGE_SIZE)
PAGE_SIZE=64K, DEFAULT_MAP_WINDOW_USER64=128TB
bits-o-entropy = log2(128TB / 64K)
bits-o-entropy = 31
Currently, randomization occurs only once during initialization at boot.
The third patch replaces the VM area with the temporary mm in the
patching code. The page for patching has to be mapped PAGE_SHARED with
the hash MMU since hash prevents the kernel from accessing userspace
pages with PAGE_PRIVILEGED bit set. There is on-going work on my side to
explore if this is actually necessary in the hash codepath.
Testing so far is limited to booting on QEMU (power8 and power9 targets)
and a POWER8 VM along with setting some simple xmon breakpoints (which
makes use of code-patching). A POC lkdtm test is in-progress to actually
exploit the existing vulnerability (ie. the mapping during patching is
exposed in kernel page tables and accessible by other CPUS) - this will
accompany a future v1 of this series.
Got following failures on an 8xx. Note that "fault blocked by AP
register !" means an unauthorised access from Kernel to Userspace.
On 03/23/2020 04:52 AM, Christopher M. Riedl wrote:
quoted
When compiled with CONFIG_STRICT_KERNEL_RWX, the kernel must create
temporary mappings when patching itself. These mappings temporarily
override the strict RWX text protections to permit a write. Currently,
powerpc allocates a per-CPU VM area for patching. Patching occurs as
follows:
1. Map page of text to be patched to per-CPU VM area w/
PAGE_KERNEL protection
2. Patch text
3. Remove the temporary mapping
While the VM area is per-CPU, the mapping is actually inserted into the
kernel page tables. Presumably, this could allow another CPU to access
the normally write-protected text - either malicously or accidentally -
via this same mapping if the address of the VM area is known. Ideally,
the mapping should be kept local to the CPU doing the patching (or any
other sensitive operations requiring temporarily overriding memory
protections) [0].
x86 introduced "temporary mm" structs which allow the creation of
mappings local to a particular CPU [1]. This series intends to bring the
notion of a temporary mm to powerpc and harden powerpc by using such a
mapping for patching a kernel with strict RWX permissions.
The first patch introduces the temporary mm struct and API for powerpc
along with a new function to retrieve a current hw breakpoint.
The second patch uses the `poking_init` init hook added by the x86
patches to initialize a temporary mm and patching address. The patching
address is randomized between 0 and DEFAULT_MAP_WINDOW-PAGE_SIZE. The
upper limit is necessary due to how the hash MMU operates - by default
the space above DEFAULT_MAP_WINDOW is not available. For now, both hash
and radix randomize inside this range. The number of possible random
addresses is dependent on PAGE_SIZE and limited by DEFAULT_MAP_WINDOW.
Bits of entropy with 64K page size on BOOK3S_64:
bits-o-entropy = log2(DEFAULT_MAP_WINDOW_USER64 / PAGE_SIZE)
PAGE_SIZE=64K, DEFAULT_MAP_WINDOW_USER64=128TB
bits-o-entropy = log2(128TB / 64K)
bits-o-entropy = 31
Currently, randomization occurs only once during initialization at boot.
The third patch replaces the VM area with the temporary mm in the
patching code. The page for patching has to be mapped PAGE_SHARED with
the hash MMU since hash prevents the kernel from accessing userspace
pages with PAGE_PRIVILEGED bit set. There is on-going work on my side to
explore if this is actually necessary in the hash codepath.
Testing so far is limited to booting on QEMU (power8 and power9 targets)
and a POWER8 VM along with setting some simple xmon breakpoints (which
makes use of code-patching). A POC lkdtm test is in-progress to actually
exploit the existing vulnerability (ie. the mapping during patching is
exposed in kernel page tables and accessible by other CPUS) - this will
accompany a future v1 of this series.
Got following failures on an 8xx. Note that "fault blocked by AP
register !" means an unauthorised access from Kernel to Userspace.
Still a problem even without CONFIG_PPC_KUAP:
I've been able to dig into the problem.
With CONFIG_PPC_KUAP, it can definitely not work. See why in commit
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=ef296729b735e083d8919e76ac213b8ff237eb78
Without CONFIG_PPC_KUAP, on the 8xx, __put_user_asm() in
__patch_instruction() returns -EFAULT. That's because _PAGE_DIRTY is not
set on the page. Normally it should be a minor fault and the fault
handler should set the _PAGE_DIRTY flag. It must be something in the way
the page is allocated and mapped which prevents that. If I forge
_PAGE_DIRTY in addition to PAGE_SHARED, it works. But I don't think it
is valid approach to solve the issue.
Christophe
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted hunk
x86 supports the notion of a temporary mm which restricts access to
temporary PTEs to a single CPU. A temporary mm is useful for situations
where a CPU needs to perform sensitive operations (such as patching a
STRICT_KERNEL_RWX kernel) requiring temporary mappings without exposing
said mappings to other CPUs. A side benefit is that other CPU TLBs do
not need to be flushed when the temporary mm is torn down.
Mappings in the temporary mm can be set in the userspace portion of the
address-space.
Interrupts must be disabled while the temporary mm is in use. HW
breakpoints, which may have been set by userspace as watchpoints on
addresses now within the temporary mm, are saved and disabled when
loading the temporary mm. The HW breakpoints are restored when unloading
the temporary mm. All HW breakpoints are indiscriminately disabled while
the temporary mm is in use.
Based on x86 implementation:
commit cefa929c034e
("x86/mm: Introduce temporary mm structs")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/include/asm/debug.h | 1 +
arch/powerpc/include/asm/mmu_context.h | 56 +++++++++++++++++++++++++-
arch/powerpc/kernel/process.c | 5 +++
3 files changed, 61 insertions(+), 1 deletion(-)
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted hunk
When code patching a STRICT_KERNEL_RWX kernel the page containing the
address to be patched is temporarily mapped with permissive memory
protections. Currently, a per-cpu vmalloc patch area is used for this
purpose. While the patch area is per-cpu, the temporary page mapping is
inserted into the kernel page tables for the duration of the patching.
The mapping is exposed to CPUs other than the patching CPU - this is
undesirable from a hardening perspective.
Use the `poking_init` init hook to prepare a temporary mm and patching
address. Initialize the temporary mm by copying the init mm. Choose a
randomized patching address inside the temporary mm userspace address
portion. The next patch uses the temporary mm and patching address for
code patching.
Based on x86 implementation:
commit 4fc19708b165
("x86/alternatives: Initialize temporary mm for patching")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
@@ -39,6 +41,30 @@ int raw_patch_instruction(unsigned int *addr, unsigned int instr)}#ifdef CONFIG_STRICT_KERNEL_RWX++__ro_after_initstructmm_struct*patching_mm;+__ro_after_initunsignedlongpatching_addr;
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted hunk
Currently, code patching a STRICT_KERNEL_RWX exposes the temporary
mappings to other CPUs. These mappings should be kept local to the CPU
doing the patching. Use the pre-initialized temporary mm and patching
address for this purpose. Also add a check after patching to ensure the
patch succeeded.
Based on x86 implementation:
commit b3fd8e83ada0
("x86/alternatives: Use temporary mm for text poking")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 128 ++++++++++++++-----------------
1 file changed, 57 insertions(+), 71 deletions(-)
@@ -65,99 +66,79 @@ void __init poking_init(void)pte_unmap_unlock(ptep,ptl);}-staticDEFINE_PER_CPU(structvm_struct*,text_poke_area);--staticinttext_area_cpu_up(unsignedintcpu)-{-structvm_struct*area;--area=get_vm_area(PAGE_SIZE,VM_ALLOC);-if(!area){-WARN_ONCE(1,"Failed to create text area for cpu %d\n",-cpu);-return-1;-}-this_cpu_write(text_poke_area,area);--return0;-}--staticinttext_area_cpu_down(unsignedintcpu)-{-free_vm_area(this_cpu_read(text_poke_area));-return0;-}--/*-*Runasalateinitcall.Thisallowsalltheboottimepatchingtobedone-*simplybypatchingthecode,andthenwe'recalledherepriorto-*mark_rodata_ro(),whichhappensafterallinitcallsarerun.Although-*BUG_ON()isrude,inthiscaseitshouldonlyhappenifENOMEM,andwejudge-*itasbeingpreferabletoakernelthatwillcrashlaterwhensomeonetries-*tousepatch_instruction().-*/-staticint__initsetup_text_poke_area(void)-{-BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,-"powerpc/text_poke:online",text_area_cpu_up,-text_area_cpu_down));--return0;-}-late_initcall(setup_text_poke_area);+structpatch_mapping{+spinlock_t*ptl;/* for protecting pte table */+structtemp_mmtemp_mm;+};/**Thiscanbecalledforkerneltextoramodule.*/-staticintmap_patch_area(void*addr,unsignedlongtext_poke_addr)+staticintmap_patch(constvoid*addr,structpatch_mapping*patch_mapping)
Can you explain the difference between radix and non radix ?
Why PAGE_KERNEL for a page that is mapped in userspace ?
Why do you need to do __pgprot(pgprot_val(PAGE_KERNEL)) instead of just
using PAGE_KERNEL ?
ptep should be stored in the patch_mapping struct instead of walking
again the page tables.
quoted hunk
if (unlikely(!ptep))
return -EINVAL;
- pr_devel("clearing mm %p, pte %p, addr %lx\n", &init_mm, ptep, addr);
+ /*
+ * In hash, pte_clear flushes the tlb
+ */
+ pte_clear(patching_mm, patching_addr, ptep);
+ unuse_temporary_mm(&patch_mapping->temp_mm);
/*
- * In hash, pte_clear flushes the tlb, in radix, we have to
+ * In radix, we have to explicitly flush the tlb (no-op in hash)
*/
- pte_clear(&init_mm, addr, ptep);
- flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+ local_flush_tlb_mm(patching_mm);
+ pte_unmap_unlock(ptep, patch_mapping->ptl);
return 0;
}
@@ -167,33 +148,38 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr) int err; unsigned int *patch_addr = NULL; unsigned long flags;- unsigned long text_poke_addr;- unsigned long kaddr = (unsigned long)addr;+ struct patch_mapping patch_mapping; /*- * During early early boot patch_instruction is called- * when text_poke_area is not ready, but we still need- * to allow patching. We just do the plain old patching+ * The patching_mm is initialized before calling mark_rodata_ro. Prior+ * to this, patch_instruction is called when we don't have (and don't+ * need) the patching_mm so just do plain old patching. */- if (!this_cpu_read(text_poke_area))+ if (!patching_mm) return raw_patch_instruction(addr, instr); local_irq_save(flags);- text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;- if (map_patch_area(addr, text_poke_addr)) {- err = -1;+ err = map_patch(addr, &patch_mapping);+ if (err) goto out;- }- patch_addr = (unsigned int *)(text_poke_addr) +- ((kaddr & ~PAGE_MASK) / sizeof(unsigned int));+ patch_addr = (unsigned int *)(patching_addr) ++ (offset_in_page((unsigned long)addr) /+ sizeof(unsigned int)); __patch_instruction(addr, instr, patch_addr);
The error returned by __patch_instruction() should be managed.
- err = unmap_patch_area(text_poke_addr);
+ err = unmap_patch(&patch_mapping);
if (err)
- pr_warn("failed to unmap %lx\n", text_poke_addr);
+ pr_warn("unmap patch: failed to unmap patch\n");
+
+ /*
+ * Something is wrong if what we just wrote doesn't match what we
+ * think we just wrote.
+ * XXX: BUG_ON() instead?
No, not a BUG_ON(). If patching fails, that's no a vital fault, we can
fail gracefully. You should return a fault instead.
Come on. addr is an *int, instr is an int. By doing a memcmp() on
&instr, you for the compiler to write instr into the stack whereas local
vars are mainly in registers on RISC processors like powerpc. Following
should do it:
WARN_ON(*addr != instr);
From: Andrew Donnellan <hidden> Date: 2020-03-25 02:54:01
On 23/3/20 3:52 pm, Christopher M. Riedl wrote:
When compiled with CONFIG_STRICT_KERNEL_RWX, the kernel must create
temporary mappings when patching itself. These mappings temporarily
override the strict RWX text protections to permit a write. Currently,
powerpc allocates a per-CPU VM area for patching. Patching occurs as
follows:
1. Map page of text to be patched to per-CPU VM area w/
PAGE_KERNEL protection
2. Patch text
3. Remove the temporary mapping
While the VM area is per-CPU, the mapping is actually inserted into the
kernel page tables. Presumably, this could allow another CPU to access
the normally write-protected text - either malicously or accidentally -
via this same mapping if the address of the VM area is known. Ideally,
the mapping should be kept local to the CPU doing the patching (or any
other sensitive operations requiring temporarily overriding memory
protections) [0].
x86 introduced "temporary mm" structs which allow the creation of
mappings local to a particular CPU [1]. This series intends to bring the
notion of a temporary mm to powerpc and harden powerpc by using such a
mapping for patching a kernel with strict RWX permissions.
The first patch introduces the temporary mm struct and API for powerpc
along with a new function to retrieve a current hw breakpoint.
The second patch uses the `poking_init` init hook added by the x86
patches to initialize a temporary mm and patching address. The patching
address is randomized between 0 and DEFAULT_MAP_WINDOW-PAGE_SIZE. The
upper limit is necessary due to how the hash MMU operates - by default
the space above DEFAULT_MAP_WINDOW is not available. For now, both hash
and radix randomize inside this range. The number of possible random
addresses is dependent on PAGE_SIZE and limited by DEFAULT_MAP_WINDOW.
Bits of entropy with 64K page size on BOOK3S_64:
bits-o-entropy = log2(DEFAULT_MAP_WINDOW_USER64 / PAGE_SIZE)
PAGE_SIZE=64K, DEFAULT_MAP_WINDOW_USER64=128TB
bits-o-entropy = log2(128TB / 64K)
bits-o-entropy = 31
Currently, randomization occurs only once during initialization at boot.
The third patch replaces the VM area with the temporary mm in the
patching code. The page for patching has to be mapped PAGE_SHARED with
the hash MMU since hash prevents the kernel from accessing userspace
pages with PAGE_PRIVILEGED bit set. There is on-going work on my side to
explore if this is actually necessary in the hash codepath.
Testing so far is limited to booting on QEMU (power8 and power9 targets)
and a POWER8 VM along with setting some simple xmon breakpoints (which
makes use of code-patching). A POC lkdtm test is in-progress to actually
exploit the existing vulnerability (ie. the mapping during patching is
exposed in kernel page tables and accessible by other CPUS) - this will
accompany a future v1 of this series.
[0]: https://github.com/linuxppc/issues/issues/224
[1]: https://lore.kernel.org/kernel-hardening/20190426232303.28381-1-nadav.amit@gmail.com/
Christopher M. Riedl (3):
powerpc/mm: Introduce temporary mm
powerpc/lib: Initialize a temporary mm for code patching
powerpc/lib: Use a temporary mm for code patching
arch/powerpc/include/asm/debug.h | 1 +
arch/powerpc/include/asm/mmu_context.h | 56 +++++++++-
arch/powerpc/kernel/process.c | 5 +
arch/powerpc/lib/code-patching.c | 140 ++++++++++++++-----------
4 files changed, 137 insertions(+), 65 deletions(-)
From: Christopher M Riedl <hidden> Date: 2020-03-31 02:43:33
On March 24, 2020 11:07 AM Christophe Leroy [off-list ref] wrote:
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted
x86 supports the notion of a temporary mm which restricts access to
temporary PTEs to a single CPU. A temporary mm is useful for situations
where a CPU needs to perform sensitive operations (such as patching a
STRICT_KERNEL_RWX kernel) requiring temporary mappings without exposing
said mappings to other CPUs. A side benefit is that other CPU TLBs do
not need to be flushed when the temporary mm is torn down.
Mappings in the temporary mm can be set in the userspace portion of the
address-space.
Interrupts must be disabled while the temporary mm is in use. HW
breakpoints, which may have been set by userspace as watchpoints on
addresses now within the temporary mm, are saved and disabled when
loading the temporary mm. The HW breakpoints are restored when unloading
the temporary mm. All HW breakpoints are indiscriminately disabled while
the temporary mm is in use.
Based on x86 implementation:
commit cefa929c034e
("x86/mm: Introduce temporary mm structs")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/include/asm/debug.h | 1 +
arch/powerpc/include/asm/mmu_context.h | 56 +++++++++++++++++++++++++-
arch/powerpc/kernel/process.c | 5 +++
3 files changed, 61 insertions(+), 1 deletion(-)
From: Christopher M Riedl <hidden> Date: 2020-03-31 03:21:45
On March 24, 2020 11:10 AM Christophe Leroy [off-list ref] wrote:
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted
When code patching a STRICT_KERNEL_RWX kernel the page containing the
address to be patched is temporarily mapped with permissive memory
protections. Currently, a per-cpu vmalloc patch area is used for this
purpose. While the patch area is per-cpu, the temporary page mapping is
inserted into the kernel page tables for the duration of the patching.
The mapping is exposed to CPUs other than the patching CPU - this is
undesirable from a hardening perspective.
Use the `poking_init` init hook to prepare a temporary mm and patching
address. Initialize the temporary mm by copying the init mm. Choose a
randomized patching address inside the temporary mm userspace address
portion. The next patch uses the temporary mm and patching address for
code patching.
Based on x86 implementation:
commit 4fc19708b165
("x86/alternatives: Initialize temporary mm for patching")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
@@ -39,6 +41,30 @@ int raw_patch_instruction(unsigned int *addr, unsigned int instr)}#ifdef CONFIG_STRICT_KERNEL_RWX++__ro_after_initstructmm_struct*patching_mm;+__ro_after_initunsignedlongpatching_addr;
Does it needs to be a BUG_ON() ? Can't we fail gracefully with just a
WARN_ON ?
I'm not sure what failing gracefully means here? The main reason this could
fail is if there is not enough memory to allocate the patching_mm. The
previous implementation had this justification for BUG_ON():
/*
* Run as a late init call. This allows all the boot time patching to be done
* simply by patching the code, and then we're called here prior to
* mark_rodata_ro(), which happens after all init calls are run. Although
* BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge
* it as being preferable to a kernel that will crash later when someone tries
* to use patch_instruction().
*/
static int __init setup_text_poke_area(void)
{
BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
"powerpc/text_poke:online", text_area_cpu_up,
text_area_cpu_down));
return 0;
}
late_initcall(setup_text_poke_area);
I think the BUG_ON() is appropriate even if only to adhere to the previous
judgement call. I can add a similar comment explaining the reasoning if
that helps.
quoted
+
+ /*
+ * In hash we cannot go above DEFAULT_MAP_WINDOW easily.
+ * XXX: Do we want additional bits of entropy for radix?
+ */
+ patching_addr = (get_random_long() & PAGE_MASK) %
+ (DEFAULT_MAP_WINDOW - PAGE_SIZE);
+
+ ptep = get_locked_pte(patching_mm, patching_addr, &ptl);
+ BUG_ON(!ptep);
Same here, can we fail gracefully instead ?
Same reasoning as above.
quoted
+ pte_unmap_unlock(ptep, ptl);
+}
+
static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
static int text_area_cpu_up(unsigned int cpu)
Le 31/03/2020 à 05:19, Christopher M Riedl a écrit :
quoted
On March 24, 2020 11:10 AM Christophe Leroy [off-list ref] wrote:
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted
When code patching a STRICT_KERNEL_RWX kernel the page containing the
address to be patched is temporarily mapped with permissive memory
protections. Currently, a per-cpu vmalloc patch area is used for this
purpose. While the patch area is per-cpu, the temporary page mapping is
inserted into the kernel page tables for the duration of the patching.
The mapping is exposed to CPUs other than the patching CPU - this is
undesirable from a hardening perspective.
Use the `poking_init` init hook to prepare a temporary mm and patching
address. Initialize the temporary mm by copying the init mm. Choose a
randomized patching address inside the temporary mm userspace address
portion. The next patch uses the temporary mm and patching address for
code patching.
Based on x86 implementation:
commit 4fc19708b165
("x86/alternatives: Initialize temporary mm for patching")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
@@ -39,6 +41,30 @@ int raw_patch_instruction(unsigned int *addr, unsigned int instr)}#ifdef CONFIG_STRICT_KERNEL_RWX++__ro_after_initstructmm_struct*patching_mm;+__ro_after_initunsignedlongpatching_addr;
Does it needs to be a BUG_ON() ? Can't we fail gracefully with just a
WARN_ON ?
I'm not sure what failing gracefully means here? The main reason this could
fail is if there is not enough memory to allocate the patching_mm. The
previous implementation had this justification for BUG_ON():
But the system can continue running just fine after this failure.
Only the things that make use of code patching will fail (ftrace, kgdb, ...)
Checkpatch tells: "Avoid crashing the kernel - try using WARN_ON &
recovery code rather than BUG() or BUG_ON()"
All vital code patching has already been done previously, so I think a
WARN_ON() should be enough, plus returning non 0 to indicate that the
late_initcall failed.
/*
* Run as a late init call. This allows all the boot time patching to be done
* simply by patching the code, and then we're called here prior to
* mark_rodata_ro(), which happens after all init calls are run. Although
* BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge
* it as being preferable to a kernel that will crash later when someone tries
* to use patch_instruction().
*/
static int __init setup_text_poke_area(void)
{
BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
"powerpc/text_poke:online", text_area_cpu_up,
text_area_cpu_down));
return 0;
}
late_initcall(setup_text_poke_area);
I think the BUG_ON() is appropriate even if only to adhere to the previous
judgement call. I can add a similar comment explaining the reasoning if
that helps.
quoted
quoted
+
+ /*
+ * In hash we cannot go above DEFAULT_MAP_WINDOW easily.
+ * XXX: Do we want additional bits of entropy for radix?
+ */
+ patching_addr = (get_random_long() & PAGE_MASK) %
+ (DEFAULT_MAP_WINDOW - PAGE_SIZE);
+
+ ptep = get_locked_pte(patching_mm, patching_addr, &ptl);
+ BUG_ON(!ptep);
Same here, can we fail gracefully instead ?
Same reasoning as above.
Here as well, a WARN_ON() should be enough, the system will continue
running after that.
quoted
quoted
+ pte_unmap_unlock(ptep, ptl);
+}
+
static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
static int text_area_cpu_up(unsigned int cpu)
From: Christopher M Riedl <hidden> Date: 2020-04-15 04:41:47
On April 8, 2020 6:01 AM Christophe Leroy [off-list ref] wrote:
Le 31/03/2020 à 05:19, Christopher M Riedl a écrit :
quoted
quoted
On March 24, 2020 11:10 AM Christophe Leroy [off-list ref] wrote:
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted
When code patching a STRICT_KERNEL_RWX kernel the page containing the
address to be patched is temporarily mapped with permissive memory
protections. Currently, a per-cpu vmalloc patch area is used for this
purpose. While the patch area is per-cpu, the temporary page mapping is
inserted into the kernel page tables for the duration of the patching.
The mapping is exposed to CPUs other than the patching CPU - this is
undesirable from a hardening perspective.
Use the `poking_init` init hook to prepare a temporary mm and patching
address. Initialize the temporary mm by copying the init mm. Choose a
randomized patching address inside the temporary mm userspace address
portion. The next patch uses the temporary mm and patching address for
code patching.
Based on x86 implementation:
commit 4fc19708b165
("x86/alternatives: Initialize temporary mm for patching")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
@@ -39,6 +41,30 @@ int raw_patch_instruction(unsigned int *addr, unsigned int instr)}#ifdef CONFIG_STRICT_KERNEL_RWX++__ro_after_initstructmm_struct*patching_mm;+__ro_after_initunsignedlongpatching_addr;
Does it needs to be a BUG_ON() ? Can't we fail gracefully with just a
WARN_ON ?
I'm not sure what failing gracefully means here? The main reason this could
fail is if there is not enough memory to allocate the patching_mm. The
previous implementation had this justification for BUG_ON():
But the system can continue running just fine after this failure.
Only the things that make use of code patching will fail (ftrace, kgdb, ...)
Checkpatch tells: "Avoid crashing the kernel - try using WARN_ON &
recovery code rather than BUG() or BUG_ON()"
All vital code patching has already been done previously, so I think a
WARN_ON() should be enough, plus returning non 0 to indicate that the
late_initcall failed.
Got it, makes sense to me. I will make these changes in the next version.
Thanks!
quoted
/*
* Run as a late init call. This allows all the boot time patching to be done
* simply by patching the code, and then we're called here prior to
* mark_rodata_ro(), which happens after all init calls are run. Although
* BUG_ON() is rude, in this case it should only happen if ENOMEM, and we judge
* it as being preferable to a kernel that will crash later when someone tries
* to use patch_instruction().
*/
static int __init setup_text_poke_area(void)
{
BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
"powerpc/text_poke:online", text_area_cpu_up,
text_area_cpu_down));
return 0;
}
late_initcall(setup_text_poke_area);
I think the BUG_ON() is appropriate even if only to adhere to the previous
judgement call. I can add a similar comment explaining the reasoning if
that helps.
quoted
quoted
+
+ /*
+ * In hash we cannot go above DEFAULT_MAP_WINDOW easily.
+ * XXX: Do we want additional bits of entropy for radix?
+ */
+ patching_addr = (get_random_long() & PAGE_MASK) %
+ (DEFAULT_MAP_WINDOW - PAGE_SIZE);
+
+ ptep = get_locked_pte(patching_mm, patching_addr, &ptl);
+ BUG_ON(!ptep);
Same here, can we fail gracefully instead ?
Same reasoning as above.
Here as well, a WARN_ON() should be enough, the system will continue
running after that.
quoted
quoted
quoted
+ pte_unmap_unlock(ptep, ptl);
+}
+
static DEFINE_PER_CPU(struct vm_struct *, text_poke_area);
static int text_area_cpu_up(unsigned int cpu)
From: Christopher M Riedl <hidden> Date: 2020-04-15 05:12:54
On March 24, 2020 11:25 AM Christophe Leroy [off-list ref] wrote:
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted
Currently, code patching a STRICT_KERNEL_RWX exposes the temporary
mappings to other CPUs. These mappings should be kept local to the CPU
doing the patching. Use the pre-initialized temporary mm and patching
address for this purpose. Also add a check after patching to ensure the
patch succeeded.
Based on x86 implementation:
commit b3fd8e83ada0
("x86/alternatives: Use temporary mm for text poking")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 128 ++++++++++++++-----------------
1 file changed, 57 insertions(+), 71 deletions(-)
@@ -65,99 +66,79 @@ void __init poking_init(void)pte_unmap_unlock(ptep,ptl);}-staticDEFINE_PER_CPU(structvm_struct*,text_poke_area);--staticinttext_area_cpu_up(unsignedintcpu)-{-structvm_struct*area;--area=get_vm_area(PAGE_SIZE,VM_ALLOC);-if(!area){-WARN_ONCE(1,"Failed to create text area for cpu %d\n",-cpu);-return-1;-}-this_cpu_write(text_poke_area,area);--return0;-}--staticinttext_area_cpu_down(unsignedintcpu)-{-free_vm_area(this_cpu_read(text_poke_area));-return0;-}--/*-*Runasalateinitcall.Thisallowsalltheboottimepatchingtobedone-*simplybypatchingthecode,andthenwe'recalledherepriorto-*mark_rodata_ro(),whichhappensafterallinitcallsarerun.Although-*BUG_ON()isrude,inthiscaseitshouldonlyhappenifENOMEM,andwejudge-*itasbeingpreferabletoakernelthatwillcrashlaterwhensomeonetries-*tousepatch_instruction().-*/-staticint__initsetup_text_poke_area(void)-{-BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,-"powerpc/text_poke:online",text_area_cpu_up,-text_area_cpu_down));--return0;-}-late_initcall(setup_text_poke_area);+structpatch_mapping{+spinlock_t*ptl;/* for protecting pte table */+structtemp_mmtemp_mm;+};/**Thiscanbecalledforkerneltextoramodule.*/-staticintmap_patch_area(void*addr,unsignedlongtext_poke_addr)+staticintmap_patch(constvoid*addr,structpatch_mapping*patch_mapping)
Can you explain the difference between radix and non radix ?
Why PAGE_KERNEL for a page that is mapped in userspace ?
Why do you need to do __pgprot(pgprot_val(PAGE_KERNEL)) instead of just
using PAGE_KERNEL ?
On hash there is a manual check which prevents setting _PAGE_PRIVILEGED for
kernel to userspace access in __hash_page - hence we cannot access the mapping
if the page is mapped PAGE_KERNEL on hash. However, I would like to use
PAGE_KERNEL here as well and am working on understanding why this check is
done in hash and if this can change. On radix this works just fine.
The page is mapped PAGE_KERNEL because the address is technically a userspace
address - but only to keep the mapping local to this CPU doing the patching.
PAGE_KERNEL makes it clear both in intent and protection that this is a kernel
mapping.
I think the correct way is pgprot_val(PAGE_KERNEL) since PAGE_KERNEL is defined
as:
#define PAGE_KERNEL __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW)
and __pgprot() is defined as:
typedef struct { unsigned long pgprot; } pgprot_t;
#define pgprot_val(x) ((x).pgprot)
#define __pgprot(x) ((pgprot_t) { (x) })
ptep should be stored in the patch_mapping struct instead of walking
again the page tables.
Oh yes - this will be in the next version.
quoted
if (unlikely(!ptep))
return -EINVAL;
- pr_devel("clearing mm %p, pte %p, addr %lx\n", &init_mm, ptep, addr);
+ /*
+ * In hash, pte_clear flushes the tlb
+ */
+ pte_clear(patching_mm, patching_addr, ptep);
+ unuse_temporary_mm(&patch_mapping->temp_mm);
/*
- * In hash, pte_clear flushes the tlb, in radix, we have to
+ * In radix, we have to explicitly flush the tlb (no-op in hash)
*/
- pte_clear(&init_mm, addr, ptep);
- flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+ local_flush_tlb_mm(patching_mm);
+ pte_unmap_unlock(ptep, patch_mapping->ptl);
return 0;
}
@@ -167,33 +148,38 @@ static int do_patch_instruction(unsigned int *addr, unsigned int instr) int err; unsigned int *patch_addr = NULL; unsigned long flags;- unsigned long text_poke_addr;- unsigned long kaddr = (unsigned long)addr;+ struct patch_mapping patch_mapping; /*- * During early early boot patch_instruction is called- * when text_poke_area is not ready, but we still need- * to allow patching. We just do the plain old patching+ * The patching_mm is initialized before calling mark_rodata_ro. Prior+ * to this, patch_instruction is called when we don't have (and don't+ * need) the patching_mm so just do plain old patching. */- if (!this_cpu_read(text_poke_area))+ if (!patching_mm) return raw_patch_instruction(addr, instr); local_irq_save(flags);- text_poke_addr = (unsigned long)__this_cpu_read(text_poke_area)->addr;- if (map_patch_area(addr, text_poke_addr)) {- err = -1;+ err = map_patch(addr, &patch_mapping);+ if (err) goto out;- }- patch_addr = (unsigned int *)(text_poke_addr) +- ((kaddr & ~PAGE_MASK) / sizeof(unsigned int));+ patch_addr = (unsigned int *)(patching_addr) ++ (offset_in_page((unsigned long)addr) /+ sizeof(unsigned int)); __patch_instruction(addr, instr, patch_addr);
The error returned by __patch_instruction() should be managed.
Agreed, will do something in the next spin.
quoted
- err = unmap_patch_area(text_poke_addr);
+ err = unmap_patch(&patch_mapping);
if (err)
- pr_warn("failed to unmap %lx\n", text_poke_addr);
+ pr_warn("unmap patch: failed to unmap patch\n");
+
+ /*
+ * Something is wrong if what we just wrote doesn't match what we
+ * think we just wrote.
+ * XXX: BUG_ON() instead?
No, not a BUG_ON(). If patching fails, that's no a vital fault, we can
fail gracefully. You should return a fault instead.
Yup - will make these changes in the next version.
Come on. addr is an *int, instr is an int. By doing a memcmp() on
&instr, you for the compiler to write instr into the stack whereas local
vars are mainly in registers on RISC processors like powerpc. Following
should do it:
WARN_ON(*addr != instr);
Oh man - I agree, that's just embarrassing.
Appreciate your feedback on this RFC series, thanks!
Le 15/04/2020 à 07:11, Christopher M Riedl a écrit :
quoted
On March 24, 2020 11:25 AM Christophe Leroy [off-list ref] wrote:
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted
Currently, code patching a STRICT_KERNEL_RWX exposes the temporary
mappings to other CPUs. These mappings should be kept local to the CPU
doing the patching. Use the pre-initialized temporary mm and patching
address for this purpose. Also add a check after patching to ensure the
patch succeeded.
Based on x86 implementation:
commit b3fd8e83ada0
("x86/alternatives: Use temporary mm for text poking")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 128 ++++++++++++++-----------------
1 file changed, 57 insertions(+), 71 deletions(-)
@@ -65,99 +66,79 @@ void __init poking_init(void)pte_unmap_unlock(ptep,ptl);}-staticDEFINE_PER_CPU(structvm_struct*,text_poke_area);--staticinttext_area_cpu_up(unsignedintcpu)-{-structvm_struct*area;--area=get_vm_area(PAGE_SIZE,VM_ALLOC);-if(!area){-WARN_ONCE(1,"Failed to create text area for cpu %d\n",-cpu);-return-1;-}-this_cpu_write(text_poke_area,area);--return0;-}--staticinttext_area_cpu_down(unsignedintcpu)-{-free_vm_area(this_cpu_read(text_poke_area));-return0;-}--/*-*Runasalateinitcall.Thisallowsalltheboottimepatchingtobedone-*simplybypatchingthecode,andthenwe'recalledherepriorto-*mark_rodata_ro(),whichhappensafterallinitcallsarerun.Although-*BUG_ON()isrude,inthiscaseitshouldonlyhappenifENOMEM,andwejudge-*itasbeingpreferabletoakernelthatwillcrashlaterwhensomeonetries-*tousepatch_instruction().-*/-staticint__initsetup_text_poke_area(void)-{-BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,-"powerpc/text_poke:online",text_area_cpu_up,-text_area_cpu_down));--return0;-}-late_initcall(setup_text_poke_area);+structpatch_mapping{+spinlock_t*ptl;/* for protecting pte table */+structtemp_mmtemp_mm;+};/**Thiscanbecalledforkerneltextoramodule.*/-staticintmap_patch_area(void*addr,unsignedlongtext_poke_addr)+staticintmap_patch(constvoid*addr,structpatch_mapping*patch_mapping)
Can you explain the difference between radix and non radix ?
Why PAGE_KERNEL for a page that is mapped in userspace ?
Why do you need to do __pgprot(pgprot_val(PAGE_KERNEL)) instead of just
using PAGE_KERNEL ?
On hash there is a manual check which prevents setting _PAGE_PRIVILEGED for
kernel to userspace access in __hash_page - hence we cannot access the mapping
if the page is mapped PAGE_KERNEL on hash. However, I would like to use
PAGE_KERNEL here as well and am working on understanding why this check is
done in hash and if this can change. On radix this works just fine.
The page is mapped PAGE_KERNEL because the address is technically a userspace
address - but only to keep the mapping local to this CPU doing the patching.
PAGE_KERNEL makes it clear both in intent and protection that this is a kernel
mapping.
I think the correct way is pgprot_val(PAGE_KERNEL) since PAGE_KERNEL is defined
as:
#define PAGE_KERNEL __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW)
and __pgprot() is defined as:
typedef struct { unsigned long pgprot; } pgprot_t;
#define pgprot_val(x) ((x).pgprot)
#define __pgprot(x) ((pgprot_t) { (x) })
Yes, so:
pgprot_val(__pgprot(x)) == x
You do:
pgprot = __pgprot(pgprot_val(PAGE_KERNEL));
Which is:
pgprot = __pgprot(pgprot_val(__pgprot(_PAGE_BASE | _PAGE_KERNEL_RW)));
Which is equivalent to:
pgprot = __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW);
So at the end it should simply be:
pgprot = PAGE_KERNEL;
Christophe
From: Christopher M Riedl <hidden> Date: 2020-04-15 16:35:14
On April 15, 2020 3:45 AM Christophe Leroy [off-list ref] wrote:
Le 15/04/2020 à 07:11, Christopher M Riedl a écrit :
quoted
quoted
On March 24, 2020 11:25 AM Christophe Leroy [off-list ref] wrote:
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted
Currently, code patching a STRICT_KERNEL_RWX exposes the temporary
mappings to other CPUs. These mappings should be kept local to the CPU
doing the patching. Use the pre-initialized temporary mm and patching
address for this purpose. Also add a check after patching to ensure the
patch succeeded.
Based on x86 implementation:
commit b3fd8e83ada0
("x86/alternatives: Use temporary mm for text poking")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 128 ++++++++++++++-----------------
1 file changed, 57 insertions(+), 71 deletions(-)
@@ -65,99 +66,79 @@ void __init poking_init(void)pte_unmap_unlock(ptep,ptl);}-staticDEFINE_PER_CPU(structvm_struct*,text_poke_area);--staticinttext_area_cpu_up(unsignedintcpu)-{-structvm_struct*area;--area=get_vm_area(PAGE_SIZE,VM_ALLOC);-if(!area){-WARN_ONCE(1,"Failed to create text area for cpu %d\n",-cpu);-return-1;-}-this_cpu_write(text_poke_area,area);--return0;-}--staticinttext_area_cpu_down(unsignedintcpu)-{-free_vm_area(this_cpu_read(text_poke_area));-return0;-}--/*-*Runasalateinitcall.Thisallowsalltheboottimepatchingtobedone-*simplybypatchingthecode,andthenwe'recalledherepriorto-*mark_rodata_ro(),whichhappensafterallinitcallsarerun.Although-*BUG_ON()isrude,inthiscaseitshouldonlyhappenifENOMEM,andwejudge-*itasbeingpreferabletoakernelthatwillcrashlaterwhensomeonetries-*tousepatch_instruction().-*/-staticint__initsetup_text_poke_area(void)-{-BUG_ON(!cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,-"powerpc/text_poke:online",text_area_cpu_up,-text_area_cpu_down));--return0;-}-late_initcall(setup_text_poke_area);+structpatch_mapping{+spinlock_t*ptl;/* for protecting pte table */+structtemp_mmtemp_mm;+};/**Thiscanbecalledforkerneltextoramodule.*/-staticintmap_patch_area(void*addr,unsignedlongtext_poke_addr)+staticintmap_patch(constvoid*addr,structpatch_mapping*patch_mapping)
Can you explain the difference between radix and non radix ?
Why PAGE_KERNEL for a page that is mapped in userspace ?
Why do you need to do __pgprot(pgprot_val(PAGE_KERNEL)) instead of just
using PAGE_KERNEL ?
On hash there is a manual check which prevents setting _PAGE_PRIVILEGED for
kernel to userspace access in __hash_page - hence we cannot access the mapping
if the page is mapped PAGE_KERNEL on hash. However, I would like to use
PAGE_KERNEL here as well and am working on understanding why this check is
done in hash and if this can change. On radix this works just fine.
The page is mapped PAGE_KERNEL because the address is technically a userspace
address - but only to keep the mapping local to this CPU doing the patching.
PAGE_KERNEL makes it clear both in intent and protection that this is a kernel
mapping.
I think the correct way is pgprot_val(PAGE_KERNEL) since PAGE_KERNEL is defined
as:
#define PAGE_KERNEL __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW)
and __pgprot() is defined as:
typedef struct { unsigned long pgprot; } pgprot_t;
#define pgprot_val(x) ((x).pgprot)
#define __pgprot(x) ((pgprot_t) { (x) })
Yes, so:
pgprot_val(__pgprot(x)) == x
You do:
pgprot = __pgprot(pgprot_val(PAGE_KERNEL));
Which is:
pgprot = __pgprot(pgprot_val(__pgprot(_PAGE_BASE | _PAGE_KERNEL_RW)));
Which is equivalent to:
pgprot = __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW);
So at the end it should simply be:
pgprot = PAGE_KERNEL;
Yes you're correct. Picking this up in the next spin.
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2020-04-17 00:59:44
Christophe Leroy [off-list ref] writes:
Le 31/03/2020 à 05:19, Christopher M Riedl a écrit :
quoted
quoted
On March 24, 2020 11:10 AM Christophe Leroy [off-list ref] wrote:
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted
When code patching a STRICT_KERNEL_RWX kernel the page containing the
address to be patched is temporarily mapped with permissive memory
protections. Currently, a per-cpu vmalloc patch area is used for this
purpose. While the patch area is per-cpu, the temporary page mapping is
inserted into the kernel page tables for the duration of the patching.
The mapping is exposed to CPUs other than the patching CPU - this is
undesirable from a hardening perspective.
Use the `poking_init` init hook to prepare a temporary mm and patching
address. Initialize the temporary mm by copying the init mm. Choose a
randomized patching address inside the temporary mm userspace address
portion. The next patch uses the temporary mm and patching address for
code patching.
Based on x86 implementation:
commit 4fc19708b165
("x86/alternatives: Initialize temporary mm for patching")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/lib/code-patching.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
@@ -39,6 +41,30 @@ int raw_patch_instruction(unsigned int *addr, unsigned int instr)}#ifdef CONFIG_STRICT_KERNEL_RWX++__ro_after_initstructmm_struct*patching_mm;+__ro_after_initunsignedlongpatching_addr;
Does it needs to be a BUG_ON() ? Can't we fail gracefully with just a
WARN_ON ?
I'm not sure what failing gracefully means here? The main reason this could
fail is if there is not enough memory to allocate the patching_mm. The
previous implementation had this justification for BUG_ON():
But the system can continue running just fine after this failure.
Only the things that make use of code patching will fail (ftrace, kgdb, ...)
That's probably true of ftrace, but we can't fail patching for jump
labels (static keys).
See:
void arch_jump_label_transform(struct jump_entry *entry,
enum jump_label_type type)
{
u32 *addr = (u32 *)(unsigned long)entry->code;
if (type == JUMP_LABEL_JMP)
patch_branch(addr, entry->target, 0);
else
patch_instruction(addr, PPC_INST_NOP);
}
cheers
Le 23/03/2020 à 05:52, Christopher M. Riedl a écrit :
quoted hunk
x86 supports the notion of a temporary mm which restricts access to
temporary PTEs to a single CPU. A temporary mm is useful for situations
where a CPU needs to perform sensitive operations (such as patching a
STRICT_KERNEL_RWX kernel) requiring temporary mappings without exposing
said mappings to other CPUs. A side benefit is that other CPU TLBs do
not need to be flushed when the temporary mm is torn down.
Mappings in the temporary mm can be set in the userspace portion of the
address-space.
Interrupts must be disabled while the temporary mm is in use. HW
breakpoints, which may have been set by userspace as watchpoints on
addresses now within the temporary mm, are saved and disabled when
loading the temporary mm. The HW breakpoints are restored when unloading
the temporary mm. All HW breakpoints are indiscriminately disabled while
the temporary mm is in use.
Based on x86 implementation:
commit cefa929c034e
("x86/mm: Introduce temporary mm structs")
Signed-off-by: Christopher M. Riedl <redacted>
---
arch/powerpc/include/asm/debug.h | 1 +
arch/powerpc/include/asm/mmu_context.h | 56 +++++++++++++++++++++++++-
arch/powerpc/kernel/process.c | 5 +++
3 files changed, 61 insertions(+), 1 deletion(-)
From: Steven Rostedt <rostedt@goodmis.org> Date: 2020-04-24 13:14:40
On Fri, 17 Apr 2020 10:57:10 +1000
Michael Ellerman [off-list ref] wrote:
quoted
quoted
quoted
Does it needs to be a BUG_ON() ? Can't we fail gracefully with just a
WARN_ON ?
I'm not sure what failing gracefully means here? The main reason this could
fail is if there is not enough memory to allocate the patching_mm. The
previous implementation had this justification for BUG_ON():
But the system can continue running just fine after this failure.
Only the things that make use of code patching will fail (ftrace, kgdb, ...)
That's probably true of ftrace, but we can't fail patching for jump
labels (static keys).
See:
void arch_jump_label_transform(struct jump_entry *entry,
enum jump_label_type type)
{
u32 *addr = (u32 *)(unsigned long)entry->code;
if (type == JUMP_LABEL_JMP)
patch_branch(addr, entry->target, 0);
else
patch_instruction(addr, PPC_INST_NOP);
}
I would still error on a WARN_ON() as a lot of static keys should still
work if they don't get switched over.
If a user is concerned about something like this, they can always set
panic_on_warn.
-- Steve