[PATCH v5] Livepatch fixes and general features for Xen 4.8.

[PATCH v5] Livepatch fixes and general features for Xen 4.8.

[Konrad Rzeszutek Wilk]

Hey!

Since v4: [https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg02705.html]
 - Committed Acked/Reviewed patches.
 - Discarded couple of patches to address later.
Since v3: [https://lists.xen.org/archives/html/xen-devel/2016-08/msg01825.html]
 - Acked on reviews
v2, v1:
 - Left over fixes and features that didn't get quite done in 4.7

Included are:
 - Bug-fixes
 - NOP patching

The first two deal with .bss and replace the "livepatch: Clear .bss when
payload is reverted" which was posted in v4:

 [PATCH v5 1/4] livepatch/docs: Document .bss not being cleared, and
 [PATCH v5 2/4] livepatch: Add limit of 2MB to payload .bss sections.

The NOP patching mechanism is updated per Jan's thinking:
 [PATCH v5 3/4] livepatch: NOP if func->new_addr is zero.

And the last one was slighty changed since the Review done in v4 (ditched
the BUG_ON test-case as .bss can have old values) so will need one more review.
 [PATCH v5 4/4] livepach: Add .livepatch.hooks functions and test-case

Thanks!

The git tree `

 git://xenbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v5

contains all the following patches (and more):

Konrad Rzeszutek Wilk (3):
      livepatch/docs: Document .bss not being cleared, and .data potentially having changed values
      livepatch: Add limit of 2MB to payload .bss sections.
      livepatch: NOP if func->new_addr is zero.

Ross Lagerwall (1):
      livepach: Add .livepatch.hooks functions and test-case

 docs/misc/livepatch.markdown        | 36 +++++++++++++++++++++++--
 xen/arch/x86/alternative.c          |  2 +-
 xen/arch/x86/livepatch.c            | 38 +++++++++++++++++---------
 xen/arch/x86/test/xen_hello_world.c | 30 +++++++++++++++++++++
 xen/common/livepatch.c              | 53 +++++++++++++++++++++++++++++++++++--
 xen/common/livepatch_elf.c          | 10 ++++++-
 xen/include/asm-x86/alternative.h   |  1 +
 xen/include/asm-x86/livepatch.h     | 21 +++++++++++++++
 xen/include/xen/livepatch.h         |  9 +++++++
 xen/include/xen/livepatch_payload.h | 49 ++++++++++++++++++++++++++++++++++
 10 files changed, 230 insertions(+), 19 deletions(-)

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

[PATCH v5 1/4] livepatch/docs: Document .bss not being cleared, and .data potentially having changed values

[Konrad Rzeszutek Wilk]

. since the load time.

In summary we clear the .bss only during upload time. Subsequent
apply/revert/apply can change the .bss values - meaning that the
.bss values are not guaranteed to be zero - in fact they can be
anything!

And the .data can also be in various states.

Suggested-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>

v5: New submission
---
 docs/misc/livepatch.markdown | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 89c1050..07be0af 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -875,6 +875,12 @@ section and the new function will reference the new string in the new
 
 This is implemented in the Xen Project hypervisor.
 
+Note that the .bss section is only cleared when the ELF payload is uploaded.
+Subsequent apply/revert/apply operation do no clear the .bss (or reset the
+.data to what it was when loaded). Hence it is the responsibility of the
+creator of the payload to reset these values to known good state if they
+depend on them having certain values at apply/revert states.
+
 ### Security
 
 Only the privileged domain should be allowed to do this operation.
-- 
2.4.11


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

[PATCH v5 2/4] livepatch: Add limit of 2MB to payload .bss sections.

[Konrad Rzeszutek Wilk]

The initial patch: 11ff40fa7bb5fdcc69a58d0fec49c904ffca4793
"xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op" caps the
size of the binary at 2MB. We follow that in capping the size
of the .BSSes to be at maximum 2MB.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>

v5: Initial submission. Came about from conversation about
    "livepatch: Clear .bss when payload is reverted"
   - Use only one sh_flags comparison instead of two.
   - And check for the _right_ combination (WA).
---
 xen/common/livepatch_elf.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index cda9b27..e2264ad 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -86,7 +86,15 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
                     delta < sizeof(Elf_Ehdr) ? "at ELF header" : "is past end");
             return -EINVAL;
         }
-
+        else if ( (sec[i].sec->sh_flags & (SHF_WRITE | SHF_ALLOC)) &&
+                  sec[i].sec->sh_type == SHT_NOBITS &&
+                  sec[i].sec->sh_size > MB(2) )
+        {
+            /* Arbitrary limit. */
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section [%u] .bss is bigger than 2MB!\n",
+                    elf->name, i);
+            return -EINVAL;
+        }
         sec[i].data = data + delta;
         /* Name is populated in elf_resolve_section_names. */
         sec[i].name = NULL;
-- 
2.4.11


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

[PATCH v5 3/4] livepatch: NOP if func->new_addr is zero.

[Konrad Rzeszutek Wilk]

The NOP functionality will NOP any of the code at
the 'old_addr' or at 'name' if the 'new_addr' is zero.
The purpose of this is to NOP out calls, such as:

 e8 <4-bytes-offset>

(5 byte insn), or on ARM a 4 byte insn for branching.

We need the EIP of where we need to the NOP, and that can
be provided via the `old_addr` or `name`.

If the `old_addr` is provided we will NOP 'new_size'
amount of bytes at that location.

The amount is up to 31 instructions if desired (which is
the size of the opaque member). If there is a need to NOP
more then either more 'struct livepatch_func' structures need
to be present or we have to implement a variable size buffer.

While at it, also unify the code on x86 patching so
it is a bit simpler (instead of two seperate writes
just make it one memcpy).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3: First submission
v4: Fix description - e9 -> e8
    Remove the restriction of only doing 5 or 4 bytes.
    Redo the patching code to deal with variable size of new_size.
    Expand the amount of bytes we can NOP.
    Move the PATCH_INSN_SIZE definition in platform specific headers
    Move the get_len to livepatch_get_insn_len inline function.
---
 docs/misc/livepatch.markdown      |  7 +++++--
 xen/arch/x86/alternative.c        |  2 +-
 xen/arch/x86/livepatch.c          | 38 +++++++++++++++++++++++++-------------
 xen/common/livepatch.c            |  3 ++-
 xen/include/asm-x86/alternative.h |  1 +
 xen/include/asm-x86/livepatch.h   | 21 +++++++++++++++++++++
 xen/include/xen/livepatch.h       |  9 +++++++++
 7 files changed, 64 insertions(+), 17 deletions(-)
 create mode 100644 xen/include/asm-x86/livepatch.h

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 07be0af..e2ea351 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -320,10 +320,13 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be
 
 * `new_addr` is the address of the function that is replacing the old
   function. The address is filled in during relocation. The value **MUST** be
-  the address of the new function in the file.
+  either the address of the new function in the file, or zero if we are NOPing out
+  at `old_addr` (and `new_size` **MUST** not be zero).
 
 * `old_size` and `new_size` contain the sizes of the respective functions in bytes.
-   The value of `old_size` **MUST** not be zero.
+   The value of `old_size` **MUST** not be zero. If the value of `new_addr` is
+   zero then `new_size` determines how many instruction bytes to NOP (up to
+   opaque size modulo smallest platform instruction - 1 byte x86 and 4 bytes on ARM).
 
 * `version` is to be one.
 
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 05e3eb8..6eaa10f 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -101,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_or_livepatch add_nops(void *insns, unsigned int len)
+void init_or_livepatch add_nops(void *insns, unsigned int len)
 {
     while ( len > 0 )
     {
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 725b3f6..2f54d7b 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -12,8 +12,7 @@
 #include <xen/livepatch.h>
 
 #include <asm/nmi.h>
-
-#define PATCH_INSN_SIZE 5
+#include <asm/livepatch.h>
 
 int arch_livepatch_quiesce(void)
 {
@@ -31,8 +30,8 @@ void arch_livepatch_revive(void)
 
 int arch_livepatch_verify_func(const struct livepatch_func *func)
 {
-    /* No NOP patching yet. */
-    if ( !func->new_size )
+    /* If NOPing only do up to maximum amount we can put in the ->opaque. */
+    if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
         return -EOPNOTSUPP;
 
     if ( func->old_size < PATCH_INSN_SIZE )
@@ -43,23 +42,36 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
 
 void arch_livepatch_apply_jmp(struct livepatch_func *func)
 {
-    int32_t val;
     uint8_t *old_ptr;
-
-    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
-    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
+    uint8_t insn[sizeof(func->opaque)];
+    size_t len;
 
     old_ptr = func->old_addr;
-    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
+    len = arch_livepatch_insn_len(func);
+    if ( !len )
+        return;
+
+    memcpy(func->opaque, old_ptr, len);
+    if ( func->new_addr )
+    {
+        int32_t val;
+
+        BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
+
+        insn[0] = 0xe9;
+        val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
+
+        memcpy(&insn[1], &val, sizeof(val));
+    }
+    else
+        add_nops(&insn, len);
 
-    *old_ptr++ = 0xe9; /* Relative jump */
-    val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
-    memcpy(old_ptr, &val, sizeof(val));
+    memcpy(old_ptr, insn, len);
 }
 
 void arch_livepatch_revert_jmp(const struct livepatch_func *func)
 {
-    memcpy(func->old_addr, func->opaque, PATCH_INSN_SIZE);
+    memcpy(func->old_addr, func->opaque, arch_livepatch_insn_len(func));
 }
 
 /* Serialise the CPU pipeline. */
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 23e4d51..f218c25 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -510,7 +510,8 @@ static int prepare_payload(struct payload *payload,
             return -EOPNOTSUPP;
         }
 
-        if ( !f->new_addr || !f->new_size )
+        /* 'old_addr', 'new_addr', 'new_size' can all be zero. */
+        if ( !f->old_size )
         {
             dprintk(XENLOG_ERR, LIVEPATCH "%s: Address or size fields are zero!\n",
                     elf->name);
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index 67fc0d2..db4f08e 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -27,6 +27,7 @@ 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 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);
diff --git a/xen/include/asm-x86/livepatch.h b/xen/include/asm-x86/livepatch.h
new file mode 100644
index 0000000..63ea079
--- /dev/null
+++ b/xen/include/asm-x86/livepatch.h
@@ -0,0 +1,21 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#ifndef __XEN_X86_LIVEPATCH_H__
+#define __XEN_X86_LIVEPATCH_H__
+
+#define PATCH_INSN_SIZE 5
+
+#endif /* __XEN_X86_LIVEPATCH_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 243e240..8258e02 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -66,7 +66,16 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type types
 void arch_livepatch_init(void);
 
 #include <public/sysctl.h> /* For struct livepatch_func. */
+#include <asm/livepatch.h> /* For PATCH_INSN_SIZE. */
 int arch_livepatch_verify_func(const struct livepatch_func *func);
+
+static inline size_t arch_livepatch_insn_len(const struct livepatch_func *func)
+{
+    if ( !func->new_addr )
+        return func->new_size;
+
+    return PATCH_INSN_SIZE;
+}
 /*
  * These functions are called around the critical region patching live code,
  * for an architecture to take make appropratie global state adjustments.
-- 
2.4.11


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

[PATCH v5 4/4] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Add hook functions which run during patch apply and patch revert.
Hook functions are used by livepatch payloads to manipulate data
structures during patching, etc.

One use case is the XSA91. As Martin mentions it:
"If we have shadow variables, we also need an unload hook to garbage
collect all the variables introduced by a hotpatch to prevent memory
leaks.  Potentially, we also want to pre-reserve memory for static or
existing dynamic objects in the load-hook instead of on the fly.

For testing and debugging, various applications are possible.

In general, the hooks provide flexibility when having to deal with
unforeseen cases, but their application should be rarely required (<
10%)."

Furthermore include a test-case for it.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

v0.4..v0.11: Defered for v4.8
v0.12: s/xsplice/livepatch/

v3: Clarify the comments about spin_debug_enable
    Rename one of the hooks to lower-case (Z->z) to guarantee it being
    called last.
    Learned a lot of about 'const'
v4: Do the actual const of the hooks.
    Remove the requirement for the tests-case hooks to be sorted
    (they never were to start with).
    Fix up the comment about spin_debug_enable so more.
v5: Added Reviewed-by from Andrew.
    Removed some of the hooks test-cases as the .bss is only cleared during loading.
    Dropped Reviewed-by from Andrew as the test-case changed.
---
 docs/misc/livepatch.markdown        | 23 +++++++++++++++++
 xen/arch/x86/test/xen_hello_world.c | 30 ++++++++++++++++++++++
 xen/common/livepatch.c              | 50 ++++++++++++++++++++++++++++++++++++-
 xen/include/xen/livepatch_payload.h | 49 ++++++++++++++++++++++++++++++++++++
 4 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/xen/livepatch_payload.h

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index e2ea351..53b5371 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -343,6 +343,13 @@ When reverting a patch, the hypervisor iterates over each `livepatch_func`
 and the core code copies the data from the undo buffer (private internal copy)
 to `old_addr`.
 
+It optionally may contain the address of functions to be called right before
+being applied and after being reverted:
+
+ * `.livepatch.hooks.load` - an array of function pointers.
+ * `.livepatch.hooks.unload` - an array of function pointers.
+
+
 ### Example of .livepatch.funcs
 
 A simple example of what a payload file can be:
@@ -380,6 +387,22 @@ struct livepatch_func livepatch_hello_world = {
 
 Code must be compiled with -fPIC.
 
+### .livepatch.hooks.load and .livepatch.hooks.unload
+
+This section contains an array of function pointers to be executed
+before payload is being applied (.livepatch.funcs) or after reverting
+the payload. This is useful to prepare data structures that need to
+be modified patching.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+<pre>
+typedef void (*livepatch_loadcall_t)(void);  
+typedef void (*livepatch_unloadcall_t)(void);   
+</pre>
+
 ### .livepatch.depends and .note.gnu.build-id
 
 To support dependencies checking and safe loading (to load the
diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
index 422bdf1..472fe2e 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -4,14 +4,44 @@
  */
 
 #include "config.h"
+#include <xen/lib.h>
 #include <xen/types.h>
 #include <xen/version.h>
 #include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
 
 #include <public/sysctl.h>
 
 static char hello_world_patch_this_fnc[] = "xen_extra_version";
 extern const char *xen_hello_world(void);
+/*
+ * .bss is only cleared once - during upload. Apply/revert/apply will result
+ * in cnt to have a non-zero value (see the hooks below).
+ */
+static unsigned int cnt;
+
+static void apply_hook(void)
+{
+    printk(KERN_DEBUG "Hook executing.\n");
+}
+
+static void revert_hook(void)
+{
+    printk(KERN_DEBUG "Hook unloaded.\n");
+}
+
+static void  hi_func(void)
+{
+    printk(KERN_DEBUG "%s: Hi! (called %u times)\n", __func__, ++cnt);
+};
+
+LIVEPATCH_LOAD_HOOK(apply_hook);
+LIVEPATCH_UNLOAD_HOOK(revert_hook);
+
+/* Imbalance here. Two load and three unload. */
+
+LIVEPATCH_LOAD_HOOK(hi_func);
+LIVEPATCH_UNLOAD_HOOK(hi_func);
 
 struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
     .version = LIVEPATCH_PAYLOAD_VERSION,
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index f218c25..f75171b 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -23,6 +23,7 @@
 #include <xen/wait.h>
 #include <xen/livepatch_elf.h>
 #include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
 
 #include <asm/event.h>
 
@@ -70,7 +71,11 @@ struct payload {
     unsigned int nsyms;                  /* Nr of entries in .strtab and symbols. */
     struct livepatch_build_id id;        /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
     struct livepatch_build_id dep;       /* ELFNOTE_DESC(.livepatch.depends). */
-    char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
+    livepatch_loadcall_t *const *load_funcs;   /* The array of funcs to call after */
+    livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the payload. */
+    unsigned int n_load_funcs;           /* Nr of the funcs to load and execute. */
+    unsigned int n_unload_funcs;         /* Nr of funcs to call durung unload. */
+    char name[XEN_LIVEPATCH_NAME_SIZE];  /* Name of it. */
 };
 
 /* Defines an outstanding patching action. */
@@ -527,6 +532,25 @@ static int prepare_payload(struct payload *payload,
             return rc;
     }
 
+    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
+    if ( sec )
+    {
+        if ( sec->sec->sh_size % sizeof(*payload->load_funcs) )
+            return -EINVAL;
+
+        payload->load_funcs = sec->load_addr;
+        payload->n_load_funcs = sec->sec->sh_size / sizeof(*payload->load_funcs);
+    }
+
+    sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.unload");
+    if ( sec )
+    {
+        if ( sec->sec->sh_size % sizeof(*payload->unload_funcs) )
+            return -EINVAL;
+
+        payload->unload_funcs = sec->load_addr;
+        payload->n_unload_funcs = sec->sec->sh_size / sizeof(*payload->unload_funcs);
+    }
     sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
     if ( sec )
     {
@@ -1017,6 +1041,18 @@ static int apply_payload(struct payload *data)
         return rc;
     }
 
+    /*
+     * Since we are running with IRQs disabled and the hooks may call common
+     * code - which expects certain spinlocks to run with IRQs enabled - we
+     * temporarily disable the spin locks IRQ state checks.
+     */
+    spin_debug_disable();
+    for ( i = 0; i < data->n_load_funcs; i++ )
+        data->load_funcs[i]();
+    spin_debug_enable();
+
+    ASSERT(!local_irq_is_enabled());
+
     for ( i = 0; i < data->nfuncs; i++ )
         arch_livepatch_apply_jmp(&data->funcs[i]);
 
@@ -1049,6 +1085,18 @@ static int revert_payload(struct payload *data)
     for ( i = 0; i < data->nfuncs; i++ )
         arch_livepatch_revert_jmp(&data->funcs[i]);
 
+    /*
+     * Since we are running with IRQs disabled and the hooks may call common
+     * code - which expects certain spinlocks to run with IRQs enabled - we
+     * temporarily disable the spin locks IRQ state checks.
+     */
+    spin_debug_disable();
+    for ( i = 0; i < data->n_unload_funcs; i++ )
+        data->unload_funcs[i]();
+    spin_debug_enable();
+
+    ASSERT(!local_irq_is_enabled());
+
     arch_livepatch_revive();
 
     /*
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
new file mode 100644
index 0000000..8f38cc2
--- /dev/null
+++ b/xen/include/xen/livepatch_payload.h
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2016 Citrix Systems R&D Ltd.
+ */
+
+#ifndef __XEN_LIVEPATCH_PAYLOAD_H__
+#define __XEN_LIVEPATCH_PAYLOAD_H__
+
+/*
+ * The following definitions are to be used in patches. They are taken
+ * from kpatch.
+ */
+typedef void livepatch_loadcall_t(void);
+typedef void livepatch_unloadcall_t(void);
+
+/*
+ * LIVEPATCH_LOAD_HOOK macro
+ *
+ * Declares a function pointer to be allocated in a new
+ * .livepatch.hook.load section.  This livepatch_load_data symbol is later
+ * stripped by create-diff-object so that it can be declared in multiple
+ * objects that are later linked together, avoiding global symbol
+ * collision.  Since multiple hooks can be registered, the
+ * .livepatch.hook.load section is a table of functions that will be
+ * executed in series by the livepatch infrastructure at patch load time.
+ */
+#define LIVEPATCH_LOAD_HOOK(_fn) \
+    livepatch_loadcall_t *__attribute__((weak)) \
+        const livepatch_load_data_##_fn __section(".livepatch.hooks.load") = _fn;
+
+/*
+ * LIVEPATCH_UNLOAD_HOOK macro
+ *
+ * Same as LOAD hook with s/load/unload/
+ */
+#define LIVEPATCH_UNLOAD_HOOK(_fn) \
+     livepatch_unloadcall_t *__attribute__((weak)) \
+        const livepatch_unload_data_##_fn __section(".livepatch.hooks.unload") = _fn;
+
+#endif /* __XEN_LIVEPATCH_PAYLOAD_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.4.11


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

Re: [PATCH v5 1/4] livepatch/docs: Document .bss not being cleared, and .data potentially having changed values

[Jan Beulich]

>>> On 11.09.16 at 17:48, <konrad.wilk@oracle.com> wrote:
> --- a/docs/misc/livepatch.markdown
> +++ b/docs/misc/livepatch.markdown
> @@ -875,6 +875,12 @@ section and the new function will reference the new string in the new
>  
>  This is implemented in the Xen Project hypervisor.
>  
> +Note that the .bss section is only cleared when the ELF payload is uploaded.
> +Subsequent apply/revert/apply operation do no clear the .bss (or reset the
> +.data to what it was when loaded). Hence it is the responsibility of the
> +creator of the payload to reset these values to known good state if they
> +depend on them having certain values at apply/revert states.

Was it, as an alternative, considered to disallow re-applying a
reverted patch without re-uploading?

Jan


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

Re: [PATCH v5 2/4] livepatch: Add limit of 2MB to payload .bss sections.

[Jan Beulich]

>>> On 11.09.16 at 17:48, <konrad.wilk@oracle.com> wrote:
> The initial patch: 11ff40fa7bb5fdcc69a58d0fec49c904ffca4793
> "xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op" caps the
> size of the binary at 2MB. We follow that in capping the size
> of the .BSSes to be at maximum 2MB.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> ---
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> 
> v5: Initial submission. Came about from conversation about
>     "livepatch: Clear .bss when payload is reverted"
>    - Use only one sh_flags comparison instead of two.
>    - And check for the _right_ combination (WA).
> ---
>  xen/common/livepatch_elf.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
> index cda9b27..e2264ad 100644
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -86,7 +86,15 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
>                      delta < sizeof(Elf_Ehdr) ? "at ELF header" : "is past end");
>              return -EINVAL;
>          }
> -

I think this blank line would better be retained (after the addition you
make).

> +        else if ( (sec[i].sec->sh_flags & (SHF_WRITE | SHF_ALLOC)) &&
> +                  sec[i].sec->sh_type == SHT_NOBITS &&
> +                  sec[i].sec->sh_size > MB(2) )
> +        {
> +            /* Arbitrary limit. */

I guess this is as arbitrary as the one in verify_payload(). I'd like to
recommend making this a #define, with the comment put next to it.

> +            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section [%u] .bss is bigger than 2MB!\n",
> +                    elf->name, i);

The other limit violation doesn't get a message logged.

Jan


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

Re: [PATCH v5 3/4] livepatch: NOP if func->new_addr is zero.

[Jan Beulich]

>>> On 11.09.16 at 17:48, <konrad.wilk@oracle.com> wrote:
> The NOP functionality will NOP any of the code at
> the 'old_addr' or at 'name' if the 'new_addr' is zero.
> The purpose of this is to NOP out calls, such as:
> 
>  e8 <4-bytes-offset>
> 
> (5 byte insn), or on ARM a 4 byte insn for branching.
> 
> We need the EIP of where we need to the NOP, and that can
> be provided via the `old_addr` or `name`.
> 
> If the `old_addr` is provided we will NOP 'new_size'
> amount of bytes at that location.
> 
> The amount is up to 31 instructions if desired (which is
> the size of the opaque member). If there is a need to NOP
> more then either more 'struct livepatch_func' structures need
> to be present or we have to implement a variable size buffer.

Actually I meanwhile realized that if we wanted to void more than
this many, it would probably be better to simply make the first bytes
an unconditional branch skipping the to be disabled code (of course
provided there are no branch targets in the middle).

> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -66,7 +66,16 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type types
>  void arch_livepatch_init(void);
>  
>  #include <public/sysctl.h> /* For struct livepatch_func. */
> +#include <asm/livepatch.h> /* For PATCH_INSN_SIZE. */
>  int arch_livepatch_verify_func(const struct livepatch_func *func);
> +
> +static inline size_t arch_livepatch_insn_len(const struct livepatch_func *func)

This being defined (rather than just declared) here makes me question
the arch_ prefix.

> +{
> +    if ( !func->new_addr )
> +        return func->new_size;
> +
> +    return PATCH_INSN_SIZE;

This, otoh, now might better be ARCH_PATCH_INSN_SIZE.

Jan


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

Re: [PATCH v5 1/4] livepatch/docs: Document .bss not being cleared, and .data potentially having changed values

[Konrad Rzeszutek Wilk]

On Mon, Sep 12, 2016 at 01:49:51AM -0600, Jan Beulich wrote:
> >>> On 11.09.16 at 17:48, <konrad.wilk@oracle.com> wrote:
> > --- a/docs/misc/livepatch.markdown
> > +++ b/docs/misc/livepatch.markdown
> > @@ -875,6 +875,12 @@ section and the new function will reference the new string in the new
> >  
> >  This is implemented in the Xen Project hypervisor.
> >  
> > +Note that the .bss section is only cleared when the ELF payload is uploaded.
> > +Subsequent apply/revert/apply operation do no clear the .bss (or reset the
> > +.data to what it was when loaded). Hence it is the responsibility of the
> > +creator of the payload to reset these values to known good state if they
> > +depend on them having certain values at apply/revert states.
> 
> Was it, as an alternative, considered to disallow re-applying a
> reverted patch without re-uploading?

I was thinking about it but not exactly sure of the ramifications.

That is if you go this route - revert a patch - we could go the
next step and just unload it. But that puts some question on how
to schedule that without corruption - say we have payload A,B and we
are replacing A,B with C. A,B should be reverted and unloaded..

That means some form of list .. Ugh.

We could do a simpler way (which is what I think inline with your
suggestion). This does the trick (and needs to be split up obviously)
and also handles the case where you only have .text changes (like
for NOPs).

Interestingly, the linker always adds an .bss and .data section
even if the code does not produce it.

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d9eeab1..4a1abb2 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -53,6 +53,8 @@ struct livepatch_build_id {
 struct payload {
     uint32_t state;                      /* One of the LIVEPATCH_STATE_*. */
     int32_t rc;                          /* 0 or -XEN_EXX. */
+    bool_t reverted;                     /* Whether it was reverted. */
+    bool_t safe_to_reapply;              /* Can apply safely after revert. */
     struct list_head list;               /* Linked to 'payload_list'. */
     const void *text_addr;               /* Virtual address of .text. */
     size_t text_size;                    /* .. and its size. */
@@ -313,7 +315,7 @@ static void calc_section(const struct livepatch_elf_sec *sec, size_t *size,
 static int move_payload(struct payload *payload, struct livepatch_elf *elf)
 {
     void *text_buf, *ro_buf, *rw_buf;
-    unsigned int i;
+    unsigned int i, rw_buf_sec, rw_buf_cnt = 0;
     size_t size = 0;
     unsigned int *offset;
     int rc = 0;
@@ -386,7 +388,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
             if ( elf->sec[i].sec->sh_flags & SHF_EXECINSTR )
                 buf = text_buf;
             else if ( elf->sec[i].sec->sh_flags & SHF_WRITE )
+            {
                 buf = rw_buf;
+                rw_buf_sec = i;
+                rw_buf_cnt++;
+            }
             else
                 buf = ro_buf;
 
@@ -407,6 +413,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
         }
     }
 
+    if ( rw_buf_cnt == 1 )
+    {
+        if ( !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) )
+            payload->safe_to_reapply = true;
+    }
  out:
     xfree(offset);
 
@@ -1120,6 +1131,7 @@ static int revert_payload(struct payload *data)
     list_del_rcu(&data->applied_list);
     unregister_virtual_region(&data->region);
 
+    data->reverted = true;
     return 0;
 }
 
@@ -1501,6 +1513,19 @@ static int livepatch_action(xen_sysctl_livepatch_action_t *action)
     case LIVEPATCH_ACTION_APPLY:
         if ( data->state == LIVEPATCH_STATE_CHECKED )
         {
+            /*
+             * It is unsafe to apply an reverted as the .data may not be in
+             * in pristine condition. Hence MUST unload and then apply patch
+             * again.
+             */
+            if ( data->reverted && !data->safe_to_reapply )
+            {
+                dprintk(XENLOG_ERR, "%s%s: can't revert as payload has .data. Please unload!\n",
+                        LIVEPATCH, data->name);
+                data->rc = -EINVAL;
+                break;
+            }
+
             rc = build_id_dep(data, !!list_empty(&applied_list));
             if ( rc )
                 break;
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 69b0cdd..e03a26d 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -72,7 +72,7 @@ $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
 note.o:
 	$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin
 	$(OBJCOPY) $(OBJCOPY_MAGIC) \
-		   --rename-section=.data=.livepatch.depends -S $@.bin $@
+		   --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@
 	rm -f $@.bin
 
 #
@@ -83,7 +83,7 @@ note.o:
 hello_world_note.o: $(LIVEPATCH)
 	$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) $@.bin
 	$(OBJCOPY) $(OBJCOPY_MAGIC) \
-		   --rename-section=.data=.livepatch.depends -S $@.bin $@
+		   --rename-section=.data=.livepatch.depends,,alloc,load,readonly,data,contents -S $@.bin $@
 	rm -f $@.bin
 
 xen_bye_world.o: config.h
@@ -103,6 +103,8 @@ xen_nop.o: config.h
 .PHONY: $(LIVEPATCH_NOP)
 $(LIVEPATCH_NOP): xen_nop.o note.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^
+	$(OBJCOPY) --remove-section=.bss --remove-section=.data $@
+	$(OBJCOPY) --strip-unneeded $@
 
 .PHONY: livepatch
 livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP)

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

Re: [PATCH v5 2/4] livepatch: Add limit of 2MB to payload .bss sections.

[Ross Lagerwall]

On 09/11/2016 04:48 PM, Konrad Rzeszutek Wilk wrote:
> The initial patch: 11ff40fa7bb5fdcc69a58d0fec49c904ffca4793
> "xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op" caps the
> size of the binary at 2MB. We follow that in capping the size
> of the .BSSes to be at maximum 2MB.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [PATCH v5 1/4] livepatch/docs: Document .bss not being cleared, and .data potentially having changed values

[Jan Beulich]

>>> On 13.09.16 at 17:59, <konrad.wilk@oracle.com> wrote:
> On Mon, Sep 12, 2016 at 01:49:51AM -0600, Jan Beulich wrote:
>> >>> On 11.09.16 at 17:48, <konrad.wilk@oracle.com> wrote:
>> > --- a/docs/misc/livepatch.markdown
>> > +++ b/docs/misc/livepatch.markdown
>> > @@ -875,6 +875,12 @@ section and the new function will reference the new string in the new
>> >  
>> >  This is implemented in the Xen Project hypervisor.
>> >  
>> > +Note that the .bss section is only cleared when the ELF payload is uploaded.
>> > +Subsequent apply/revert/apply operation do no clear the .bss (or reset the
>> > +.data to what it was when loaded). Hence it is the responsibility of the
>> > +creator of the payload to reset these values to known good state if they
>> > +depend on them having certain values at apply/revert states.
>> 
>> Was it, as an alternative, considered to disallow re-applying a
>> reverted patch without re-uploading?
> 
> I was thinking about it but not exactly sure of the ramifications.
> 
> That is if you go this route - revert a patch - we could go the
> next step and just unload it. But that puts some question on how
> to schedule that without corruption - say we have payload A,B and we
> are replacing A,B with C. A,B should be reverted and unloaded..
> 
> That means some form of list .. Ugh.
> 
> We could do a simpler way (which is what I think inline with your
> suggestion). This does the trick (and needs to be split up obviously)
> and also handles the case where you only have .text changes (like
> for NOPs).

At least that draft patch looks reasonable at a first glance.

Jan


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