[PATCH 0/5] x86: Implement interrupt-safe livepatching

[PATCH 0/5] x86: Implement interrupt-safe livepatching

[Andrew Cooper]

So it turns out that this is quite a lot easier than I feared.

Only compile-tested on ARM, but functionally tested on x86.

Andrew Cooper (5):
  arm/alternatives: Fix apply_alternatives() API
  xen/alternatives: Plumb a 'live' parameter through apply_alternatives()
  x86/livepatch: Use text_poke() and plumb a live parameter through
  x86/alternative: Implement NMI/#MC-safe patching
  DO-NOT-APPLY - Demonstrates an NMI hitting an in-progress patch

 xen/arch/arm/alternative.c        |  20 +++---
 xen/arch/x86/alternative.c        | 131 ++++++++++++++++++++++++++++++++++++--
 xen/arch/x86/livepatch.c          |   8 +--
 xen/arch/x86/traps.c              |   2 +
 xen/arch/x86/x86_64/entry.S       |  38 ++++++++++-
 xen/common/livepatch.c            |   2 +-
 xen/include/asm-arm/alternative.h |   9 ++-
 xen/include/asm-x86/alternative.h |   5 +-
 xen/include/asm-x86/processor.h   |   2 +
 9 files changed, 187 insertions(+), 30 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 1/5] arm/alternatives: Fix apply_alternatives() API

[Andrew Cooper]

The !HAS_ALTERNATIVE case has bit-rotten and won't even compile.

The x86 side of apply_alternatives() is void, while the ARM side returns int.
However, the functions can't fail and no return values are checked.  Switch
the ARM side to be void as well.

One observation is that __apply_alternatives_multi_stop() is only used at boot
time, so can become __init and its static data can become __initdata

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>

TODO: Drop the !HAS_ALTERNATIVE case on ARM?
---
 xen/arch/arm/alternative.c        | 18 +++++++-----------
 xen/include/asm-arm/alternative.h |  7 ++++---
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 9ffdc47..99112e1 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -98,7 +98,7 @@ static u32 get_alt_insn(const struct alt_instr *alt,
  * The region patched should be read-write to allow __apply_alternatives
  * to replacing the instructions when necessary.
  */
-static int __apply_alternatives(const struct alt_region *region)
+static void __apply_alternatives(const struct alt_region *region)
 {
     const struct alt_instr *alt;
     const u32 *replptr;
@@ -135,17 +135,15 @@ static int __apply_alternatives(const struct alt_region *region)
 
     /* Nuke the instruction cache */
     invalidate_icache();
-
-    return 0;
 }
 
 /*
  * We might be patching the stop_machine state machine, so implement a
  * really simple polling protocol here.
  */
-static int __apply_alternatives_multi_stop(void *unused)
+static int __init __apply_alternatives_multi_stop(void *unused)
 {
-    static int patched = 0;
+    static int __initdata patched = 0;
 
     /* We always have a CPU 0 at this point (__init) */
     if ( smp_processor_id() )
@@ -156,7 +154,6 @@ static int __apply_alternatives_multi_stop(void *unused)
     }
     else
     {
-        int ret;
         struct alt_region region;
         mfn_t xen_mfn = virt_to_mfn(_start);
         paddr_t xen_size = _end - _start;
@@ -196,9 +193,7 @@ static int __apply_alternatives_multi_stop(void *unused)
         region.begin = (void *)__alt_instructions - (void *)_start + xenmap;
         region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;
 
-        ret = __apply_alternatives(&region);
-        /* The patching is not expected to fail during boot. */
-        BUG_ON(ret != 0);
+        __apply_alternatives(&region);
 
         unregister_virtual_region(&patch_region);
 
@@ -228,14 +223,15 @@ void __init apply_alternatives_all(void)
     BUG_ON(ret);
 }
 
-int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end)
+void apply_alternatives(const struct alt_instr *start,
+                        const struct alt_instr *end)
 {
     const struct alt_region region = {
         .begin = start,
         .end = end,
     };
 
-    return __apply_alternatives(&region);
+    __apply_alternatives(&region);
 }
 
 /*
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index 6cc9d0d..49a055e 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -25,7 +25,8 @@ struct alt_instr {
 #define ALT_REPL_PTR(a)		__ALT_PTR(a, alt_offset)
 
 void __init apply_alternatives_all(void);
-int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end);
+void apply_alternatives(const struct alt_instr *start,
+                        const struct alt_instr *end);
 
 #define ALTINSTR_ENTRY(feature)						      \
 	" .word 661b - .\n"				/* label           */ \
@@ -158,9 +159,9 @@ static inline void apply_alternatives_all(void)
 {
 }
 
-static inline int apply_alternatives(void *start, size_t length)
+static inline void apply_alternatives(const struct alt_instr *start,
+                                      const struct alt_instr *end)
 {
-    return 0;
 }
 
 #endif
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 2/5] xen/alternatives: Plumb a 'live' parameter through apply_alternatives()

[Andrew Cooper]

On x86, we would like to alter how we patch based on whether there is any
chance of the code being patched being concurrently executed.

prepare_payload() passes false (as the livepatch definitely isn't live at this
point), whereas the boot-time alternatives application passes true.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/arm/alternative.c        | 10 ++++++----
 xen/arch/x86/alternative.c        |  5 +++--
 xen/common/livepatch.c            |  2 +-
 xen/include/asm-arm/alternative.h |  6 ++++--
 xen/include/asm-x86/alternative.h |  3 ++-
 5 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
index 99112e1..078b259 100644
--- a/xen/arch/arm/alternative.c
+++ b/xen/arch/arm/alternative.c
@@ -98,7 +98,8 @@ static u32 get_alt_insn(const struct alt_instr *alt,
  * The region patched should be read-write to allow __apply_alternatives
  * to replacing the instructions when necessary.
  */
-static void __apply_alternatives(const struct alt_region *region)
+static void __apply_alternatives(const struct alt_region *region,
+                                 bool live)
 {
     const struct alt_instr *alt;
     const u32 *replptr;
@@ -193,7 +194,7 @@ static int __init __apply_alternatives_multi_stop(void *unused)
         region.begin = (void *)__alt_instructions - (void *)_start + xenmap;
         region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;
 
-        __apply_alternatives(&region);
+        __apply_alternatives(&region, true);
 
         unregister_virtual_region(&patch_region);
 
@@ -224,14 +225,15 @@ void __init apply_alternatives_all(void)
 }
 
 void apply_alternatives(const struct alt_instr *start,
-                        const struct alt_instr *end)
+                        const struct alt_instr *end,
+                        bool live)
 {
     const struct alt_region region = {
         .begin = start,
         .end = end,
     };
 
-    __apply_alternatives(&region);
+    __apply_alternatives(&region, live);
 }
 
 /*
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index ee18e6c..5b3fb80 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -163,7 +163,8 @@ text_poke(void *addr, const void *opcode, size_t len)
  * Tough. Make sure you disable such features by hand.
  */
 void init_or_livepatch apply_alternatives(const struct alt_instr *start,
-                                          const struct alt_instr *end)
+                                          const struct alt_instr *end,
+                                          bool live)
 {
     const struct alt_instr *a;
     u8 *instr, *replacement;
@@ -235,7 +236,7 @@ void __init alternative_instructions(void)
     /* Disable WP to allow application of alternatives to read-only pages. */
     write_cr0(cr0 & ~X86_CR0_WP);
 
-    apply_alternatives(__alt_instructions, __alt_instructions_end);
+    apply_alternatives(__alt_instructions, __alt_instructions_end, true);
 
     /* Reinstate WP. */
     write_cr0(cr0);
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index b9376c9..6f03e89 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload,
                 return -EINVAL;
             }
         }
-        apply_alternatives(start, end);
+        apply_alternatives(start, end, false);
 #else
         dprintk(XENLOG_ERR, LIVEPATCH "%s: We don't support alternative patching!\n",
                 elf->name);
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index 49a055e..77b0944 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -26,7 +26,8 @@ struct alt_instr {
 
 void __init apply_alternatives_all(void);
 void apply_alternatives(const struct alt_instr *start,
-                        const struct alt_instr *end);
+                        const struct alt_instr *end,
+                        bool live);
 
 #define ALTINSTR_ENTRY(feature)						      \
 	" .word 661b - .\n"				/* label           */ \
@@ -160,7 +161,8 @@ static inline void apply_alternatives_all(void)
 }
 
 static inline void apply_alternatives(const struct alt_instr *start,
-                                      const struct alt_instr *end)
+                                      const struct alt_instr *end,
+                                      bool live)
 {
 }
 
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index ba537d6..07ff424 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -23,7 +23,8 @@ struct alt_instr {
 extern void add_nops(void *insns, unsigned int len);
 /* Similar to alternative_instructions except it can be run with IRQs enabled. */
 extern void apply_alternatives(const struct alt_instr *start,
-                               const struct alt_instr *end);
+                               const struct alt_instr *end,
+                               bool live);
 extern void alternative_instructions(void);
 
 #define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 3/5] x86/livepatch: Use text_poke() and plumb a live parameter through

[Andrew Cooper]

Update text_poke() to return void (rather than void *), take a live parameter,
and export it for arch_livepatch_apply() to use.

arch_livepatch_apply() can therefore lose its noinline for i-cache safey, as
text_poke() is doing the heavy lifting.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/alternative.c        | 9 +++++----
 xen/arch/x86/livepatch.c          | 8 ++------
 xen/include/asm-x86/alternative.h | 2 ++
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 5b3fb80..0b309c3 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -136,6 +136,7 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
  * @addr: address to modify
  * @opcode: source of the copy
  * @len: length to copy
+ * @live: whether @addr is liable to be executed during patching
  *
  * When you use this code to patch more than one byte of an instruction
  * you need to make sure that other CPUs cannot execute this code in parallel.
@@ -149,10 +150,10 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
  * "noinline" to cause control flow change and thus invalidate I$ and
  * cause refetch after modification.
  */
-static void *init_or_livepatch noinline
-text_poke(void *addr, const void *opcode, size_t len)
+void init_or_livepatch noinline
+text_poke(void *addr, const void *opcode, size_t len, bool live)
 {
-    return memcpy(addr, opcode, len);
+    memcpy(addr, opcode, len);
 }
 
 /*
@@ -199,7 +200,7 @@ void init_or_livepatch apply_alternatives(const struct alt_instr *start,
 
         add_nops(insnbuf + a->replacementlen,
                  a->instrlen - a->replacementlen);
-        text_poke(instr, insnbuf, a->instrlen);
+        text_poke(instr, insnbuf, a->instrlen, live);
     }
 }
 
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 406eb91..e624474 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -46,11 +46,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
     return 0;
 }
 
-/*
- * "noinline" to cause control flow change and thus invalidate I$ and
- * cause refetch after modification.
- */
-void noinline arch_livepatch_apply(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
 {
     uint8_t *old_ptr;
     uint8_t insn[sizeof(func->opaque)];
@@ -76,7 +72,7 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
     else
         add_nops(insn, len);
 
-    memcpy(old_ptr, insn, len);
+    text_poke(old_ptr, insn, len, true);
 }
 
 /*
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index 07ff424..2d7cca5 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -20,6 +20,8 @@ struct alt_instr {
 #define ALT_ORIG_PTR(a)     __ALT_PTR(a, instr_offset)
 #define ALT_REPL_PTR(a)     __ALT_PTR(a, repl_offset)
 
+extern void text_poke(void *addr, const void *opcode, size_t len, bool live);
+
 extern void add_nops(void *insns, unsigned int len);
 /* Similar to alternative_instructions except it can be run with IRQs enabled. */
 extern void apply_alternatives(const struct alt_instr *start,
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 4/5] x86/alternative: Implement NMI/#MC-safe patching

[Andrew Cooper]

Patching code which is being executed is problematic, because it impossible to
arrange an atomic update of the instruction stream outside of a few corner
cases.  Furthermore, we have no feasible way to prevent execution of the NMI
and #MC exception handlers, but have patch points in them.

Use a breakpoint to capture execution which hits an in-progress patch, and
have the exception handler cope with the safety.

See the code comments for exact algorithm details.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/alternative.c      | 104 +++++++++++++++++++++++++++++++++++++++-
 xen/arch/x86/traps.c            |   2 +
 xen/arch/x86/x86_64/entry.S     |  34 ++++++++++++-
 xen/include/asm-x86/processor.h |   2 +
 4 files changed, 139 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 0b309c3..40bfaad 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -131,6 +131,84 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
     }
 }
 
+static init_or_livepatch_data struct live_poke_info {
+    uint8_t *addr;
+    const uint8_t *opcode;
+    size_t len;
+    unsigned int cpu;
+} live_poke_info;
+
+/*
+ * text_poke_live - Update the live .text area, in an interrupt-safe way.
+ *
+ * !!! WARNING - Reentrantly called from the int3 exception handler !!!
+ *
+ * All patching data are passed via the live_poke_info structure.  @regs is
+ * non-NULL if entering from an exception handler.
+ *
+ * Returns a boolean indicating whether the transient breakpoint was hit.
+ */
+bool init_or_livepatch text_poke_live(const struct cpu_user_regs *regs)
+{
+    struct live_poke_info *i = &live_poke_info;
+
+    if ( unlikely(i->cpu != smp_processor_id()) )
+    {
+        ASSERT(regs);
+
+        /*
+         * We hit a breakpoint, on a CPU which was not performing the
+         * patching.  This is only expected to be possible for the NMI/#MC
+         * paths, and even then, only if we hit the tiny race window below
+         * while patching in the NMI/#MC handlers.
+         *
+         * We can't safely evaluate whether we hit a transient breakpoint
+         * because i->cpu has likely completed the patch and moved on to the
+         * next patch site.
+         *
+         * Go to sleep for a bit and synchronise the pipeline as we are now in
+         * a cross-modifying scenario.
+         */
+        cpu_relax();
+        cpuid_eax(0);
+
+        /*
+         * Presume that we hit the transient breakpoint, as we can't confirm
+         * whether we did or not.  We don't expect there to be any other
+         * breakpoints to hit, but if we did hit another one, then in the
+         * worse case we will only livelock until i->cpu has finished all of
+         * its patching.
+         */
+        return true;
+    }
+
+    /*
+     * We are the CPU performing the patching, and might have ended up here by
+     * hitting a breakpoint.
+     *
+     * Either way, we need to complete particular patch to make forwards
+     * progress.  This logic is safe even if executed recursively in the
+     * breakpoint handler; the worst that will happen when normal execution
+     * resumes is that we will rewrite the same bytes a second time.
+     */
+
+    /* First, insert a breakpoint to prevent execution of the patch site. */
+    i->addr[0] = 0xcc;
+    smp_wmb();
+    /* Second, copy the remaining instructions into place. */
+    memcpy(i->addr + 1, i->opcode + 1, i->len - 1);
+    smp_wmb();
+    /* Third, replace the breakpoint with the real instruction byte. */
+    i->addr[0] = i->opcode[0];
+
+    /*
+     * Inform our caller whether we hit the transient breakpoint (in which
+     * case we iret normally, having already finished the patch), or whether
+     * we need to continue further into the debug trap handler.
+     */
+    return regs && regs->rip == (unsigned long)&i->addr[1];
+}
+
 /*
  * text_poke - Update instructions on a live kernel or non-executed code.
  * @addr: address to modify
@@ -153,7 +231,31 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
 void init_or_livepatch noinline
 text_poke(void *addr, const void *opcode, size_t len, bool live)
 {
-    memcpy(addr, opcode, len);
+    if ( !live || len == 1 )
+    {
+        /*
+         * If we know *addr can't be executed, or we are patching a single
+         * byte, it is safe to use a straight memcpy().
+         */
+        memcpy(addr, opcode, len);
+    }
+    else
+    {
+        /*
+         * If not, arrange safe patching via the use of breakpoints.  Ordering
+         * of actions here are between this CPU, and the instruction fetch of
+         * the breakpoint exception handler on any CPU.
+         */
+        live_poke_info = (struct live_poke_info){
+            addr, opcode, len, smp_processor_id()
+        };
+        smp_wmb();
+        active_text_patching = true;
+        smp_wmb();
+        text_poke_live(NULL);
+        smp_wmb();
+        active_text_patching = false;
+    }
 }
 
 /*
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index a3e8f0c..6bea963 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -119,6 +119,8 @@ boolean_param("ler", opt_ler);
 #define stack_words_per_line 4
 #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
 
+bool active_text_patching;
+
 static void show_code(const struct cpu_user_regs *regs)
 {
     unsigned char insns_before[8] = {}, insns_after[16] = {};
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index a5a6702..56f52c7 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -530,7 +530,10 @@ ENTRY(page_fault)
         movl  $TRAP_page_fault,4(%rsp)
 /* No special register assumptions. */
 GLOBAL(handle_exception)
-        SAVE_ALL CLAC
+        SAVE_ALL
+
+handle_exception_gprs_saved:
+        ASM_CLAC
 
         GET_STACK_END(14)
 
@@ -686,9 +689,36 @@ ENTRY(debug)
         jmp   handle_exception
 
 ENTRY(int3)
+        /* For patching-safety, there must not be any alternatives here. */
         pushq $0
         movl  $TRAP_int3,4(%rsp)
-        jmp   handle_exception
+
+        /* If there is no patching active, continue normally.  */
+        cmpb  $1, active_text_patching(%rip)
+        jne   handle_exception
+
+        /*
+         * We hit a debug trap, but not necessarily the one from active
+         * patching.  Let text_poke_live() work out what to do.
+         */
+        SAVE_ALL
+        mov   %rsp, %rdi
+        call  text_poke_live
+
+        /*
+         * Does text_poke_live() think we hit the transient debug trap?  If
+         * not, continue down the normal int3 handler.
+         */
+        cmp   $0, %eax
+        je    handle_exception_gprs_saved
+
+        /*
+         * We think we hit the transient debug trap.  text_poke_live() has
+         * probably completed the patching, so rewind the instruction pointer
+         * and try again.
+         */
+        subq  $1, UREGS_rip(%rsp)
+        jmp   restore_all_xen
 
 ENTRY(overflow)
         pushq $0
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index e8c2f02..5433157 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -439,6 +439,8 @@ extern idt_entry_t *idt_tables[];
 DECLARE_PER_CPU(struct tss_struct, init_tss);
 DECLARE_PER_CPU(root_pgentry_t *, root_pgt);
 
+extern bool active_text_patching;
+
 extern void init_int80_direct_trap(struct vcpu *v);
 
 extern void write_ptbase(struct vcpu *v);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 5/5] DO-NOT-APPLY - Demonstrates an NMI hitting an in-progress patch

[Andrew Cooper]

---
 xen/arch/x86/alternative.c  | 15 +++++++++++++++
 xen/arch/x86/x86_64/entry.S |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 40bfaad..10e423c 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -138,6 +138,9 @@ static init_or_livepatch_data struct live_poke_info {
     unsigned int cpu;
 } live_poke_info;
 
+extern void nmi_patch_point(void);
+#include <asm/apic.h>
+
 /*
  * text_poke_live - Update the live .text area, in an interrupt-safe way.
  *
@@ -197,6 +200,18 @@ bool init_or_livepatch text_poke_live(const struct cpu_user_regs *regs)
     smp_wmb();
     /* Second, copy the remaining instructions into place. */
     memcpy(i->addr + 1, i->opcode + 1, i->len - 1);
+
+    if ( _p(i->addr) == _p(nmi_patch_point) )
+    {
+        if ( !regs )
+        {
+            printk("Found NMI patch point\n");
+            apic_icr_write(APIC_DEST_SELF | APIC_DM_NMI, 0);
+        }
+        else
+            printk("Hit debugtrap in NMI\n");
+    }
+
     smp_wmb();
     /* Third, replace the breakpoint with the real instruction byte. */
     i->addr[0] = i->opcode[0];
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 56f52c7..1337562 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -790,6 +790,10 @@ ENTRY(early_page_fault)
 ENTRY(nmi)
         pushq $0
         movl  $TRAP_nmi,4(%rsp)
+
+GLOBAL(nmi_patch_point)
+        ALTERNATIVE __stringify(ASM_NOP3), "lfence", X86_FEATURE_ALWAYS
+
 handle_ist_exception:
         SAVE_ALL CLAC
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/5] arm/alternatives: Fix apply_alternatives() API

[Stefano Stabellini]

On Mon, 29 Jan 2018, Andrew Cooper wrote:
> The !HAS_ALTERNATIVE case has bit-rotten and won't even compile.
> 
> The x86 side of apply_alternatives() is void, while the ARM side returns int.
> However, the functions can't fail and no return values are checked.  Switch
> the ARM side to be void as well.
> 
> One observation is that __apply_alternatives_multi_stop() is only used at boot
> time, so can become __init and its static data can become __initdata
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> TODO: Drop the !HAS_ALTERNATIVE case on ARM?

Yeah, we could do. Julien, what do you think?


> ---
>  xen/arch/arm/alternative.c        | 18 +++++++-----------
>  xen/include/asm-arm/alternative.h |  7 ++++---
>  2 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 9ffdc47..99112e1 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -98,7 +98,7 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>   * The region patched should be read-write to allow __apply_alternatives
>   * to replacing the instructions when necessary.
>   */
> -static int __apply_alternatives(const struct alt_region *region)
> +static void __apply_alternatives(const struct alt_region *region)
>  {
>      const struct alt_instr *alt;
>      const u32 *replptr;
> @@ -135,17 +135,15 @@ static int __apply_alternatives(const struct alt_region *region)
>  
>      /* Nuke the instruction cache */
>      invalidate_icache();
> -
> -    return 0;
>  }
>  
>  /*
>   * We might be patching the stop_machine state machine, so implement a
>   * really simple polling protocol here.
>   */
> -static int __apply_alternatives_multi_stop(void *unused)
> +static int __init __apply_alternatives_multi_stop(void *unused)
>  {
> -    static int patched = 0;
> +    static int __initdata patched = 0;
>  
>      /* We always have a CPU 0 at this point (__init) */
>      if ( smp_processor_id() )
> @@ -156,7 +154,6 @@ static int __apply_alternatives_multi_stop(void *unused)
>      }
>      else
>      {
> -        int ret;
>          struct alt_region region;
>          mfn_t xen_mfn = virt_to_mfn(_start);
>          paddr_t xen_size = _end - _start;
> @@ -196,9 +193,7 @@ static int __apply_alternatives_multi_stop(void *unused)
>          region.begin = (void *)__alt_instructions - (void *)_start + xenmap;
>          region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;
>  
> -        ret = __apply_alternatives(&region);
> -        /* The patching is not expected to fail during boot. */
> -        BUG_ON(ret != 0);
> +        __apply_alternatives(&region);
>  
>          unregister_virtual_region(&patch_region);
>  
> @@ -228,14 +223,15 @@ void __init apply_alternatives_all(void)
>      BUG_ON(ret);
>  }
>  
> -int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end)
> +void apply_alternatives(const struct alt_instr *start,
> +                        const struct alt_instr *end)
>  {
>      const struct alt_region region = {
>          .begin = start,
>          .end = end,
>      };
>  
> -    return __apply_alternatives(&region);
> +    __apply_alternatives(&region);
>  }
>  
>  /*
> diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
> index 6cc9d0d..49a055e 100644
> --- a/xen/include/asm-arm/alternative.h
> +++ b/xen/include/asm-arm/alternative.h
> @@ -25,7 +25,8 @@ struct alt_instr {
>  #define ALT_REPL_PTR(a)		__ALT_PTR(a, alt_offset)
>  
>  void __init apply_alternatives_all(void);
> -int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end);
> +void apply_alternatives(const struct alt_instr *start,
> +                        const struct alt_instr *end);
>  
>  #define ALTINSTR_ENTRY(feature)						      \
>  	" .word 661b - .\n"				/* label           */ \
> @@ -158,9 +159,9 @@ static inline void apply_alternatives_all(void)
>  {
>  }
>  
> -static inline int apply_alternatives(void *start, size_t length)
> +static inline void apply_alternatives(const struct alt_instr *start,
> +                                      const struct alt_instr *end)
>  {
> -    return 0;
>  }
>  
>  #endif
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/5] xen/alternatives: Plumb a 'live' parameter through apply_alternatives()

[Stefano Stabellini]

On Mon, 29 Jan 2018, Andrew Cooper wrote:
> On x86, we would like to alter how we patch based on whether there is any
> chance of the code being patched being concurrently executed.
> 
> prepare_payload() passes false (as the livepatch definitely isn't live at this
> point), whereas the boot-time alternatives application passes true.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>  xen/arch/arm/alternative.c        | 10 ++++++----
>  xen/arch/x86/alternative.c        |  5 +++--
>  xen/common/livepatch.c            |  2 +-
>  xen/include/asm-arm/alternative.h |  6 ++++--
>  xen/include/asm-x86/alternative.h |  3 ++-
>  5 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 99112e1..078b259 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -98,7 +98,8 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>   * The region patched should be read-write to allow __apply_alternatives
>   * to replacing the instructions when necessary.
>   */
> -static void __apply_alternatives(const struct alt_region *region)
> +static void __apply_alternatives(const struct alt_region *region,
> +                                 bool live)
>  {
>      const struct alt_instr *alt;
>      const u32 *replptr;
> @@ -193,7 +194,7 @@ static int __init __apply_alternatives_multi_stop(void *unused)
>          region.begin = (void *)__alt_instructions - (void *)_start + xenmap;
>          region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;
>  
> -        __apply_alternatives(&region);
> +        __apply_alternatives(&region, true);
>  
>          unregister_virtual_region(&patch_region);
>  
> @@ -224,14 +225,15 @@ void __init apply_alternatives_all(void)
>  }
>  
>  void apply_alternatives(const struct alt_instr *start,
> -                        const struct alt_instr *end)
> +                        const struct alt_instr *end,
> +                        bool live)
>  {
>      const struct alt_region region = {
>          .begin = start,
>          .end = end,
>      };
>  
> -    __apply_alternatives(&region);
> +    __apply_alternatives(&region, live);
>  }
>  
>  /*
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index ee18e6c..5b3fb80 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -163,7 +163,8 @@ text_poke(void *addr, const void *opcode, size_t len)
>   * Tough. Make sure you disable such features by hand.
>   */
>  void init_or_livepatch apply_alternatives(const struct alt_instr *start,
> -                                          const struct alt_instr *end)
> +                                          const struct alt_instr *end,
> +                                          bool live)
>  {
>      const struct alt_instr *a;
>      u8 *instr, *replacement;
> @@ -235,7 +236,7 @@ void __init alternative_instructions(void)
>      /* Disable WP to allow application of alternatives to read-only pages. */
>      write_cr0(cr0 & ~X86_CR0_WP);
>  
> -    apply_alternatives(__alt_instructions, __alt_instructions_end);
> +    apply_alternatives(__alt_instructions, __alt_instructions_end, true);
>  
>      /* Reinstate WP. */
>      write_cr0(cr0);
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index b9376c9..6f03e89 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload,
>                  return -EINVAL;
>              }
>          }
> -        apply_alternatives(start, end);
> +        apply_alternatives(start, end, false);
>  #else
>          dprintk(XENLOG_ERR, LIVEPATCH "%s: We don't support alternative patching!\n",
>                  elf->name);
> diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
> index 49a055e..77b0944 100644
> --- a/xen/include/asm-arm/alternative.h
> +++ b/xen/include/asm-arm/alternative.h
> @@ -26,7 +26,8 @@ struct alt_instr {
>  
>  void __init apply_alternatives_all(void);
>  void apply_alternatives(const struct alt_instr *start,
> -                        const struct alt_instr *end);
> +                        const struct alt_instr *end,
> +                        bool live);
>  
>  #define ALTINSTR_ENTRY(feature)						      \
>  	" .word 661b - .\n"				/* label           */ \
> @@ -160,7 +161,8 @@ static inline void apply_alternatives_all(void)
>  }
>  
>  static inline void apply_alternatives(const struct alt_instr *start,
> -                                      const struct alt_instr *end)
> +                                      const struct alt_instr *end,
> +                                      bool live)
>  {
>  }
>  
> diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
> index ba537d6..07ff424 100644
> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -23,7 +23,8 @@ struct alt_instr {
>  extern void add_nops(void *insns, unsigned int len);
>  /* Similar to alternative_instructions except it can be run with IRQs enabled. */
>  extern void apply_alternatives(const struct alt_instr *start,
> -                               const struct alt_instr *end);
> +                               const struct alt_instr *end,
> +                               bool live);
>  extern void alternative_instructions(void);
>  
>  #define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/5] x86/alternative: Implement NMI/#MC-safe patching

[Jan Beulich]

>>> On 29.01.18 at 16:38, <andrew.cooper3@citrix.com> wrote:
> +bool init_or_livepatch text_poke_live(const struct cpu_user_regs *regs)
> +{
> +    struct live_poke_info *i = &live_poke_info;
> +
> +    if ( unlikely(i->cpu != smp_processor_id()) )
> +    {
> +        ASSERT(regs);
> +
> +        /*
> +         * We hit a breakpoint, on a CPU which was not performing the
> +         * patching.  This is only expected to be possible for the NMI/#MC
> +         * paths, and even then, only if we hit the tiny race window below
> +         * while patching in the NMI/#MC handlers.
> +         *
> +         * We can't safely evaluate whether we hit a transient breakpoint
> +         * because i->cpu has likely completed the patch and moved on to the
> +         * next patch site.
> +         *
> +         * Go to sleep for a bit and synchronise the pipeline as we are now in
> +         * a cross-modifying scenario.
> +         */
> +        cpu_relax();
> +        cpuid_eax(0);
> +
> +        /*
> +         * Presume that we hit the transient breakpoint, as we can't confirm
> +         * whether we did or not.  We don't expect there to be any other
> +         * breakpoints to hit, but if we did hit another one, then in the
> +         * worse case we will only livelock until i->cpu has finished all of
> +         * its patching.
> +         */
> +        return true;
> +    }
> +
> +    /*
> +     * We are the CPU performing the patching, and might have ended up here by
> +     * hitting a breakpoint.
> +     *
> +     * Either way, we need to complete particular patch to make forwards
> +     * progress.  This logic is safe even if executed recursively in the
> +     * breakpoint handler; the worst that will happen when normal execution
> +     * resumes is that we will rewrite the same bytes a second time.
> +     */
> +
> +    /* First, insert a breakpoint to prevent execution of the patch site. */
> +    i->addr[0] = 0xcc;
> +    smp_wmb();

This is necessary, but not sufficient when replacing more than a
single insn: The other CPU may be executing instructions _after_
the initial one that is being replaced, and ...

> +    /* Second, copy the remaining instructions into place. */
> +    memcpy(i->addr + 1, i->opcode + 1, i->len - 1);

... this may be altering things underneath its feet.

> @@ -153,7 +231,31 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
>  void init_or_livepatch noinline
>  text_poke(void *addr, const void *opcode, size_t len, bool live)
>  {
> -    memcpy(addr, opcode, len);
> +    if ( !live || len == 1 )
> +    {
> +        /*
> +         * If we know *addr can't be executed, or we are patching a single
> +         * byte, it is safe to use a straight memcpy().
> +         */
> +        memcpy(addr, opcode, len);

Is it really worth special casing this? Whether to actually ack
patches 2 and 3 depends on that.

> +    }
> +    else
> +    {
> +        /*
> +         * If not, arrange safe patching via the use of breakpoints.  Ordering
> +         * of actions here are between this CPU, and the instruction fetch of
> +         * the breakpoint exception handler on any CPU.
> +         */
> +        live_poke_info = (struct live_poke_info){
> +            addr, opcode, len, smp_processor_id()

Better use C99 field initializers here? At which point (together with
the next comment) it may become better not to use a compound
initializer in the first place.

> +        };
> +        smp_wmb();
> +        active_text_patching = true;
> +        smp_wmb();
> +        text_poke_live(NULL);
> +        smp_wmb();
> +        active_text_patching = false;

Perhaps better to zap live_poke_info.cpu again here? That could
in fact replace the separate active_text_patching flag.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -119,6 +119,8 @@ boolean_param("ler", opt_ler);
>  #define stack_words_per_line 4
>  #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
>  
> +bool active_text_patching;

Why here rather than in alternative.c?

> --- a/xen/arch/x86/x86_64/entry.S
> +++ b/xen/arch/x86/x86_64/entry.S
> @@ -530,7 +530,10 @@ ENTRY(page_fault)
>          movl  $TRAP_page_fault,4(%rsp)
>  /* No special register assumptions. */
>  GLOBAL(handle_exception)
> -        SAVE_ALL CLAC
> +        SAVE_ALL
> +
> +handle_exception_gprs_saved:
> +        ASM_CLAC

I'm not convinced it is a good idea to defer the CLAC here. I see
no problem doing the CLAC below in the INT3 path before jumping
here.

> @@ -686,9 +689,36 @@ ENTRY(debug)
>          jmp   handle_exception
>  
>  ENTRY(int3)
> +        /* For patching-safety, there must not be any alternatives here. */
>          pushq $0
>          movl  $TRAP_int3,4(%rsp)
> -        jmp   handle_exception
> +
> +        /* If there is no patching active, continue normally.  */
> +        cmpb  $1, active_text_patching(%rip)

I think it is better to compare against zero in cases like this. But
then - is this safe? There's no guarantee that the INT3 handling
on a non-patching CPU makes it here before the patching CPU
clears the flag again.

Additionally the comment near anything like this should probably
call out that no CPU can be in guest context at the point of any
patching activity (otherwise you'd have to check the saved
CS.RPL first).

> +        jne   handle_exception
> +
> +        /*
> +         * We hit a debug trap, but not necessarily the one from active
> +         * patching.  Let text_poke_live() work out what to do.
> +         */
> +        SAVE_ALL
> +        mov   %rsp, %rdi
> +        call  text_poke_live
> +
> +        /*
> +         * Does text_poke_live() think we hit the transient debug trap?  If
> +         * not, continue down the normal int3 handler.
> +         */
> +        cmp   $0, %eax

test %al, %al (and then jz below)

> +        je    handle_exception_gprs_saved
> +
> +        /*
> +         * We think we hit the transient debug trap.  text_poke_live() has
> +         * probably completed the patching, so rewind the instruction pointer
> +         * and try again.
> +         */
> +        subq  $1, UREGS_rip(%rsp)
> +        jmp   restore_all_xen

If it was an NMI that was interrupted, you mustn't return using
IRET.

Overall I think your attempt to solve two problems at once here
goes beyond what we need immediately: Live patching in an NMI/
#MC safe way ought to be a second step. The immediate goal
should be to make boot time alternatives patching NMI/#MC safe,
and that can be had with a slimmed down version of this patch
(as at that time only a single CPU is up); most of the not purely
mechanical issues pointed out above are solely related to the
live patching scenario.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/5] arm/alternatives: Fix apply_alternatives() API

[Julien Grall]

Hi,

On 29/01/18 20:23, Stefano Stabellini wrote:
> On Mon, 29 Jan 2018, Andrew Cooper wrote:
>> The !HAS_ALTERNATIVE case has bit-rotten and won't even compile.
>>
>> The x86 side of apply_alternatives() is void, while the ARM side returns int.
>> However, the functions can't fail and no return values are checked.  Switch
>> the ARM side to be void as well.
>>
>> One observation is that __apply_alternatives_multi_stop() is only used at boot
>> time, so can become __init and its static data can become __initdata
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
>>
>> TODO: Drop the !HAS_ALTERNATIVE case on ARM?
> 
> Yeah, we could do. Julien, what do you think?

Sounds good. In that case, we should remove all the reference of 
HAS_ALTERNATIVE in the code.

Cheers,


-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/5] xen/alternatives: Plumb a 'live' parameter through apply_alternatives()

[Julien Grall]

Hi Andrew,

On 29/01/18 15:38, Andrew Cooper wrote:
> On x86, we would like to alter how we patch based on whether there is any
> chance of the code being patched being concurrently executed.
> 
> prepare_payload() passes false (as the livepatch definitely isn't live at this
> point), whereas the boot-time alternatives application passes true.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>   xen/arch/arm/alternative.c        | 10 ++++++----
>   xen/arch/x86/alternative.c        |  5 +++--
>   xen/common/livepatch.c            |  2 +-
>   xen/include/asm-arm/alternative.h |  6 ++++--
>   xen/include/asm-x86/alternative.h |  3 ++-
>   5 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
> index 99112e1..078b259 100644
> --- a/xen/arch/arm/alternative.c
> +++ b/xen/arch/arm/alternative.c
> @@ -98,7 +98,8 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>    * The region patched should be read-write to allow __apply_alternatives
>    * to replacing the instructions when necessary.
>    */
> -static void __apply_alternatives(const struct alt_region *region)
> +static void __apply_alternatives(const struct alt_region *region,
> +                                 bool live)
>   {
>       const struct alt_instr *alt;
>       const u32 *replptr;
> @@ -193,7 +194,7 @@ static int __init __apply_alternatives_multi_stop(void *unused)
>           region.begin = (void *)__alt_instructions - (void *)_start + xenmap;
>           region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;
>   
> -        __apply_alternatives(&region);
> +        __apply_alternatives(&region, true);
>   
>           unregister_virtual_region(&patch_region);
>   
> @@ -224,14 +225,15 @@ void __init apply_alternatives_all(void)
>   }
>   
>   void apply_alternatives(const struct alt_instr *start,
> -                        const struct alt_instr *end)
> +                        const struct alt_instr *end,
> +                        bool live)

This function is not able to deal with "live" code, so I think at least 
need an ASSERT(!live) to prevent mis-usage of the code.

With that:

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH 0.5/5] arm/alternatives: Drop the !HAS_ALTERNATIVE infrastructure

[Andrew Cooper]

ARM now unconditionally selects HAS_ALTERNATIVE, which has caused the
!HAS_ALTERNATIVE code in include/asm-arm/alternative.h to bitrot to the point
of failing to compile.

Expand all the CONFIG_HAS_ALTERNATIVE references in ARM code.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

N.B. Only compile tested
---
 xen/arch/arm/xen.lds.S                    |  2 --
 xen/include/asm-arm/alternative.h         | 15 ---------------
 xen/test/livepatch/xen_hello_world_func.c |  2 +-
 3 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index c9b9546..b039018 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -154,14 +154,12 @@ SECTIONS
        *(.initcall1.init)
        __initcall_end = .;
 
-#ifdef CONFIG_HAS_ALTERNATIVE
        . = ALIGN(4);
        __alt_instructions = .;
        *(.altinstructions)
        __alt_instructions_end = .;
        . = ALIGN(4);
        *(.altinstr_replacement)
-#endif
 
        *(.init.data)
        *(.init.data.rel)
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index 6cc9d0d..4e33d1c 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -3,8 +3,6 @@
 
 #include <asm/cpufeature.h>
 
-#ifdef CONFIG_HAS_ALTERNATIVE
-
 #ifndef __ASSEMBLY__
 
 #include <xen/init.h>
@@ -152,17 +150,4 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
 #define ALTERNATIVE(oldinstr, newinstr, ...)   \
 	_ALTERNATIVE_CFG(oldinstr, newinstr, __VA_ARGS__, 1)
 
-#else /* !CONFIG_HAS_ALTERNATIVE */
-
-static inline void apply_alternatives_all(void)
-{
-}
-
-static inline int apply_alternatives(void *start, size_t length)
-{
-    return 0;
-}
-
-#endif
-
 #endif /* __ASM_ALTERNATIVE_H */
diff --git a/xen/test/livepatch/xen_hello_world_func.c b/xen/test/livepatch/xen_hello_world_func.c
index 1518f71..b358224 100644
--- a/xen/test/livepatch/xen_hello_world_func.c
+++ b/xen/test/livepatch/xen_hello_world_func.c
@@ -29,7 +29,7 @@ const char *xen_hello_world(void)
     rc = __get_user(tmp, non_canonical_addr);
     BUG_ON(rc != -EFAULT);
 #endif
-#if defined(CONFIG_ARM) && defined(CONFIG_HAS_ALTERNATIVE)
+#if defined(CONFIG_ARM)
     asm(ALTERNATIVE("nop", "nop", LIVEPATCH_FEATURE));
 #endif
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/5] xen/alternatives: Plumb a 'live' parameter through apply_alternatives()

[Andrew Cooper]

On 30/01/18 11:05, Julien Grall wrote:
> Hi Andrew,
>
> On 29/01/18 15:38, Andrew Cooper wrote:
>> On x86, we would like to alter how we patch based on whether there is
>> any
>> chance of the code being patched being concurrently executed.
>>
>> prepare_payload() passes false (as the livepatch definitely isn't
>> live at this
>> point), whereas the boot-time alternatives application passes true.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
>> ---
>>   xen/arch/arm/alternative.c        | 10 ++++++----
>>   xen/arch/x86/alternative.c        |  5 +++--
>>   xen/common/livepatch.c            |  2 +-
>>   xen/include/asm-arm/alternative.h |  6 ++++--
>>   xen/include/asm-x86/alternative.h |  3 ++-
>>   5 files changed, 16 insertions(+), 10 deletions(-)
>>
>> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
>> index 99112e1..078b259 100644
>> --- a/xen/arch/arm/alternative.c
>> +++ b/xen/arch/arm/alternative.c
>> @@ -98,7 +98,8 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>>    * The region patched should be read-write to allow
>> __apply_alternatives
>>    * to replacing the instructions when necessary.
>>    */
>> -static void __apply_alternatives(const struct alt_region *region)
>> +static void __apply_alternatives(const struct alt_region *region,
>> +                                 bool live)
>>   {
>>       const struct alt_instr *alt;
>>       const u32 *replptr;
>> @@ -193,7 +194,7 @@ static int __init
>> __apply_alternatives_multi_stop(void *unused)
>>           region.begin = (void *)__alt_instructions - (void *)_start
>> + xenmap;
>>           region.end = (void *)__alt_instructions_end - (void
>> *)_start + xenmap;
>>   -        __apply_alternatives(&region);
>> +        __apply_alternatives(&region, true);
>>             unregister_virtual_region(&patch_region);
>>   @@ -224,14 +225,15 @@ void __init apply_alternatives_all(void)
>>   }
>>     void apply_alternatives(const struct alt_instr *start,
>> -                        const struct alt_instr *end)
>> +                        const struct alt_instr *end,
>> +                        bool live)
>
> This function is not able to deal with "live" code, so I think at
> least need an ASSERT(!live) to prevent mis-usage of the code.

This passes straight through into __apply_alternatives(), just like
__apply_alternatives_multi_stop does, and multi_stop is used on live code.

Either both are unsafe (although all evidence to the contrary), or both
are safe, but I don't think that an assert here is appropriate.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0.5/5] arm/alternatives: Drop the !HAS_ALTERNATIVE infrastructure

[Julien Grall]

Hi Andrew,

Thank you for doing the clean-up.

On 30/01/18 11:16, Andrew Cooper wrote:
> ARM now unconditionally selects HAS_ALTERNATIVE, which has caused the
> !HAS_ALTERNATIVE code in include/asm-arm/alternative.h to bitrot to the point
> of failing to compile.
> 
> Expand all the CONFIG_HAS_ALTERNATIVE references in ARM code.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> N.B. Only compile tested
> ---
>   xen/arch/arm/xen.lds.S                    |  2 --
>   xen/include/asm-arm/alternative.h         | 15 ---------------
>   xen/test/livepatch/xen_hello_world_func.c |  2 +-

You forgot on in include/asm-arm/cpuerrata.h :).

>   3 files changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index c9b9546..b039018 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -154,14 +154,12 @@ SECTIONS
>          *(.initcall1.init)
>          __initcall_end = .;
>   
> -#ifdef CONFIG_HAS_ALTERNATIVE
>          . = ALIGN(4);
>          __alt_instructions = .;
>          *(.altinstructions)
>          __alt_instructions_end = .;
>          . = ALIGN(4);
>          *(.altinstr_replacement)
> -#endif
>   
>          *(.init.data)
>          *(.init.data.rel)
> diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
> index 6cc9d0d..4e33d1c 100644
> --- a/xen/include/asm-arm/alternative.h
> +++ b/xen/include/asm-arm/alternative.h
> @@ -3,8 +3,6 @@
>   
>   #include <asm/cpufeature.h>
>   
> -#ifdef CONFIG_HAS_ALTERNATIVE
> -
>   #ifndef __ASSEMBLY__
>   
>   #include <xen/init.h>
> @@ -152,17 +150,4 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
>   #define ALTERNATIVE(oldinstr, newinstr, ...)   \
>   	_ALTERNATIVE_CFG(oldinstr, newinstr, __VA_ARGS__, 1)
>   
> -#else /* !CONFIG_HAS_ALTERNATIVE */
> -
> -static inline void apply_alternatives_all(void)
> -{
> -}
> -
> -static inline int apply_alternatives(void *start, size_t length)
> -{
> -    return 0;
> -}
> -
> -#endif
> -
>   #endif /* __ASM_ALTERNATIVE_H */
> diff --git a/xen/test/livepatch/xen_hello_world_func.c b/xen/test/livepatch/xen_hello_world_func.c
> index 1518f71..b358224 100644
> --- a/xen/test/livepatch/xen_hello_world_func.c
> +++ b/xen/test/livepatch/xen_hello_world_func.c
> @@ -29,7 +29,7 @@ const char *xen_hello_world(void)
>       rc = __get_user(tmp, non_canonical_addr);
>       BUG_ON(rc != -EFAULT);
>   #endif
> -#if defined(CONFIG_ARM) && defined(CONFIG_HAS_ALTERNATIVE)
> +#if defined(CONFIG_ARM)
>       asm(ALTERNATIVE("nop", "nop", LIVEPATCH_FEATURE));
>   #endif
>   
> 

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0.5/5] arm/alternatives: Drop the !HAS_ALTERNATIVE infrastructure

[Andrew Cooper]

On 30/01/18 11:29, Julien Grall wrote:
> Hi Andrew,
>
> Thank you for doing the clean-up.
>
> On 30/01/18 11:16, Andrew Cooper wrote:
>> ARM now unconditionally selects HAS_ALTERNATIVE, which has caused the
>> !HAS_ALTERNATIVE code in include/asm-arm/alternative.h to bitrot to
>> the point
>> of failing to compile.
>>
>> Expand all the CONFIG_HAS_ALTERNATIVE references in ARM code.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> N.B. Only compile tested
>> ---
>>   xen/arch/arm/xen.lds.S                    |  2 --
>>   xen/include/asm-arm/alternative.h         | 15 ---------------
>>   xen/test/livepatch/xen_hello_world_func.c |  2 +-
>
> You forgot on in include/asm-arm/cpuerrata.h :).

Oops - so I did.  Folded this incremental diff.

~Andrew

diff --git a/xen/include/asm-arm/cpuerrata.h
b/xen/include/asm-arm/cpuerrata.h
index 7de6836..4e45b23 100644
--- a/xen/include/asm-arm/cpuerrata.h
+++ b/xen/include/asm-arm/cpuerrata.h
@@ -7,8 +7,6 @@
 void check_local_cpu_errata(void);
 void enable_errata_workarounds(void);
 
-#ifdef CONFIG_HAS_ALTERNATIVE
-
 #define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
 static inline bool check_workaround_##erratum(void)             \
 {                                                               \
@@ -27,19 +25,6 @@ static inline bool
check_workaround_##erratum(void)             \
     }                                                           \
 }
 
-#else /* CONFIG_HAS_ALTERNATIVE */
-
-#define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
-static inline bool check_workaround_##erratum(void)             \
-{                                                               \
-    if ( !IS_ENABLED(arch) )                                    \
-        return false;                                           \
-    else                                                        \
-        return unlikely(cpus_have_cap(feature));                \
-}
-
-#endif
-
 CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422, CONFIG_ARM_32)
 CHECK_WORKAROUND_HELPER(834220, ARM64_WORKAROUND_834220, CONFIG_ARM_64)
 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 2/5] xen/alternatives: Plumb a 'live' parameter through apply_alternatives()

[Julien Grall]

Hi Andrew,

On 30/01/18 11:24, Andrew Cooper wrote:
> On 30/01/18 11:05, Julien Grall wrote:
>> Hi Andrew,
>>
>> On 29/01/18 15:38, Andrew Cooper wrote:
>>> On x86, we would like to alter how we patch based on whether there is
>>> any
>>> chance of the code being patched being concurrently executed.
>>>
>>> prepare_payload() passes false (as the livepatch definitely isn't
>>> live at this
>>> point), whereas the boot-time alternatives application passes true.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> CC: Ross Lagerwall <ross.lagerwall@citrix.com>
>>> ---
>>>    xen/arch/arm/alternative.c        | 10 ++++++----
>>>    xen/arch/x86/alternative.c        |  5 +++--
>>>    xen/common/livepatch.c            |  2 +-
>>>    xen/include/asm-arm/alternative.h |  6 ++++--
>>>    xen/include/asm-x86/alternative.h |  3 ++-
>>>    5 files changed, 16 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
>>> index 99112e1..078b259 100644
>>> --- a/xen/arch/arm/alternative.c
>>> +++ b/xen/arch/arm/alternative.c
>>> @@ -98,7 +98,8 @@ static u32 get_alt_insn(const struct alt_instr *alt,
>>>     * The region patched should be read-write to allow
>>> __apply_alternatives
>>>     * to replacing the instructions when necessary.
>>>     */
>>> -static void __apply_alternatives(const struct alt_region *region)
>>> +static void __apply_alternatives(const struct alt_region *region,
>>> +                                 bool live)
>>>    {
>>>        const struct alt_instr *alt;
>>>        const u32 *replptr;
>>> @@ -193,7 +194,7 @@ static int __init
>>> __apply_alternatives_multi_stop(void *unused)
>>>            region.begin = (void *)__alt_instructions - (void *)_start
>>> + xenmap;
>>>            region.end = (void *)__alt_instructions_end - (void
>>> *)_start + xenmap;
>>>    -        __apply_alternatives(&region);
>>> +        __apply_alternatives(&region, true);
>>>              unregister_virtual_region(&patch_region);
>>>    @@ -224,14 +225,15 @@ void __init apply_alternatives_all(void)
>>>    }
>>>      void apply_alternatives(const struct alt_instr *start,
>>> -                        const struct alt_instr *end)
>>> +                        const struct alt_instr *end,
>>> +                        bool live)
>>
>> This function is not able to deal with "live" code, so I think at
>> least need an ASSERT(!live) to prevent mis-usage of the code.
> 
> This passes straight through into __apply_alternatives(), just like
> __apply_alternatives_multi_stop does, and multi_stop is used on live code.
> 
> Either both are unsafe (although all evidence to the contrary), or both
> are safe, but I don't think that an assert here is appropriate.

I disagree here. In the commit message you wrote: "On x86, we would like 
to alter how we patch based on whether there is any chance of the code 
being patched being concurrently executed."

I translate this as all the other CPUs may be alive and the code would 
be mapped with read-executable permission (no write permission). It will 
not be easily possible to make the region writable because the processor 
has been configured to forbid it.

__apply_alternatives relies on the region patched to be write accessible 
and the region not executed by any CPUs.

  __apply_alternatives_multi_stop has the logic make the write 
accessible. This is not the case of apply_alternatives.

So the former function is safe while the latter one is unsafe when live 
is true.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/5] x86/alternative: Implement NMI/#MC-safe patching

[Andrew Cooper]

On 30/01/18 08:39, Jan Beulich wrote:
>>>> On 29.01.18 at 16:38, <andrew.cooper3@citrix.com> wrote:
>> +bool init_or_livepatch text_poke_live(const struct cpu_user_regs *regs)
>> +{
>> +    struct live_poke_info *i = &live_poke_info;
>> +
>> +    if ( unlikely(i->cpu != smp_processor_id()) )
>> +    {
>> +        ASSERT(regs);
>> +
>> +        /*
>> +         * We hit a breakpoint, on a CPU which was not performing the
>> +         * patching.  This is only expected to be possible for the NMI/#MC
>> +         * paths, and even then, only if we hit the tiny race window below
>> +         * while patching in the NMI/#MC handlers.
>> +         *
>> +         * We can't safely evaluate whether we hit a transient breakpoint
>> +         * because i->cpu has likely completed the patch and moved on to the
>> +         * next patch site.
>> +         *
>> +         * Go to sleep for a bit and synchronise the pipeline as we are now in
>> +         * a cross-modifying scenario.
>> +         */
>> +        cpu_relax();
>> +        cpuid_eax(0);
>> +
>> +        /*
>> +         * Presume that we hit the transient breakpoint, as we can't confirm
>> +         * whether we did or not.  We don't expect there to be any other
>> +         * breakpoints to hit, but if we did hit another one, then in the
>> +         * worse case we will only livelock until i->cpu has finished all of
>> +         * its patching.
>> +         */
>> +        return true;
>> +    }
>> +
>> +    /*
>> +     * We are the CPU performing the patching, and might have ended up here by
>> +     * hitting a breakpoint.
>> +     *
>> +     * Either way, we need to complete particular patch to make forwards
>> +     * progress.  This logic is safe even if executed recursively in the
>> +     * breakpoint handler; the worst that will happen when normal execution
>> +     * resumes is that we will rewrite the same bytes a second time.
>> +     */
>> +
>> +    /* First, insert a breakpoint to prevent execution of the patch site. */
>> +    i->addr[0] = 0xcc;
>> +    smp_wmb();
> This is necessary, but not sufficient when replacing more than a
> single insn: The other CPU may be executing instructions _after_
> the initial one that is being replaced, and ...
>
>> +    /* Second, copy the remaining instructions into place. */
>> +    memcpy(i->addr + 1, i->opcode + 1, i->len - 1);
> ... this may be altering things underneath its feet.

Hmm.

It is completely impossible to recover if another CPU hits the middle of
this memcpy().  It is impossible to synchronise a specific write against
the remote CPU pipelines.

The only way to be safe is to guarantee that CPUs can't hit the code to
begin with.

On AMD, we can use STGI/CLGI to do this sensibly, and really really
inhibit all interrupts.  For Intel, we can fix the NMI problem by
rendezvousing and running the patching loop in NMI context, at which
point the hardware latch will prevent further NMIs.

However, there is literally nothing we can do to prevent #MC from
arriving.  We can stop servicing #MC by disabling CR4.MCE, but then the
processor will shut down.

We can't put a big barrier at the head of #MC handler which delays
processing, because we have no way to ensure that processors aren't
already later in the #MC handler.  Furthermore, attempting to do so
heightens the risk of hitting a shutdown condition from a second queued #MC.


The chance of hitting an NMI/#MC collide with patching is already
minuscule.  Alternatives and livepatching have been used (by XenServer
alone) in this form for nearly 3 years, across millions of servers,
without a single report of such a collision.  The chance of an #MC
collision is far less likely than an NMI collision, and in the best case.

While we can arrange and test full NMI safety, we cannot test #MC safety
at all.  Any code to try and implement #MC safety is going to be
complicated, and make things worse if an #MC does hit.

Therefore, I agree with the Linux view that trying to do anything for
#MC safety is going to be worse than doing nothing at all.

>> @@ -153,7 +231,31 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
>>  void init_or_livepatch noinline
>>  text_poke(void *addr, const void *opcode, size_t len, bool live)
>>  {
>> -    memcpy(addr, opcode, len);
>> +    if ( !live || len == 1 )
>> +    {
>> +        /*
>> +         * If we know *addr can't be executed, or we are patching a single
>> +         * byte, it is safe to use a straight memcpy().
>> +         */
>> +        memcpy(addr, opcode, len);
> Is it really worth special casing this? Whether to actually ack
> patches 2 and 3 depends on that.

Yes, and even more so if we are going to use an NMI rendezvous.  We
definitely don't want to have an NMI rendezvous while applying
alternatives as part of livepatch preparation.

>
>> +        };
>> +        smp_wmb();
>> +        active_text_patching = true;
>> +        smp_wmb();
>> +        text_poke_live(NULL);
>> +        smp_wmb();
>> +        active_text_patching = false;
> Perhaps better to zap live_poke_info.cpu again here? That could
> in fact replace the separate active_text_patching flag.

No - it cant because...

>> --- a/xen/arch/x86/traps.c
>> +++ b/xen/arch/x86/traps.c
>> @@ -119,6 +119,8 @@ boolean_param("ler", opt_ler);
>>  #define stack_words_per_line 4
>>  #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
>>  
>> +bool active_text_patching;
> Why here rather than in alternative.c?

... in the !LIVEPATCH case, alternative.c is an .init.o and the build
fails because of a nonzero bss.

We cannot have a possibly-init variable referenced in the head of an
exception handler.

>> @@ -686,9 +689,36 @@ ENTRY(debug)
>>          jmp   handle_exception
>>  
>>  ENTRY(int3)
>> +        /* For patching-safety, there must not be any alternatives here. */
>>          pushq $0
>>          movl  $TRAP_int3,4(%rsp)
>> -        jmp   handle_exception
>> +
>> +        /* If there is no patching active, continue normally.  */
>> +        cmpb  $1, active_text_patching(%rip)
> I think it is better to compare against zero in cases like this. But
> then - is this safe? There's no guarantee that the INT3 handling
> on a non-patching CPU makes it here before the patching CPU
> clears the flag again.

Hmm - in the case of an non-patching CPU, we'd need to peek under %rip
to see if the breakpoint had disappeared in the case we find this
boolean clear.  OTOH, the NMI-rendezvous plan would avoid this problem
to begin with.

>
>> +        je    handle_exception_gprs_saved
>> +
>> +        /*
>> +         * We think we hit the transient debug trap.  text_poke_live() has
>> +         * probably completed the patching, so rewind the instruction pointer
>> +         * and try again.
>> +         */
>> +        subq  $1, UREGS_rip(%rsp)
>> +        jmp   restore_all_xen
> If it was an NMI that was interrupted, you mustn't return using
> IRET.

I thought we fixed that, but it appears that the patch never got in.

>
> Overall I think your attempt to solve two problems at once here
> goes beyond what we need immediately: Live patching in an NMI/
> #MC safe way ought to be a second step. The immediate goal
> should be to make boot time alternatives patching NMI/#MC safe,
> and that can be had with a slimmed down version of this patch
> (as at that time only a single CPU is up); most of the not purely
> mechanical issues pointed out above are solely related to the
> live patching scenario.

I could try this, but I don't think the result will be much simpler.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/5] x86/alternative: Implement NMI/#MC-safe patching

[Juergen Gross]

On 30/01/18 20:26, Andrew Cooper wrote:
> However, there is literally nothing we can do to prevent #MC from
> arriving.  We can stop servicing #MC by disabling CR4.MCE, but then the
> processor will shut down.

Hmm, there is a way to avoid #MC on other processors, but this
requires the really big hammer: stop all other cpus and restart
them after patching.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/5] x86/alternative: Implement NMI/#MC-safe patching

[Andrew Cooper]

On 31/01/18 06:07, Juergen Gross wrote:
> On 30/01/18 20:26, Andrew Cooper wrote:
>> However, there is literally nothing we can do to prevent #MC from
>> arriving.  We can stop servicing #MC by disabling CR4.MCE, but then the
>> processor will shut down.
> Hmm, there is a way to avoid #MC on other processors, but this
> requires the really big hammer: stop all other cpus and restart
> them after patching.

I think that firmly goes on the pile of "worse than doing nothing" :)

Also, it doesn't solve the problem of #MC arriving on the current processor.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/5] x86/alternative: Implement NMI/#MC-safe patching

[Jan Beulich]

>>> On 30.01.18 at 20:26, <andrew.cooper3@citrix.com> wrote:
> On 30/01/18 08:39, Jan Beulich wrote:
>>>>> On 29.01.18 at 16:38, <andrew.cooper3@citrix.com> wrote:
>>> +    /*
>>> +     * We are the CPU performing the patching, and might have ended up here by
>>> +     * hitting a breakpoint.
>>> +     *
>>> +     * Either way, we need to complete particular patch to make forwards
>>> +     * progress.  This logic is safe even if executed recursively in the
>>> +     * breakpoint handler; the worst that will happen when normal execution
>>> +     * resumes is that we will rewrite the same bytes a second time.
>>> +     */
>>> +
>>> +    /* First, insert a breakpoint to prevent execution of the patch site. */
>>> +    i->addr[0] = 0xcc;
>>> +    smp_wmb();
>> This is necessary, but not sufficient when replacing more than a
>> single insn: The other CPU may be executing instructions _after_
>> the initial one that is being replaced, and ...
>>
>>> +    /* Second, copy the remaining instructions into place. */
>>> +    memcpy(i->addr + 1, i->opcode + 1, i->len - 1);
>> ... this may be altering things underneath its feet.
> 
> Hmm.
> 
> It is completely impossible to recover if another CPU hits the middle of
> this memcpy().  It is impossible to synchronise a specific write against
> the remote CPU pipelines.

It is not completely impossible, but the solution would be awkward
to use: If the code doing the patching knew instruction boundaries,
it could put breakpoints onto all of them. Or maybe it isn't all that
bad to use - we have an insn decoder after all.

> The only way to be safe is to guarantee that CPUs can't hit the code to
> begin with.
> 
> On AMD, we can use STGI/CLGI to do this sensibly, and really really
> inhibit all interrupts.

Not really for #MC - clear GIF may also result in shutdown when
one would need delivering.

>  For Intel, we can fix the NMI problem by
> rendezvousing and running the patching loop in NMI context, at which
> point the hardware latch will prevent further NMIs.
> 
> However, there is literally nothing we can do to prevent #MC from
> arriving.  We can stop servicing #MC by disabling CR4.MCE, but then the
> processor will shut down.

Not a good idea indeed.

> We can't put a big barrier at the head of #MC handler which delays
> processing, because we have no way to ensure that processors aren't
> already later in the #MC handler.  Furthermore, attempting to do so
> heightens the risk of hitting a shutdown condition from a second queued #MC.
> 
> 
> The chance of hitting an NMI/#MC collide with patching is already
> minuscule.  Alternatives and livepatching have been used (by XenServer
> alone) in this form for nearly 3 years, across millions of servers,
> without a single report of such a collision.  The chance of an #MC
> collision is far less likely than an NMI collision, and in the best case.
> 
> While we can arrange and test full NMI safety, we cannot test #MC safety
> at all.  Any code to try and implement #MC safety is going to be
> complicated, and make things worse if an #MC does hit.
> 
> Therefore, I agree with the Linux view that trying to do anything for
> #MC safety is going to be worse than doing nothing at all.

I agree. But as said before - the immediate goal ought to be to
make alternatives patching safe, and that doesn't require any
of these considerations.

>>> @@ -153,7 +231,31 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
>>>  void init_or_livepatch noinline
>>>  text_poke(void *addr, const void *opcode, size_t len, bool live)
>>>  {
>>> -    memcpy(addr, opcode, len);
>>> +    if ( !live || len == 1 )
>>> +    {
>>> +        /*
>>> +         * If we know *addr can't be executed, or we are patching a single
>>> +         * byte, it is safe to use a straight memcpy().
>>> +         */
>>> +        memcpy(addr, opcode, len);
>> Is it really worth special casing this? Whether to actually ack
>> patches 2 and 3 depends on that.
> 
> Yes, and even more so if we are going to use an NMI rendezvous.  We
> definitely don't want to have an NMI rendezvous while applying
> alternatives as part of livepatch preparation.

Well, again - live patching should be the second step here.

>>> +        };
>>> +        smp_wmb();
>>> +        active_text_patching = true;
>>> +        smp_wmb();
>>> +        text_poke_live(NULL);
>>> +        smp_wmb();
>>> +        active_text_patching = false;
>> Perhaps better to zap live_poke_info.cpu again here? That could
>> in fact replace the separate active_text_patching flag.
> 
> No - it cant because...
> 
>>> --- a/xen/arch/x86/traps.c
>>> +++ b/xen/arch/x86/traps.c
>>> @@ -119,6 +119,8 @@ boolean_param("ler", opt_ler);
>>>  #define stack_words_per_line 4
>>>  #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
>>>  
>>> +bool active_text_patching;
>> Why here rather than in alternative.c?
> 
> ... in the !LIVEPATCH case, alternative.c is an .init.o and the build
> fails because of a nonzero bss.
> 
> We cannot have a possibly-init variable referenced in the head of an
> exception handler.

I'd like to revisit this once the live patching aspect was split off
of the more immediate alternatives patching part.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0.5/5] arm/alternatives: Drop the !HAS_ALTERNATIVE infrastructure

[Julien Grall]

Hi,

On 30/01/18 11:31, Andrew Cooper wrote:
> On 30/01/18 11:29, Julien Grall wrote:
>> Hi Andrew,
>>
>> Thank you for doing the clean-up.
>>
>> On 30/01/18 11:16, Andrew Cooper wrote:
>>> ARM now unconditionally selects HAS_ALTERNATIVE, which has caused the
>>> !HAS_ALTERNATIVE code in include/asm-arm/alternative.h to bitrot to
>>> the point
>>> of failing to compile.
>>>
>>> Expand all the CONFIG_HAS_ALTERNATIVE references in ARM code.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>>
>>> N.B. Only compile tested
>>> ---
>>>    xen/arch/arm/xen.lds.S                    |  2 --
>>>    xen/include/asm-arm/alternative.h         | 15 ---------------
>>>    xen/test/livepatch/xen_hello_world_func.c |  2 +-
>>
>> You forgot on in include/asm-arm/cpuerrata.h :).
> 
> Oops - so I did.  Folded this incremental diff.

With that folded:

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

> 
> ~Andrew
> 
> diff --git a/xen/include/asm-arm/cpuerrata.h
> b/xen/include/asm-arm/cpuerrata.h
> index 7de6836..4e45b23 100644
> --- a/xen/include/asm-arm/cpuerrata.h
> +++ b/xen/include/asm-arm/cpuerrata.h
> @@ -7,8 +7,6 @@
>   void check_local_cpu_errata(void);
>   void enable_errata_workarounds(void);
>   
> -#ifdef CONFIG_HAS_ALTERNATIVE
> -
>   #define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
>   static inline bool check_workaround_##erratum(void)             \
>   {                                                               \
> @@ -27,19 +25,6 @@ static inline bool
> check_workaround_##erratum(void)             \
>       }                                                           \
>   }
>   
> -#else /* CONFIG_HAS_ALTERNATIVE */
> -
> -#define CHECK_WORKAROUND_HELPER(erratum, feature, arch)         \
> -static inline bool check_workaround_##erratum(void)             \
> -{                                                               \
> -    if ( !IS_ENABLED(arch) )                                    \
> -        return false;                                           \
> -    else                                                        \
> -        return unlikely(cpus_have_cap(feature));                \
> -}
> -
> -#endif
> -
>   CHECK_WORKAROUND_HELPER(766422, ARM32_WORKAROUND_766422, CONFIG_ARM_32)
>   CHECK_WORKAROUND_HELPER(834220, ARM64_WORKAROUND_834220, CONFIG_ARM_64)
>   
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 0.5/5] arm/alternatives: Drop the !HAS_ALTERNATIVE infrastructure

[Konrad Rzeszutek Wilk]

On Tue, Jan 30, 2018 at 11:31:27AM +0000, Andrew Cooper wrote:
> On 30/01/18 11:29, Julien Grall wrote:
> > Hi Andrew,
> >
> > Thank you for doing the clean-up.
> >
> > On 30/01/18 11:16, Andrew Cooper wrote:
> >> ARM now unconditionally selects HAS_ALTERNATIVE, which has caused the
> >> !HAS_ALTERNATIVE code in include/asm-arm/alternative.h to bitrot to
> >> the point
> >> of failing to compile.
> >>
> >> Expand all the CONFIG_HAS_ALTERNATIVE references in ARM code.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Stefano Stabellini <sstabellini@kernel.org>
> >> CC: Julien Grall <julien.grall@arm.com>
> >> CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >>
> >> N.B. Only compile tested
> >> ---
> >>   xen/arch/arm/xen.lds.S                    |  2 --
> >>   xen/include/asm-arm/alternative.h         | 15 ---------------
> >>   xen/test/livepatch/xen_hello_world_func.c |  2 +-
> >
> > You forgot on in include/asm-arm/cpuerrata.h :).
> 
> Oops - so I did.  Folded this incremental diff.
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/5] x86/alternative: Implement NMI/#MC-safe patching

[Andrew Cooper]

On 31/01/18 11:00, Jan Beulich wrote:
>>>> On 30.01.18 at 20:26, <andrew.cooper3@citrix.com> wrote:
>> On 30/01/18 08:39, Jan Beulich wrote:
>>>>>> On 29.01.18 at 16:38, <andrew.cooper3@citrix.com> wrote:
>>>> +    /*
>>>> +     * We are the CPU performing the patching, and might have ended up here by
>>>> +     * hitting a breakpoint.
>>>> +     *
>>>> +     * Either way, we need to complete particular patch to make forwards
>>>> +     * progress.  This logic is safe even if executed recursively in the
>>>> +     * breakpoint handler; the worst that will happen when normal execution
>>>> +     * resumes is that we will rewrite the same bytes a second time.
>>>> +     */
>>>> +
>>>> +    /* First, insert a breakpoint to prevent execution of the patch site. */
>>>> +    i->addr[0] = 0xcc;
>>>> +    smp_wmb();
>>> This is necessary, but not sufficient when replacing more than a
>>> single insn: The other CPU may be executing instructions _after_
>>> the initial one that is being replaced, and ...
>>>
>>>> +    /* Second, copy the remaining instructions into place. */
>>>> +    memcpy(i->addr + 1, i->opcode + 1, i->len - 1);
>>> ... this may be altering things underneath its feet.
>> Hmm.
>>
>> It is completely impossible to recover if another CPU hits the middle of
>> this memcpy().  It is impossible to synchronise a specific write against
>> the remote CPU pipelines.
> It is not completely impossible, but the solution would be awkward
> to use: If the code doing the patching knew instruction boundaries,
> it could put breakpoints onto all of them. Or maybe it isn't all that
> bad to use - we have an insn decoder after all.

An instruction decoder doesn't help.  Yes - it allows us to avoid
executing a spliced instruction, but we can't recover/rewind state for a
cpu which hits one of these latter breakpoints.

>
>> The only way to be safe is to guarantee that CPUs can't hit the code to
>> begin with.
>>
>> On AMD, we can use STGI/CLGI to do this sensibly, and really really
>> inhibit all interrupts.
> Not really for #MC - clear GIF may also result in shutdown when
> one would need delivering.

With GIF clear, #MC is held pending if possible, but otherwise a
shutdown will occur.

From that point of view, it is slightly better than clearing CR4.MCE,
but not by much.

>
>>  For Intel, we can fix the NMI problem by
>> rendezvousing and running the patching loop in NMI context, at which
>> point the hardware latch will prevent further NMIs.
>>
>> However, there is literally nothing we can do to prevent #MC from
>> arriving.  We can stop servicing #MC by disabling CR4.MCE, but then the
>> processor will shut down.
> Not a good idea indeed.
>
>> We can't put a big barrier at the head of #MC handler which delays
>> processing, because we have no way to ensure that processors aren't
>> already later in the #MC handler.  Furthermore, attempting to do so
>> heightens the risk of hitting a shutdown condition from a second queued #MC.
>>
>>
>> The chance of hitting an NMI/#MC collide with patching is already
>> minuscule.  Alternatives and livepatching have been used (by XenServer
>> alone) in this form for nearly 3 years, across millions of servers,
>> without a single report of such a collision.  The chance of an #MC
>> collision is far less likely than an NMI collision, and in the best case.
>>
>> While we can arrange and test full NMI safety, we cannot test #MC safety
>> at all.  Any code to try and implement #MC safety is going to be
>> complicated, and make things worse if an #MC does hit.
>>
>> Therefore, I agree with the Linux view that trying to do anything for
>> #MC safety is going to be worse than doing nothing at all.
> I agree. But as said before - the immediate goal ought to be to
> make alternatives patching safe, and that doesn't require any
> of these considerations.

? Of course it requires these considerations.

If we ignore the livepatching side of things (which includes an
alternatives pass), then we can make the boot-time alternatives pass NMI
safe by explicitly choosing to patch in NMI context before we've booted
the APs.

Arranging for boot time alternatives to be NMI-safe is easy.  Arranging
for livepatching to be NMI-safe is also fairly easy.

Arranging for anything, *even at boot time* to be #MC safe, is very
complicated, and will be a large amount of code we cannot test.  Hence
why I'm leaning towards the Linux solution for the #MC problem.

>
>>>> @@ -153,7 +231,31 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
>>>>  void init_or_livepatch noinline
>>>>  text_poke(void *addr, const void *opcode, size_t len, bool live)
>>>>  {
>>>> -    memcpy(addr, opcode, len);
>>>> +    if ( !live || len == 1 )
>>>> +    {
>>>> +        /*
>>>> +         * If we know *addr can't be executed, or we are patching a single
>>>> +         * byte, it is safe to use a straight memcpy().
>>>> +         */
>>>> +        memcpy(addr, opcode, len);
>>> Is it really worth special casing this? Whether to actually ack
>>> patches 2 and 3 depends on that.
>> Yes, and even more so if we are going to use an NMI rendezvous.  We
>> definitely don't want to have an NMI rendezvous while applying
>> alternatives as part of livepatch preparation.
> Well, again - live patching should be the second step here.

You keep on saying this, and I can only conclude that you are confused
as to which actions happen when.

At boot time, we do a single alternatives pass on the live .text section.

At livepatch load time, we do an alternatives pass on the incoming
.text, but this is safe because the text definitely isn't being executed.

At livepatch apply time, we quiesce the system and do a patching pass on
the live .text, most of which are `jmp disp32`.


We therefore have two alternatives passes (one safe, one unsafe), and
one (unsafe) livepatch pass.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/5] x86/alternative: Implement NMI/#MC-safe patching

[Jan Beulich]

>>> On 01.02.18 at 16:20, <andrew.cooper3@citrix.com> wrote:
> On 31/01/18 11:00, Jan Beulich wrote:
>>>>> On 30.01.18 at 20:26, <andrew.cooper3@citrix.com> wrote:
>>> On 30/01/18 08:39, Jan Beulich wrote:
>>>>>>> On 29.01.18 at 16:38, <andrew.cooper3@citrix.com> wrote:
>>>>> +    /*
>>>>> +     * We are the CPU performing the patching, and might have ended up here by
>>>>> +     * hitting a breakpoint.
>>>>> +     *
>>>>> +     * Either way, we need to complete particular patch to make forwards
>>>>> +     * progress.  This logic is safe even if executed recursively in the
>>>>> +     * breakpoint handler; the worst that will happen when normal execution
>>>>> +     * resumes is that we will rewrite the same bytes a second time.
>>>>> +     */
>>>>> +
>>>>> +    /* First, insert a breakpoint to prevent execution of the patch site. */
>>>>> +    i->addr[0] = 0xcc;
>>>>> +    smp_wmb();
>>>> This is necessary, but not sufficient when replacing more than a
>>>> single insn: The other CPU may be executing instructions _after_
>>>> the initial one that is being replaced, and ...
>>>>
>>>>> +    /* Second, copy the remaining instructions into place. */
>>>>> +    memcpy(i->addr + 1, i->opcode + 1, i->len - 1);
>>>> ... this may be altering things underneath its feet.
>>> Hmm.
>>>
>>> It is completely impossible to recover if another CPU hits the middle of
>>> this memcpy().  It is impossible to synchronise a specific write against
>>> the remote CPU pipelines.
>> It is not completely impossible, but the solution would be awkward
>> to use: If the code doing the patching knew instruction boundaries,
>> it could put breakpoints onto all of them. Or maybe it isn't all that
>> bad to use - we have an insn decoder after all.
> 
> An instruction decoder doesn't help.  Yes - it allows us to avoid
> executing a spliced instruction, but we can't recover/rewind state for a
> cpu which hits one of these latter breakpoints.

Oh, true. At least doing so would be very difficult.

>>> While we can arrange and test full NMI safety, we cannot test #MC safety
>>> at all.  Any code to try and implement #MC safety is going to be
>>> complicated, and make things worse if an #MC does hit.
>>>
>>> Therefore, I agree with the Linux view that trying to do anything for
>>> #MC safety is going to be worse than doing nothing at all.
>> I agree. But as said before - the immediate goal ought to be to
>> make alternatives patching safe, and that doesn't require any
>> of these considerations.
> 
> ? Of course it requires these considerations.

I don't understand, but see below.

> If we ignore the livepatching side of things (which includes an
> alternatives pass), then we can make the boot-time alternatives pass NMI
> safe by explicitly choosing to patch in NMI context before we've booted
> the APs.
> 
> Arranging for boot time alternatives to be NMI-safe is easy.  Arranging
> for livepatching to be NMI-safe is also fairly easy.
> 
> Arranging for anything, *even at boot time* to be #MC safe, is very
> complicated, and will be a large amount of code we cannot test.  Hence
> why I'm leaning towards the Linux solution for the #MC problem.

I could live with this.

>>>>> @@ -153,7 +231,31 @@ void init_or_livepatch add_nops(void *insns, unsigned int len)
>>>>>  void init_or_livepatch noinline
>>>>>  text_poke(void *addr, const void *opcode, size_t len, bool live)
>>>>>  {
>>>>> -    memcpy(addr, opcode, len);
>>>>> +    if ( !live || len == 1 )
>>>>> +    {
>>>>> +        /*
>>>>> +         * If we know *addr can't be executed, or we are patching a single
>>>>> +         * byte, it is safe to use a straight memcpy().
>>>>> +         */
>>>>> +        memcpy(addr, opcode, len);
>>>> Is it really worth special casing this? Whether to actually ack
>>>> patches 2 and 3 depends on that.
>>> Yes, and even more so if we are going to use an NMI rendezvous.  We
>>> definitely don't want to have an NMI rendezvous while applying
>>> alternatives as part of livepatch preparation.
>> Well, again - live patching should be the second step here.
> 
> You keep on saying this, and I can only conclude that you are confused
> as to which actions happen when.
> 
> At boot time, we do a single alternatives pass on the live .text section.
> 
> At livepatch load time, we do an alternatives pass on the incoming
> .text, but this is safe because the text definitely isn't being executed.
> 
> At livepatch apply time, we quiesce the system and do a patching pass on
> the live .text, most of which are `jmp disp32`.
> 
> 
> We therefore have two alternatives passes (one safe, one unsafe), and
> one (unsafe) livepatch pass.

I don't think I'm confused in any way. The approach you've chosen
in the patch - to complete the patching inside the NMI or #MC
handler - is entirely sufficient to deal with NMI and - unless there
are recursive ones - #MC. You don't need NMI context to do NMI
safe patching when only a single CPU is up.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel