lkml.org 
[lkml]   [2018]   [Nov]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 6/7] x86/alternatives: use temporary mm for text poking
On Fri, Nov 02, 2018 at 04:29:45PM -0700, Nadav Amit wrote:
> diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> index 9ceae28db1af..1a40df4db450 100644
> --- a/arch/x86/kernel/alternative.c
> +++ b/arch/x86/kernel/alternative.c

> @@ -699,41 +700,110 @@ __ro_after_init unsigned long poking_addr;
> */
> void *text_poke(void *addr, const void *opcode, size_t len)
> {
> + bool cross_page_boundary = offset_in_page(addr) + len > PAGE_SIZE;
> + temporary_mm_state_t prev;
> struct page *pages[2];
> + unsigned long flags;
> + pte_t pte, *ptep;
> + spinlock_t *ptl;
>
> /*
> + * While boot memory allocator is running we cannot use struct pages as
> + * they are not yet initialized.
> */
> BUG_ON(!after_bootmem);
>
> if (!core_kernel_text((unsigned long)addr)) {
> pages[0] = vmalloc_to_page(addr);
> + if (cross_page_boundary)
> + pages[1] = vmalloc_to_page(addr + PAGE_SIZE);
> } else {
> pages[0] = virt_to_page(addr);
> WARN_ON(!PageReserved(pages[0]));
> + if (cross_page_boundary)
> + pages[1] = virt_to_page(addr + PAGE_SIZE);
> }
> +
> + /* TODO: let the caller deal with a failure and fail gracefully. */
> BUG_ON(!pages[0]);
> + BUG_ON(cross_page_boundary && !pages[1]);
> local_irq_save(flags);
> +
> + /*
> + * The lock is not really needed, but this allows to avoid open-coding.
> + */
> + ptep = get_locked_pte(poking_mm, poking_addr, &ptl);
> +
> + /*
> + * If we failed to allocate a PTE, fail silently. The caller (text_poke)

we _are_ text_poke()..

> + * will detect that the write failed when it compares the memory with
> + * the new opcode.
> + */
> + if (unlikely(!ptep))
> + goto out;

This is the one site I'm a little uncomfortable with; OTOH it really
never should happen, since we explicitily instantiate these page-tables
earlier.

Can't we simply assume ptep will not be zero here? Like with so many
boot time memory allocations, we mostly assume they'll work.

> + pte = mk_pte(pages[0], PAGE_KERNEL);
> + set_pte_at(poking_mm, poking_addr, ptep, pte);
> +
> + if (cross_page_boundary) {
> + pte = mk_pte(pages[1], PAGE_KERNEL);
> + set_pte_at(poking_mm, poking_addr + PAGE_SIZE, ptep + 1, pte);
> + }
> +
> + /*
> + * Loading the temporary mm behaves as a compiler barrier, which
> + * guarantees that the PTE will be set at the time memcpy() is done.
> + */
> + prev = use_temporary_mm(poking_mm);
> +
> + kasan_disable_current();
> + memcpy((u8 *)poking_addr + offset_in_page(addr), opcode, len);
> + kasan_enable_current();
> +
> + /*
> + * Ensure that the PTE is only cleared after the instructions of memcpy
> + * were issued by using a compiler barrier.
> + */
> + barrier();
> +
> + pte_clear(poking_mm, poking_addr, ptep);
> +
> + /*
> + * __flush_tlb_one_user() performs a redundant TLB flush when PTI is on,
> + * as it also flushes the corresponding "user" address spaces, which
> + * does not exist.
> + *
> + * Poking, however, is already very inefficient since it does not try to
> + * batch updates, so we ignore this problem for the time being.
> + *
> + * Since the PTEs do not exist in other kernel address-spaces, we do
> + * not use __flush_tlb_one_kernel(), which when PTI is on would cause
> + * more unwarranted TLB flushes.
> + *
> + * There is a slight anomaly here: the PTE is a supervisor-only and
> + * (potentially) global and we use __flush_tlb_one_user() but this
> + * should be fine.
> + */
> + __flush_tlb_one_user(poking_addr);
> + if (cross_page_boundary) {
> + pte_clear(poking_mm, poking_addr + PAGE_SIZE, ptep + 1);
> + __flush_tlb_one_user(poking_addr + PAGE_SIZE);
> + }
> +
> + /*
> + * Loading the previous page-table hierarchy requires a serializing
> + * instruction that already allows the core to see the updated version.
> + * Xen-PV is assumed to serialize execution in a similar manner.
> + */
> + unuse_temporary_mm(prev);
> +
> + pte_unmap_unlock(ptep, ptl);
> +out:
> + /*
> + * TODO: allow the callers to deal with potential failures and do not
> + * panic so easily.
> + */
> + BUG_ON(memcmp(addr, opcode, len));
> local_irq_restore(flags);
> return addr;
> }

\
 
 \ /
  Last update: 2018-11-05 14:20    [W:0.220 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site