[v6,16/24] xsplice: Add support for alternatives

Commit Message

Konrad Rzeszutek Wilk April 7, 2016, 3:49 a.m. UTC

From: Ross Lagerwall <[e-mail]>

Add support for applying alternative sections within xsplice payload.
At payload load time, apply an alternative sections that are found.

Also we add an test-case exercising a rather useless alternative
(patching a NOP with a NOP) - but it does exercise the code-path.

Signed-off-by: Ross Lagerwall <[e-mail]>
Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>

---
Cc: Keir Fraser <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Andrew Cooper <[e-mail]>

v2: Make a new alternative function that does not ASSERT on IRQs and
    don't disable IRQs in the code when loading payload.
v4: Include test-case
    Include check for size of alternatives and that it is not a 0 size
    section.
v6: Add #define INIT to preserve __initness on alternative code.
    Double check that alt_instr are only patching payload code.
---
---
 xen/arch/x86/Makefile                    |  2 +-
 xen/arch/x86/alternative.c               | 35 ++++++++++++++++++++++----------
 xen/arch/x86/test/xen_hello_world_func.c |  5 +++++
 xen/common/xsplice.c                     | 34 +++++++++++++++++++++++++++++++
 xen/include/asm-x86/alternative.h        |  4 ++++
 5 files changed, 68 insertions(+), 12 deletions(-)

Comments

Andrew Cooper April 8, 2016, 5:34 p.m. UTC | #1

On 07/04/16 04:49, Konrad Rzeszutek Wilk wrote:
> From: Ross Lagerwall <[e-mail]>
>
> Add support for applying alternative sections within xsplice payload.
> At payload load time, apply an alternative sections that are found.
>
> Also we add an test-case exercising a rather useless alternative
> (patching a NOP with a NOP) - but it does exercise the code-path.
>
> Signed-off-by: Ross Lagerwall <[e-mail]>
> Signed-off-by: Konrad Rzeszutek Wilk <[e-mail]>
>
> ---
> Cc: Keir Fraser <[e-mail]>
> Cc: Jan Beulich <[e-mail]>
> Cc: Andrew Cooper <[e-mail]>
>
> v2: Make a new alternative function that does not ASSERT on IRQs and
>     don't disable IRQs in the code when loading payload.
> v4: Include test-case
>     Include check for size of alternatives and that it is not a 0 size
>     section.
> v6: Add #define INIT to preserve __initness on alternative code.
>     Double check that alt_instr are only patching payload code.
> ---
> ---
>  xen/arch/x86/Makefile                    |  2 +-
>  xen/arch/x86/alternative.c               | 35 ++++++++++++++++++++++----------
>  xen/arch/x86/test/xen_hello_world_func.c |  5 +++++
>  xen/common/xsplice.c                     | 34 +++++++++++++++++++++++++++++++
>  xen/include/asm-x86/alternative.h        |  4 ++++
>  5 files changed, 68 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index d85287d..08a7b68 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -6,7 +6,7 @@ subdir-y += mm
>  subdir-$(CONFIG_XENOPROF) += oprofile
>  subdir-y += x86_64
>  
> -obj-bin-y += alternative.init.o
> +obj-bin-y += alternative.o

You want to keep this as alternative.init.o if not CONFIG_XSPLICE

> @@ -142,15 +144,13 @@ static void *__init text_poke_early(void *addr, const void *opcode, size_t len)
>   * APs have less capabilities than the boot processor are not handled.
>   * Tough. Make sure you disable such features by hand.
>   */
> -static void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
> +void __INIT apply_alternatives_nocheck(struct alt_instr *start, struct alt_instr *end)
>  {
>      struct alt_instr *a;
>      u8 *instr, *replacement;
>      u8 insnbuf[MAX_PATCH_LEN];
>      unsigned long cr0 = read_cr0();

The cr0 adjustment of WP should also move from the nocheck() to the
plain variant.  This avoid temporisingly disabling WP on the CPU setting
up the patch, before the payload is "secured".

>  
> -    ASSERT(!local_irq_is_enabled());
> -
>      printk(KERN_INFO "alt table %p -> %p\n", start, end);
>  
>      /* Disable WP to allow application of alternatives to read-only pages. */
> @@ -183,13 +183,26 @@ static void __init apply_alternatives(struct alt_instr *start, struct alt_instr
>  
>          add_nops(insnbuf + a->replacementlen,
>                   a->instrlen - a->replacementlen);
> -        text_poke_early(instr, insnbuf, a->instrlen);
> +        text_poke(instr, insnbuf, a->instrlen);
>      }
>  
>      /* Reinstate WP. */
>      write_cr0(cr0);
>  }
>  
> +#undef __INIT
> +#undef __INITCONST
> +#undef __INITDATA

Why these undefs?

> +/*
> + * This routine is called with local interrupt disabled and used during
> + * bootup.
> + */
> +void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
> +{
> +    ASSERT(!local_irq_is_enabled());
> +    apply_alternatives_nocheck(start, end);
> +}
> +
>  void __init alternative_instructions(void)
>  {
>      nmi_callback_t *saved_nmi_callback;
> diff --git a/xen/arch/x86/test/xen_hello_world_func.c b/xen/arch/x86/test/xen_hello_world_func.c
> index 7e239ca..55e84ac 100644
> --- a/xen/arch/x86/test/xen_hello_world_func.c
> +++ b/xen/arch/x86/test/xen_hello_world_func.c
> @@ -3,6 +3,9 @@
>   *
>   */
>  
> +#include <asm/alternative.h>
> +#include <asm/nops.h>
> +#include <asm/uaccess.h>
>  #include <xen/types.h>
>  
>  static unsigned long *non_canonical_addr = (unsigned long *)(1UL<<48);
> @@ -12,6 +15,8 @@ const char *xen_hello_world(void)
>  {
>      unsigned long tmp = 0xdeadbeef;
>      int rc;
> +
> +    alternative(ASM_NOP1, ASM_NOP1, X86_FEATURE_NX);

NX is very definitely not always available.  Use LM instead, and leave a
comment saying that LM will always be available on Xen.

With these fixed, Reviewed-by: Andrew Cooper <[e-mail]>

Jan Beulich April 8, 2016, 5:57 p.m. UTC | #2

>>> On 08.04.16 at 19:34, <[e-mail]> wrote:
> On 07/04/16 04:49, Konrad Rzeszutek Wilk wrote:
>> --- a/xen/arch/x86/Makefile
>> +++ b/xen/arch/x86/Makefile
>> @@ -6,7 +6,7 @@ subdir-y += mm
>>  subdir-$(CONFIG_XENOPROF) += oprofile
>>  subdir-y += x86_64
>>  
>> -obj-bin-y += alternative.init.o
>> +obj-bin-y += alternative.o
> 
> You want to keep this as alternative.init.o if not CONFIG_XSPLICE

And add to obj-y instead of obj-bin-y when CONFIG_XSPLICE.

Jan

Patch

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index d85287d..08a7b68 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -6,7 +6,7 @@  subdir-y += mm
 subdir-$(CONFIG_XENOPROF) += oprofile
 subdir-y += x86_64
 
-obj-bin-y += alternative.init.o
+obj-bin-y += alternative.o
 obj-y += apic.o
 obj-y += bitops.o
 obj-bin-y += bzimage.init.o
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index f735ff8..996e312 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -22,13 +22,14 @@ 
 #include <asm/system.h>
 #include <asm/traps.h>
 #include <asm/nmi.h>
+#include <xen/xsplice.h>
 
 #define MAX_PATCH_LEN (255-1)
 
 extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
 
 #ifdef K8_NOP1
-static const unsigned char k8nops[] __initconst = {
+static const unsigned char k8nops[] __INITCONST = {
     K8_NOP1,
     K8_NOP2,
     K8_NOP3,
@@ -52,7 +53,7 @@  static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconstrel = {
 #endif
 
 #ifdef P6_NOP1
-static const unsigned char p6nops[] __initconst = {
+static const unsigned char p6nops[] __INITCONST = {
     P6_NOP1,
     P6_NOP2,
     P6_NOP3,
@@ -75,7 +76,7 @@  static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconstrel = {
 };
 #endif
 
-static const unsigned char * const *ideal_nops __initdata = k8_nops;
+static const unsigned char * const *ideal_nops __INITDATA = k8_nops;
 
 static int __init mask_nmi_callback(const struct cpu_user_regs *regs, int cpu)
 {
@@ -100,7 +101,7 @@  static void __init arch_init_ideal_nops(void)
 }
 
 /* Use this to add nops to a buffer, then text_poke the whole buffer. */
-static void __init add_nops(void *insns, unsigned int len)
+static void __INIT add_nops(void *insns, unsigned int len)
 {
     while ( len > 0 )
     {
@@ -114,7 +115,7 @@  static void __init add_nops(void *insns, unsigned int len)
 }
 
 /*
- * text_poke_early - Update instructions on a live kernel at boot time
+ * text_poke - Update instructions on a live kernel or non-executed code.
  * @addr: address to modify
  * @opcode: source of the copy
  * @len: length to copy
@@ -125,9 +126,10 @@  static void __init add_nops(void *insns, unsigned int len)
  * instructions. And on the local CPU you need to be protected again NMI or MCE
  * handlers seeing an inconsistent instruction while you patch.
  *
- * This routine is called with local interrupt disabled.
+ * You should run this with interrupts disabled or on code that has never
+ * been executed.
  */
-static void *__init text_poke_early(void *addr, const void *opcode, size_t len)
+static void *__INIT text_poke(void *addr, const void *opcode, size_t len)
 {
     memcpy(addr, opcode, len);
     sync_core();
@@ -142,15 +144,13 @@  static void *__init text_poke_early(void *addr, const void *opcode, size_t len)
  * APs have less capabilities than the boot processor are not handled.
  * Tough. Make sure you disable such features by hand.
  */
-static void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
+void __INIT apply_alternatives_nocheck(struct alt_instr *start, struct alt_instr *end)
 {
     struct alt_instr *a;
     u8 *instr, *replacement;
     u8 insnbuf[MAX_PATCH_LEN];
     unsigned long cr0 = read_cr0();
 
-    ASSERT(!local_irq_is_enabled());
-
     printk(KERN_INFO "alt table %p -> %p\n", start, end);
 
     /* Disable WP to allow application of alternatives to read-only pages. */
@@ -183,13 +183,26 @@  static void __init apply_alternatives(struct alt_instr *start, struct alt_instr
 
         add_nops(insnbuf + a->replacementlen,
                  a->instrlen - a->replacementlen);
-        text_poke_early(instr, insnbuf, a->instrlen);
+        text_poke(instr, insnbuf, a->instrlen);
     }
 
     /* Reinstate WP. */
     write_cr0(cr0);
 }
 
+#undef __INIT
+#undef __INITCONST
+#undef __INITDATA
+/*
+ * This routine is called with local interrupt disabled and used during
+ * bootup.
+ */
+void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
+{
+    ASSERT(!local_irq_is_enabled());
+    apply_alternatives_nocheck(start, end);
+}
+
 void __init alternative_instructions(void)
 {
     nmi_callback_t *saved_nmi_callback;
diff --git a/xen/arch/x86/test/xen_hello_world_func.c b/xen/arch/x86/test/xen_hello_world_func.c
index 7e239ca..55e84ac 100644
--- a/xen/arch/x86/test/xen_hello_world_func.c
+++ b/xen/arch/x86/test/xen_hello_world_func.c
@@ -3,6 +3,9 @@ 
  *
  */
 
+#include <asm/alternative.h>
+#include <asm/nops.h>
+#include <asm/uaccess.h>
 #include <xen/types.h>
 
 static unsigned long *non_canonical_addr = (unsigned long *)(1UL<<48);
@@ -12,6 +15,8 @@  const char *xen_hello_world(void)
 {
     unsigned long tmp = 0xdeadbeef;
     int rc;
+
+    alternative(ASM_NOP1, ASM_NOP1, X86_FEATURE_NX);
     /*
      * Any BUG, or WARN_ON will contain symbol and payload name. Furthermore
      * exceptions will be caught and processed properly.
diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 31ddd5d..fea184c 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -526,6 +526,39 @@  static int prepare_payload(struct payload *payload,
     }
 
 #ifdef CONFIG_X86
+    sec = xsplice_elf_sec_by_name(elf, ".altinstructions");
+    if ( sec )
+    {
+        struct alt_instr *a, *start, *end;
+
+        if ( !sec->sec->sh_size ||
+             (sec->sec->sh_size % sizeof(*a)) )
+        {
+            dprintk(XENLOG_DEBUG, XSPLICE "%s: Wrong size of .alt_instr (exp:%lu vs %lu)!\n",
+                    elf->name, sizeof(*a),
+                    sec->sec->sh_size);
+            return -EINVAL;
+        }
+
+        start = (struct alt_instr *)sec->load_addr;
+        end = (struct alt_instr *)(sec->load_addr + sec->sec->sh_size);
+
+        for ( a = start; a < end; a++ )
+        {
+            unsigned long instr = (unsigned long)(&a->instr_offset + a->instr_offset);
+            unsigned long replacement = (unsigned long)(&a->repl_offset + a->repl_offset);
+
+            if ( (instr < region->start && instr >= region->end) ||
+                 (replacement < region->start && replacement >= region->end) )
+            {
+                dprintk(XENLOG_DEBUG, XSPLICE "%s Alt patching outside payload: 0x%lx!\n",
+                        elf->name, instr);
+                return -EINVAL;
+            }
+        }
+        apply_alternatives_nocheck(start, end);
+    }
+
     sec = xsplice_elf_sec_by_name(elf, ".ex_table");
     if ( sec )
     {
@@ -548,6 +581,7 @@  static int prepare_payload(struct payload *payload,
         region->ex = (const struct exception_table_entry *)s;
         region->ex_end = (const struct exception_table_entry *)e;
     }
+
 #endif
 
     return 0;
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index 1056630..bce959f 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -23,6 +23,10 @@  struct alt_instr {
     u8  replacementlen;     /* length of new instruction, <= instrlen */
 };
 
+/* Similar to apply_alternatives except it can be run with IRQs enabled. */
+extern void apply_alternatives_nocheck(struct alt_instr *start,
+                                       struct alt_instr *end);
+extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
 extern void alternative_instructions(void);
 
 #define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"