Re: [PATCH 3/5] powerpc/lib: Use a temporary mm for code patching
On Wed Jun 3, 2020 at 9:12 AM, Christophe Leroy wrote: > > > > > Le 03/06/2020 à 07:19, Christopher M. Riedl a écrit : > > 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. > > > > Use the KUAP functions on non-BOOKS3_64 platforms since the temporary > > mapping for patching uses a userspace address (to keep the mapping > > local). On BOOKS3_64 platforms hash does not implement KUAP and on radix > > the use of PAGE_KERNEL sets EAA[0] for the PTE which means the AMR > > (KUAP) protection is ignored (see PowerISA v3.0b, Fig, 35). > > > > Based on x86 implementation: > > > > commit b3fd8e83ada0 > > ("x86/alternatives: Use temporary mm for text poking") > > > > Signed-off-by: Christopher M. Riedl > > --- > > arch/powerpc/lib/code-patching.c | 148 --- > > 1 file changed, 55 insertions(+), 93 deletions(-) > > > > diff --git a/arch/powerpc/lib/code-patching.c > > b/arch/powerpc/lib/code-patching.c > > index 599114f63b44..df0765845204 100644 > > --- a/arch/powerpc/lib/code-patching.c > > +++ b/arch/powerpc/lib/code-patching.c > > @@ -20,6 +20,7 @@ > > #include > > #include > > #include > > +#include > > > > static int __patch_instruction(struct ppc_inst *exec_addr, struct > > ppc_inst instr, > >struct ppc_inst *patch_addr) > > @@ -78,101 +79,58 @@ void __init poking_init(void) > > pte_unmap_unlock(ptep, ptl); > > } > > > > -static DEFINE_PER_CPU(struct vm_struct *, text_poke_area); > > - > > -static int text_area_cpu_up(unsigned int cpu) > > -{ > > - struct vm_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); > > - > > - return 0; > > -} > > - > > -static int text_area_cpu_down(unsigned int cpu) > > -{ > > - free_vm_area(this_cpu_read(text_poke_area)); > > - return 0; > > -} > > - > > -/* > > - * 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); > > +struct patch_mapping { > > + spinlock_t *ptl; /* for protecting pte table */ > > + pte_t *ptep; > > + struct temp_mm temp_mm; > > +}; > > > > /* > >* This can be called for kernel text or a module. > >*/ > > -static int map_patch_area(void *addr, unsigned long text_poke_addr) > > +static int map_patch(const void *addr, struct patch_mapping *patch_mapping) > > { > > - unsigned long pfn; > > - int err; > > + struct page *page; > > + pte_t pte; > > + pgprot_t pgprot; > > > > 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 = PAGE_KERNEL; > > + else > > + pgprot = PAGE_SHARED; > > > > - pr_devel("Mapped addr %lx with pfn %lx:%d\n", text_poke_addr, pfn, err); > > - if (err) > > + patch_mapping->ptep = get_locked_pte(patching_mm, patching_addr, > > +_mapping->ptl); > > + if (unlikely(!patch_mapping->ptep)) { > > + pr_warn("map patch: failed to allocate pte for patching\n"); > > return -1; > > + } > > + > > + pte = mk_pte(page, pgprot); > > + if (!IS_ENABLED(CONFIG_PPC_BOOK3S_64)) > > + pte = pte_mkdirty(pte); > > > Are you should you don't need the DIRTY bit for BOOK3S/64 non radix ? > > > I think the DIRTY bit is needed always, and adding it when it is already > there is harmless, so it should be done inconditionnnaly. > I tested this and it doesn't seem to make a differnce so I can make this common in the next spin. > > > + set_pte_at(patching_mm, patching_addr, patch_mapping->ptep, pte); > > + > > + init_temp_mm(_mapping->temp_mm, patching_mm); > > +
Re: [PATCH 3/5] powerpc/lib: Use a temporary mm for code patching
Le 03/06/2020 à 07:19, Christopher M. Riedl a écrit : 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. Use the KUAP functions on non-BOOKS3_64 platforms since the temporary mapping for patching uses a userspace address (to keep the mapping local). On BOOKS3_64 platforms hash does not implement KUAP and on radix the use of PAGE_KERNEL sets EAA[0] for the PTE which means the AMR (KUAP) protection is ignored (see PowerISA v3.0b, Fig, 35). Based on x86 implementation: commit b3fd8e83ada0 ("x86/alternatives: Use temporary mm for text poking") Signed-off-by: Christopher M. Riedl --- arch/powerpc/lib/code-patching.c | 148 --- 1 file changed, 55 insertions(+), 93 deletions(-) diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index 599114f63b44..df0765845204 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -20,6 +20,7 @@ #include #include #include +#include static int __patch_instruction(struct ppc_inst *exec_addr, struct ppc_inst instr, struct ppc_inst *patch_addr) @@ -78,101 +79,58 @@ void __init poking_init(void) pte_unmap_unlock(ptep, ptl); } -static DEFINE_PER_CPU(struct vm_struct *, text_poke_area); - -static int text_area_cpu_up(unsigned int cpu) -{ - struct vm_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); - - return 0; -} - -static int text_area_cpu_down(unsigned int cpu) -{ - free_vm_area(this_cpu_read(text_poke_area)); - return 0; -} - -/* - * 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); +struct patch_mapping { + spinlock_t *ptl; /* for protecting pte table */ + pte_t *ptep; + struct temp_mm temp_mm; +}; /* * This can be called for kernel text or a module. */ -static int map_patch_area(void *addr, unsigned long text_poke_addr) +static int map_patch(const void *addr, struct patch_mapping *patch_mapping) { - unsigned long pfn; - int err; + struct page *page; + pte_t pte; + pgprot_t pgprot; 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 = PAGE_KERNEL; + else + pgprot = PAGE_SHARED; - pr_devel("Mapped addr %lx with pfn %lx:%d\n", text_poke_addr, pfn, err); - if (err) + patch_mapping->ptep = get_locked_pte(patching_mm, patching_addr, +_mapping->ptl); + if (unlikely(!patch_mapping->ptep)) { + pr_warn("map patch: failed to allocate pte for patching\n"); return -1; + } + + pte = mk_pte(page, pgprot); + if (!IS_ENABLED(CONFIG_PPC_BOOK3S_64)) + pte = pte_mkdirty(pte); Are you should you don't need the DIRTY bit for BOOK3S/64 non radix ? I think the DIRTY bit is needed always, and adding it when it is already there is harmless, so it should be done inconditionnnaly. + set_pte_at(patching_mm, patching_addr, patch_mapping->ptep, pte); + + init_temp_mm(_mapping->temp_mm, patching_mm); + use_temporary_mm(_mapping->temp_mm); return 0; } -static inline int unmap_patch_area(unsigned long addr) +static void unmap_patch(struct patch_mapping *patch_mapping) { - pte_t *ptep; - pmd_t *pmdp; - pud_t *pudp; - pgd_t *pgdp; - - pgdp = pgd_offset_k(addr); - if (unlikely(!pgdp)) - return -EINVAL; - - pudp = pud_offset(pgdp, addr); - if (unlikely(!pudp)) - return -EINVAL; - - pmdp = pmd_offset(pudp, addr); -
[PATCH 3/5] powerpc/lib: Use a temporary mm for code patching
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. Use the KUAP functions on non-BOOKS3_64 platforms since the temporary mapping for patching uses a userspace address (to keep the mapping local). On BOOKS3_64 platforms hash does not implement KUAP and on radix the use of PAGE_KERNEL sets EAA[0] for the PTE which means the AMR (KUAP) protection is ignored (see PowerISA v3.0b, Fig, 35). Based on x86 implementation: commit b3fd8e83ada0 ("x86/alternatives: Use temporary mm for text poking") Signed-off-by: Christopher M. Riedl --- arch/powerpc/lib/code-patching.c | 148 --- 1 file changed, 55 insertions(+), 93 deletions(-) diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index 599114f63b44..df0765845204 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -20,6 +20,7 @@ #include #include #include +#include static int __patch_instruction(struct ppc_inst *exec_addr, struct ppc_inst instr, struct ppc_inst *patch_addr) @@ -78,101 +79,58 @@ void __init poking_init(void) pte_unmap_unlock(ptep, ptl); } -static DEFINE_PER_CPU(struct vm_struct *, text_poke_area); - -static int text_area_cpu_up(unsigned int cpu) -{ - struct vm_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); - - return 0; -} - -static int text_area_cpu_down(unsigned int cpu) -{ - free_vm_area(this_cpu_read(text_poke_area)); - return 0; -} - -/* - * 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); +struct patch_mapping { + spinlock_t *ptl; /* for protecting pte table */ + pte_t *ptep; + struct temp_mm temp_mm; +}; /* * This can be called for kernel text or a module. */ -static int map_patch_area(void *addr, unsigned long text_poke_addr) +static int map_patch(const void *addr, struct patch_mapping *patch_mapping) { - unsigned long pfn; - int err; + struct page *page; + pte_t pte; + pgprot_t pgprot; 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 = PAGE_KERNEL; + else + pgprot = PAGE_SHARED; - pr_devel("Mapped addr %lx with pfn %lx:%d\n", text_poke_addr, pfn, err); - if (err) + patch_mapping->ptep = get_locked_pte(patching_mm, patching_addr, +_mapping->ptl); + if (unlikely(!patch_mapping->ptep)) { + pr_warn("map patch: failed to allocate pte for patching\n"); return -1; + } + + pte = mk_pte(page, pgprot); + if (!IS_ENABLED(CONFIG_PPC_BOOK3S_64)) + pte = pte_mkdirty(pte); + set_pte_at(patching_mm, patching_addr, patch_mapping->ptep, pte); + + init_temp_mm(_mapping->temp_mm, patching_mm); + use_temporary_mm(_mapping->temp_mm); return 0; } -static inline int unmap_patch_area(unsigned long addr) +static void unmap_patch(struct patch_mapping *patch_mapping) { - pte_t *ptep; - pmd_t *pmdp; - pud_t *pudp; - pgd_t *pgdp; - - pgdp = pgd_offset_k(addr); - if (unlikely(!pgdp)) - return -EINVAL; - - pudp = pud_offset(pgdp, addr); - if (unlikely(!pudp)) - return -EINVAL; - - pmdp = pmd_offset(pudp, addr); - if (unlikely(!pmdp)) - return -EINVAL; - - ptep = pte_offset_kernel(pmdp, addr); - if (unlikely(!ptep)) - return -EINVAL; + /* In hash, pte_clear flushes the tlb */ + pte_clear(patching_mm, patching_addr,