[v5,18/28] xsplice: Add support for alternatives

Commit Message

Konrad Rzeszutek Wilk March 24, 2016, 8 p.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.
---
 xen/arch/x86/Makefile                    |  2 +-
 xen/arch/x86/alternative.c               | 20 ++++++++++++--------
 xen/arch/x86/test/xen_hello_world_func.c |  3 +++
 xen/common/xsplice.c                     | 16 ++++++++++++++++
 xen/include/asm-x86/alternative.h        |  6 ++++++
 5 files changed, 38 insertions(+), 9 deletions(-)

Comments

Jan Beulich April 1, 2016, 4:20 p.m. UTC | #1

>>> On 24.03.16 at 21:00, <[e-mail]> wrote:
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -28,7 +28,7 @@
>  extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
>  
>  #ifdef K8_NOP1
> -static const unsigned char k8nops[] __initconst = {
> +static const unsigned char k8nops[] = {

Just like in Linux these init annotations should become conditional
upon CONFIG_XSPLICE (and I realize this applies to at least the
previous patch too).

> @@ -127,7 +127,7 @@ static void __init add_nops(void *insns, unsigned int len)
>   *
>   * This routine is called with local interrupt disabled.
>   */
> -static void *__init text_poke_early(void *addr, const void *opcode, size_t len)
> +static void *text_poke_early(void *addr, const void *opcode, size_t len)

I'm afraid this function's name as well as the comment preceding it
need to change.

> -static void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
> +void apply_alternatives_nocheck(struct alt_instr *start, struct alt_instr *end)

Same here - the preceding comment needs adjustment.

> --- a/xen/arch/x86/test/xen_hello_world_func.c
> +++ b/xen/arch/x86/test/xen_hello_world_func.c
> @@ -5,10 +5,13 @@
>  
>  #include <xen/config.h>
>  #include <xen/types.h>
> +#include <asm/nops.h>
> +#include <asm/alternative.h>
>  
>  /* Our replacement function for xen_extra_version. */
>  const char *xen_hello_world(void)
>  {
> +    alternative(ASM_NOP1, ASM_NOP1, 1);

Above you say the code is being exercised by this: How can you be
sure that whatever feature has value 1 is actually present? The
pending SMEP/SMAP patches add X86_FEATURE_ALWAYS for such
a purpose.

> --- a/xen/common/xsplice.c
> +++ b/xen/common/xsplice.c
> @@ -590,6 +590,22 @@ static int prepare_payload(struct payload *payload,
>          region->ex_end = (struct exception_table_entry *)(sec->load_addr + sec->sec->sh_size);
>  
>          sort_exception_table(region->ex, region->ex_end);
> +
> +    }

These two lines want to be swapped.

> +    sec = xsplice_elf_sec_by_name(elf, ".altinstructions");
> +    if ( sec )
> +    {
> +        if ( !sec->sec->sh_size ||
> +             (sec->sec->sh_size % sizeof (struct alt_instr)) )
> +        {
> +            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .alt_instr (exp:%lu vs %lu)!\n",
> +                    XSPLICE, elf->name, sizeof (struct alt_instr),
> +                    sec->sec->sh_size);
> +            return -EINVAL;
> +        }
> +        apply_alternatives_nocheck((struct alt_instr *)sec->load_addr,
> +                                   (struct alt_instr *)(sec->load_addr +
> +                                   sec->sec->sh_size));

I think alternative patching needs to enforce that only code/data
within the owning image gets patched, to avoid abuse.

> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -23,6 +23,12 @@ struct alt_instr {
>      u8  replacementlen;     /* length of new instruction, <= instrlen */
>  };
>  
> +/*
> + * An variant to be used on code that can be patched without many checks.
> + */

"A variant", comment style, and - what does "many" mean?

Jan

Konrad Rzeszutek Wilk April 7, 2016, 3:11 a.m. UTC | #2

On Fri, Apr 01, 2016 at 10:20:40AM -0600, Jan Beulich wrote:
> >>> On 24.03.16 at 21:00, <[e-mail]> wrote:
> > --- a/xen/arch/x86/alternative.c
> > +++ b/xen/arch/x86/alternative.c
> > @@ -28,7 +28,7 @@
> >  extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
> >  
> >  #ifdef K8_NOP1
> > -static const unsigned char k8nops[] __initconst = {
> > +static const unsigned char k8nops[] = {
> 
> Just like in Linux these init annotations should become conditional
> upon CONFIG_XSPLICE (and I realize this applies to at least the
> previous patch too).

I ended up declaring #define INIT __init and so on if CONFIG_XSPLICE
is not defined. Obviouslu they are empty if CONFIG_XSPLICE is set.

Since both alternative and exceptions use this I ended up putting
this in xsplice.h file.
..snip..
> >  /* Our replacement function for xen_extra_version. */
> >  const char *xen_hello_world(void)
> >  {
> > +    alternative(ASM_NOP1, ASM_NOP1, 1);
> 
> Above you say the code is being exercised by this: How can you be
> sure that whatever feature has value 1 is actually present? The
> pending SMEP/SMAP patches add X86_FEATURE_ALWAYS for such
> a purpose.

I must have missed them. I can change it once they go in. For right now
I just changed this X86_FEATURE_NX.

Patch

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index a1ef24b..d4a8069 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 26ad2b9..e423d3a 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -28,7 +28,7 @@ 
 extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
 
 #ifdef K8_NOP1
-static const unsigned char k8nops[] __initconst = {
+static const unsigned char k8nops[] = {
     K8_NOP1,
     K8_NOP2,
     K8_NOP3,
@@ -52,7 +52,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[] = {
     P6_NOP1,
     P6_NOP2,
     P6_NOP3,
@@ -75,7 +75,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 = k8_nops;
 
 static int __init mask_nmi_callback(const struct cpu_user_regs *regs, int cpu)
 {
@@ -100,7 +100,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 add_nops(void *insns, unsigned int len)
 {
     while ( len > 0 )
     {
@@ -127,7 +127,7 @@  static void __init add_nops(void *insns, unsigned int len)
  *
  * This routine is called with local interrupt disabled.
  */
-static void *__init text_poke_early(void *addr, const void *opcode, size_t len)
+static void *text_poke_early(void *addr, const void *opcode, size_t len)
 {
     memcpy(addr, opcode, len);
     sync_core();
@@ -142,15 +142,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 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. */
@@ -190,6 +188,12 @@  static void __init apply_alternatives(struct alt_instr *start, struct alt_instr
     write_cr0(cr0);
 }
 
+void 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 81380a6..2465ce9 100644
--- a/xen/arch/x86/test/xen_hello_world_func.c
+++ b/xen/arch/x86/test/xen_hello_world_func.c
@@ -5,10 +5,13 @@ 
 
 #include <xen/config.h>
 #include <xen/types.h>
+#include <asm/nops.h>
+#include <asm/alternative.h>
 
 /* Our replacement function for xen_extra_version. */
 const char *xen_hello_world(void)
 {
+    alternative(ASM_NOP1, ASM_NOP1, 1);
     return "Hello World";
 }
 
diff --git a/xen/common/xsplice.c b/xen/common/xsplice.c
index 4548b8b..bf8cb1c 100644
--- a/xen/common/xsplice.c
+++ b/xen/common/xsplice.c
@@ -590,6 +590,22 @@  static int prepare_payload(struct payload *payload,
         region->ex_end = (struct exception_table_entry *)(sec->load_addr + sec->sec->sh_size);
 
         sort_exception_table(region->ex, region->ex_end);
+
+    }
+    sec = xsplice_elf_sec_by_name(elf, ".altinstructions");
+    if ( sec )
+    {
+        if ( !sec->sec->sh_size ||
+             (sec->sec->sh_size % sizeof (struct alt_instr)) )
+        {
+            dprintk(XENLOG_DEBUG, "%s%s: Wrong size of .alt_instr (exp:%lu vs %lu)!\n",
+                    XSPLICE, elf->name, sizeof (struct alt_instr),
+                    sec->sec->sh_size);
+            return -EINVAL;
+        }
+        apply_alternatives_nocheck((struct alt_instr *)sec->load_addr,
+                                   (struct alt_instr *)(sec->load_addr +
+                                   sec->sec->sh_size));
     }
 #endif
     return 0;
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index 1056630..d50c0b5 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -23,6 +23,12 @@  struct alt_instr {
     u8  replacementlen;     /* length of new instruction, <= instrlen */
 };
 
+/*
+ * An variant to be used on code that can be patched without many checks.
+ */
+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"