> On April 15, 2020 3:45 AM Christophe Leroy <christophe.le...@c-s.fr> wrote:
>
>
> Le 15/04/2020 à 07:11, Christopher M Riedl a écrit :
> >> On March 24, 2020 11:25 AM Christophe Leroy <christophe.le...@c-s.fr>
> >> wrote:
> >>
> >>
> >> Le 23/03/2020 à 05:52, 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.
> >>>
> >>> Based on x86 implementation:
> >>>
> >>> commit b3fd8e83ada0
> >>> ("x86/alternatives: Use temporary mm for text poking")
> >>>
> >>> Signed-off-by: Christopher M. Riedl <c...@informatik.wtf>
> >>> ---
> >>> arch/powerpc/lib/code-patching.c | 128 ++++++++++++++-----------------
> >>> 1 file changed, 57 insertions(+), 71 deletions(-)
> >>>
> >>> diff --git a/arch/powerpc/lib/code-patching.c
> >>> b/arch/powerpc/lib/code-patching.c
> >>> index 18b88ecfc5a8..f156132e8975 100644
> >>> --- a/arch/powerpc/lib/code-patching.c
> >>> +++ b/arch/powerpc/lib/code-patching.c
> >>> @@ -19,6 +19,7 @@
> >>> #include <asm/page.h>
> >>> #include <asm/code-patching.h>
> >>> #include <asm/setup.h>
> >>> +#include <asm/mmu_context.h>
> >>>
> >>> static int __patch_instruction(unsigned int *exec_addr, unsigned int
> >>> instr,
> >>> unsigned int *patch_addr)
> >>> @@ -65,99 +66,79 @@ 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 */
> >>> + 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)
> >>
> >> Why change the name ?
> >>
> >
> > It's not really an "area" anymore.
> >
> >>> {
> >>> - unsigned long pfn;
> >>> - int err;
> >>> + struct page *page;
> >>> + pte_t pte, *ptep;
> >>> + 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 = __pgprot(pgprot_val(PAGE_KERNEL));
> >>> + else
> >>> + pgprot = PAGE_SHARED;
> >>
> >> 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.
>
>
>
> Christophe
