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

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

[Konrad Rzeszutek Wilk]

Hey!

Since v6: [https://lists.xen.org/archives/html/xen-devel/2016-09/msg01719.html]
 - Remade "livepatch: Disallow applying after an revert" to look at sh_size.
 - Added some Reviewed-by/Acks.
 - Added changes as requested.
 - Included a new patch: "livepatch: Drop _jmp from arch_livepatch_[apply,revert]_jmp"
 - Commited two patches which were Acked/Reviewed-by.
Since v5:[https://lists.xen.org/archives/html/xen-devel/2016-09/msg01114.html]
 - Acted on the review comments.
 - Replaced "livepatch/docs: Document .bss not being cleared, and .data potentially 
   having changed values" with "livepatch: Disallow applying after an revert"
 - Added two minor fixes to the test-cases (one of them was part of the ARM32/64
   livepatch support).
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
 - Hooks

The legend is:

 r - Reviewed-by
 a - Acked-by

   livepatch: Disallow applying after an revert
r  livepatch: Add limit of 2MB to payload .bss sections.
   livepatch: NOP if func->new_addr is zero.
   livepatch: Drop _jmp from
r  livepach: Add .livepatch.hooks functions and test-case


Thanks!

The git tree is

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

contains all the following patches (and more):

 MAINTAINERS                         |  1 +
 docs/misc/livepatch.markdown        | 52 ++++++++++++++++++--
 xen/arch/arm/livepatch.c            |  4 +-
 xen/arch/x86/alternative.c          |  2 +-
 xen/arch/x86/livepatch.c            | 52 +++++++++++++-------
 xen/arch/x86/test/xen_hello_world.c | 34 +++++++++++++
 xen/common/livepatch.c              | 95 +++++++++++++++++++++++++++++++++----
 xen/common/livepatch_elf.c          |  7 ++-
 xen/include/asm-x86/alternative.h   |  1 +
 xen/include/asm-x86/livepatch.h     | 21 ++++++++
 xen/include/xen/livepatch.h         | 16 ++++++-
 xen/include/xen/livepatch_payload.h | 49 +++++++++++++++++++
 12 files changed, 298 insertions(+), 36 deletions(-)

Konrad Rzeszutek Wilk (4):
      livepatch: Disallow applying after an revert
      livepatch: Add limit of 2MB to payload .bss sections.
      livepatch: NOP if func->new_addr is zero.
      livepatch: Drop _jmp from arch_livepatch_[apply,revert]_jmp

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


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

[PATCH v7 1/5] livepatch: Disallow applying after an revert

[Konrad Rzeszutek Wilk]

On general this is unhealthy - as the payload's .bss (definitly)
or .data (maybe) will be modified once the payload is running.

Doing an revert and then re-applying the payload with a non-pristine
.bss or .data can lead to unforseen consequences (.bss are assumed
to always contain zero value but now they may have a different value).

There is one exception - if the payload contains only one .data section
- the .livepatch.funcs, then it is OK to re-apply an revert.
We detect this rather simply (if there is one RW section and its name
is .livepatch.funcs) - but the payload may have many other RW sections
that are not used at all (such as .bss or .data sections with zero
length). To not account those we also ignore sections with sh_size
being zero.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

v6: New submission.
v7: Use 'bool' instead of 'bool_t'
  - Ignore sh_size==0 sections as a way to make the exception check work.
  - Also add the sh_size==0 check in livepatch_elf_resolve_symbols.
  - Update comment in load move_payload to mention
    "livepatch_elf_resolve_symbols"
---
 docs/misc/livepatch.markdown |  7 +++++++
 xen/common/livepatch.c       | 36 +++++++++++++++++++++++++++++++++---
 xen/common/livepatch_elf.c   |  3 ++-
 3 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 89c1050..81f4fc9 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -1061,6 +1061,13 @@ depending on the current state of data. As such it should not be attempted.
 That said we should provide hook functions so that the existing data
 can be changed during payload application.
 
+To guarantee safety we disallow re-applying an payload after it has been
+reverted. This is because we cannot guarantee that the state of .bss
+and .data to be exactly as it was during loading. Hence the administrator
+MUST unload the payload and upload it again to apply it.
+
+There is an exception to this: if the payload only has .livepatch.funcs;
+and no .data or .bss sections.
 
 ### Inline patching
 
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 23e4d51..1f527a3 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -52,6 +52,8 @@ struct livepatch_build_id {
 struct payload {
     uint32_t state;                      /* One of the LIVEPATCH_STATE_*. */
     int32_t rc;                          /* 0 or -XEN_EXX. */
+    bool reverted;                       /* Whether it was reverted. */
+    bool 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. */
@@ -308,7 +310,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;
@@ -325,8 +327,13 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
          * and .shstrtab. For the non-relocate we allocate and copy these
          * via other means - and the .rel we can ignore as we only use it
          * once during loading.
+         *
+         * Also ignore sections with zero size. Those can be .data, or .bss.
+         *
+         * This logic must MATCH what is done in livepatch_elf_resolve_symbols.
          */
-        if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) )
+        if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) ||
+             elf->sec[i].sec->sh_size == 0 )
             offset[i] = UINT_MAX;
         else if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
                    !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
@@ -374,14 +381,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
 
     for ( i = 1; i < elf->hdr->e_shnum; i++ )
     {
-        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
+        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC && elf->sec[i].sec->sh_size )
         {
             void *buf;
 
             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;
 
@@ -402,6 +413,10 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
         }
     }
 
+    /* Only one RW section with non-zero size: .livepatch.funcs */
+    if ( rw_buf_cnt == 1 &&
+         !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) )
+        payload->safe_to_reapply = true;
  out:
     xfree(offset);
 
@@ -1057,6 +1072,7 @@ static int revert_payload(struct payload *data)
     list_del_rcu(&data->applied_list);
     unregister_virtual_region(&data->region);
 
+    data->reverted = true;
     return 0;
 }
 
@@ -1438,6 +1454,20 @@ 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 payload as the .data (or .bss)
+             * may not be in in pristine condition. Hence MUST unload and then
+             * apply patch again. Unless the payload has no .bss or only one
+             * RW section (.livepatch.funcs).
+             */
+            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/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index cda9b27..303115f 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -311,7 +311,8 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf *elf)
             }
 
             /* Matches 'move_payload' which ignores such sections. */
-            if ( !(elf->sec[idx].sec->sh_flags & SHF_ALLOC) )
+            if ( !(elf->sec[idx].sec->sh_flags & SHF_ALLOC) ||
+                 elf->sec[idx].sec->sh_size == 0 )
                 break;
 
             st_value += (unsigned long)elf->sec[idx].load_addr;
-- 
2.4.11


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

[PATCH v7 2/5] 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.

We also bubble up the payload limit and this one in one #define
called LIVEPATCH_MAX_SIZE to make it easier to find these
arbitrary limits.

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
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).
v6: Remove the logging
   - Move the MB(2) to a #define in the header file.
   - Add the newline after the addition in livepatch_elf.c.
   - Added Reviewed-by from Ross.
v7:- s/MAX_BSS_SIZE/LIVEPATCH_MAX_SIZE/
   - Also use this LIVEPATHCH_MAX_SIZE in verify_payload
---
 xen/common/livepatch.c      | 2 +-
 xen/common/livepatch_elf.c  | 4 ++++
 xen/include/xen/livepatch.h | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 1f527a3..c9e5318 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -123,7 +123,7 @@ static int verify_payload(const xen_sysctl_livepatch_upload_t *upload, char *n)
     if ( !upload->size )
         return -EINVAL;
 
-    if ( upload->size > MB(2) )
+    if ( upload->size > LIVEPATCH_MAX_SIZE )
         return -EINVAL;
 
     if ( !guest_handle_okay(upload->payload, upload->size) )
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index 303115f..f46990e 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -86,6 +86,10 @@ 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 > LIVEPATCH_MAX_SIZE )
+            return -EINVAL;
 
         sec[i].data = data + delta;
         /* Name is populated in elf_resolve_section_names. */
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 243e240..29c9b31 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -30,6 +30,8 @@ struct xen_sysctl_livepatch_op;
 #define ELF_LIVEPATCH_FUNC    ".livepatch.funcs"
 #define ELF_LIVEPATCH_DEPENDS ".livepatch.depends"
 #define ELF_BUILD_ID_NOTE      ".note.gnu.build-id"
+/* Arbitrary limit for payload size and .bss section size. */
+#define LIVEPATCH_MAX_SIZE     MB(2)
 
 struct livepatch_symbol {
     const char *name;
-- 
2.4.11


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

[PATCH v7 3/5] 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: a) more 'struct livepatch_func' structures need
to be present, b) we have to implement a variable size
buffer (in the future), or c) first byte an unconditional
branch skipping the to be disabled code (of course provided
there are no branch targets in the middle).

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).

And introduce a general livepatch_insn_len inline function
that would depend on platform specific instruction size
(for a unconditional branch). As such we also rename the
PATCH_INSN_SIZE to ARCH_PATCH_INSN_SIZE.

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.
v5: s/PATCH_INSN_SIZE/ARCH_PATCH_INSN_SIZE/
    s/arch_livepatch_insn_len/livepatch_insn_len/
    s/size_t len/unsigned int len/
    Add in commit description the c) mechanism (insert an unconditional
    branch).
v6: Expand in the documentation about the old_addr being at least 5.
v7: Added the extra entry in the MAINTAINERS file.
  - Expanded on the livepatch.markdown the description.
  - Used add_nops(insn, .. instead of add_nops(&insn, ..)
---
 MAINTAINERS                       |  1 +
 docs/misc/livepatch.markdown      | 22 ++++++++++++++----
 xen/arch/x86/alternative.c        |  2 +-
 xen/arch/x86/livepatch.c          | 48 +++++++++++++++++++++++++++------------
 xen/common/livepatch.c            |  3 ++-
 xen/include/asm-x86/alternative.h |  1 +
 xen/include/asm-x86/livepatch.h   | 21 +++++++++++++++++
 xen/include/xen/livepatch.h       | 10 ++++++++
 8 files changed, 86 insertions(+), 22 deletions(-)
 create mode 100644 xen/include/asm-x86/livepatch.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 97720a8..9b30600 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -270,6 +270,7 @@ F:  docs/misc/livepatch.markdown
 F:  tools/misc/xen-livepatch.c
 F:  xen/arch/*/livepatch*
 F:  xen/common/livepatch*
+F:  xen/include/asm-*/livepatch.h
 F:  xen/include/xen/livepatch*
 
 MACHINE CHECK (MCA) & RAS
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 81f4fc9..37a0860 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -318,13 +318,24 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be
   payload generation time if hypervisor function address is known. If unknown,
   the value *MUST* be zero and the hypervisor will attempt to resolve the address.
 
-* `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.
+* `new_addr` can either have a non-zero value or be zero.
+  * If there is a non-zero value, then it is the address of the function that is
+    replacing the old function and the address is recomputed during relocation.
+    The value **MUST** be the address of the new function in the payload file.
 
-* `old_size` and `new_size` contain the sizes of the respective functions in bytes.
+  * If the value is zero, then we NOPing out at the `old_addr` location
+    `new_size` bytes.
+
+* `old_size` contains the sizes of the respective `old_addr` function in bytes.
    The value of `old_size` **MUST** not be zero.
 
+* `new_size` depends on what `new_addr` contains:
+  * If `new_addr` contains an non-zero value, then `new_size` has the size of
+    the new function (which will replace the one at `old_addr`)  in bytes.
+  * 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.
 
 * `opaque` **MUST** be zero.
@@ -1087,7 +1098,8 @@ limit that calls the next trampoline.
 Please note there is a small limitation for trampolines in
 function entries: The target function (+ trailing padding) must be able
 to accomodate the trampoline. On x86 with +-2 GB relative jumps,
-this means 5 bytes are required.
+this means 5 bytes are required which means that `old_size` **MUST** be
+at least five bytes if patching in trampoline.
 
 Depending on compiler settings, there are several functions in Xen that
 are smaller (without inter-function padding).
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 56da154..d5e7174 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,11 +30,17 @@ void arch_livepatch_revive(void)
 
 int arch_livepatch_verify_func(const struct livepatch_func *func)
 {
-    /* No NOP patching yet. */
-    if ( !func->new_size )
-        return -EOPNOTSUPP;
+    /* If NOPing.. */
+    if ( !func->new_addr )
+    {
+        /* Only do up to maximum amount we can put in the ->opaque. */
+        if ( func->new_size > sizeof(func->opaque) )
+            return -EOPNOTSUPP;
 
-    if ( func->old_size < PATCH_INSN_SIZE )
+        if ( func->old_size < func->new_size )
+            return -EINVAL;
+    }
+    else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
         return -EINVAL;
 
     return 0;
@@ -43,23 +48,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)];
+    unsigned int len;
 
     old_ptr = func->old_addr;
-    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
+    len = livepatch_insn_len(func);
+    if ( !len )
+        return;
+
+    memcpy(func->opaque, old_ptr, len);
+    if ( func->new_addr )
+    {
+        int32_t val;
+
+        BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
+
+        insn[0] = 0xe9; /* Relative jump. */
+        val = func->new_addr - func->old_addr - ARCH_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, livepatch_insn_len(func));
 }
 
 /* Serialise the CPU pipeline. */
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index c9e5318..ed41f39 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -525,7 +525,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..5e04aa1
--- /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 ARCH_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 29c9b31..174af06 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -68,7 +68,17 @@ 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>
 int arch_livepatch_verify_func(const struct livepatch_func *func);
+
+static inline
+unsigned int livepatch_insn_len(const struct livepatch_func *func)
+{
+    if ( !func->new_addr )
+        return func->new_size;
+
+    return ARCH_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 v7 4/5] livepatch: Drop _jmp from arch_livepatch_[apply, revert]_jmp

[Konrad Rzeszutek Wilk]

With "livepatch: NOP if func->new_addr is zero." that name
makes no more sense as we also NOP now.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v7: New submission.
---
 xen/arch/arm/livepatch.c    | 4 ++--
 xen/arch/x86/livepatch.c    | 4 ++--
 xen/common/livepatch.c      | 4 ++--
 xen/include/xen/livepatch.h | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 755f596..7f067a0 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -21,11 +21,11 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
     return -ENOSYS;
 }
 
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
 {
 }
 
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func)
 {
 }
 
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index d5e7174..b0d81d7 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -46,7 +46,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
     return 0;
 }
 
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
 {
     uint8_t *old_ptr;
     uint8_t insn[sizeof(func->opaque)];
@@ -75,7 +75,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
     memcpy(old_ptr, insn, len);
 }
 
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func)
 {
     memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
 }
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index ed41f39..9d2e86d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1033,7 +1033,7 @@ static int apply_payload(struct payload *data)
     }
 
     for ( i = 0; i < data->nfuncs; i++ )
-        arch_livepatch_apply_jmp(&data->funcs[i]);
+        arch_livepatch_apply(&data->funcs[i]);
 
     arch_livepatch_revive();
 
@@ -1062,7 +1062,7 @@ static int revert_payload(struct payload *data)
     }
 
     for ( i = 0; i < data->nfuncs; i++ )
-        arch_livepatch_revert_jmp(&data->funcs[i]);
+        arch_livepatch_revert(&data->funcs[i]);
 
     arch_livepatch_revive();
 
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 174af06..b7f66d4 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -86,8 +86,8 @@ unsigned int livepatch_insn_len(const struct livepatch_func *func)
 int arch_livepatch_quiesce(void);
 void arch_livepatch_revive(void);
 
-void arch_livepatch_apply_jmp(struct livepatch_func *func);
-void arch_livepatch_revert_jmp(const struct livepatch_func *func);
+void arch_livepatch_apply(struct livepatch_func *func);
+void arch_livepatch_revert(const struct livepatch_func *func);
 void arch_livepatch_post_action(void);
 
 void arch_livepatch_mask(void);
-- 
2.4.11


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

[PATCH v7 5/5] 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.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
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.
    Dropped the check_fnc hook
    Added the check_fnc hook back again as "livepatch: Disallow applying
    after an revert" guarantees we won't hit the BUG_ON check.
---
 docs/misc/livepatch.markdown        | 23 +++++++++++++++++
 xen/arch/x86/test/xen_hello_world.c | 34 +++++++++++++++++++++++++
 xen/common/livepatch.c              | 50 ++++++++++++++++++++++++++++++++++++-
 xen/include/xen/livepatch_payload.h | 49 ++++++++++++++++++++++++++++++++++++
 4 files changed, 155 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 37a0860..f2ae52a 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -351,6 +351,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:
@@ -388,6 +395,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 d80963e..02f3f85 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -4,14 +4,48 @@
  */
 
 #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 const char hello_world_patch_this_fnc[] = "xen_extra_version";
 extern const char *xen_hello_world(void);
+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);
+};
+
+static void check_fnc(void)
+{
+    printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
+    BUG_ON(cnt == 0 || cnt > 2);
+}
+
+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);
+
+LIVEPATCH_UNLOAD_HOOK(check_fnc);
 
 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 9d2e86d..5f9986d 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>
 
@@ -72,7 +73,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. */
@@ -542,6 +547,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 )
     {
@@ -1032,6 +1056,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(&data->funcs[i]);
 
@@ -1064,6 +1100,18 @@ static int revert_payload(struct payload *data)
     for ( i = 0; i < data->nfuncs; i++ )
         arch_livepatch_revert(&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 v7 1/5] livepatch: Disallow applying after an revert

[Jan Beulich]

>>> On 21.09.16 at 18:57, <konrad.wilk@oracle.com> wrote:
> @@ -325,8 +327,13 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
>           * and .shstrtab. For the non-relocate we allocate and copy these
>           * via other means - and the .rel we can ignore as we only use it
>           * once during loading.
> +         *
> +         * Also ignore sections with zero size. Those can be .data, or .bss.

Or any others. Please make this apparent by adding "e.g." or some
such.

> +         *
> +         * This logic must MATCH what is done in livepatch_elf_resolve_symbols.

Instead of such a comment, is it perhaps worth making an inline
function or macro to cover the three instances where these
checks need to match up?

> @@ -374,14 +381,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
>  
>      for ( i = 1; i < elf->hdr->e_shnum; i++ )
>      {
> -        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> +        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC && elf->sec[i].sec->sh_size )

Please parenthesize the & in cases like this.

Jan


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

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

[Jan Beulich]

>>> On 21.09.16 at 18:57, <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.
> 
> We also bubble up the payload limit and this one in one #define
> called LIVEPATCH_MAX_SIZE to make it easier to find these
> arbitrary limits.
> 
> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

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

[Jan Beulich]

>>> On 21.09.16 at 18:57, <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: a) more 'struct livepatch_func' structures need
> to be present, b) we have to implement a variable size
> buffer (in the future), or c) first byte an unconditional
> branch skipping the to be disabled code (of course provided
> there are no branch targets in the middle).
> 
> 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).
> 
> And introduce a general livepatch_insn_len inline function
> that would depend on platform specific instruction size
> (for a unconditional branch). As such we also rename the
> PATCH_INSN_SIZE to ARCH_PATCH_INSN_SIZE.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v7 4/5] livepatch: Drop _jmp from arch_livepatch_[apply, revert]_jmp

[Ross Lagerwall]

On 09/21/2016 05:57 PM, Konrad Rzeszutek Wilk wrote:
> With "livepatch: NOP if func->new_addr is zero." that name
> makes no more sense as we also NOP now.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> 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 v7 1/5] livepatch: Disallow applying after an revert

[Konrad Rzeszutek Wilk]

On Thu, Sep 22, 2016 at 03:21:00AM -0600, Jan Beulich wrote:
> >>> On 21.09.16 at 18:57, <konrad.wilk@oracle.com> wrote:
> > @@ -325,8 +327,13 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >           * and .shstrtab. For the non-relocate we allocate and copy these
> >           * via other means - and the .rel we can ignore as we only use it
> >           * once during loading.
> > +         *
> > +         * Also ignore sections with zero size. Those can be .data, or .bss.
> 
> Or any others. Please make this apparent by adding "e.g." or some
> such.

<nods>
> 
> > +         *
> > +         * This logic must MATCH what is done in livepatch_elf_resolve_symbols.
> 
> Instead of such a comment, is it perhaps worth making an inline
> function or macro to cover the three instances where these
> checks need to match up?

Yes. That would be much simpler.
> 
> > @@ -374,14 +381,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >  
> >      for ( i = 1; i < elf->hdr->e_shnum; i++ )
> >      {
> > -        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> > +        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC && elf->sec[i].sec->sh_size )
> 
> Please parenthesize the & in cases like this.
> 
> Jan
> 

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

Re: [PATCH v7 1/5] livepatch: Disallow applying after an revert

[Konrad Rzeszutek Wilk]

On Thu, Sep 22, 2016 at 03:21:00AM -0600, Jan Beulich wrote:
> >>> On 21.09.16 at 18:57, <konrad.wilk@oracle.com> wrote:
> > @@ -325,8 +327,13 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >           * and .shstrtab. For the non-relocate we allocate and copy these
> >           * via other means - and the .rel we can ignore as we only use it
> >           * once during loading.
> > +         *
> > +         * Also ignore sections with zero size. Those can be .data, or .bss.
> 
> Or any others. Please make this apparent by adding "e.g." or some
> such.
> 
> > +         *
> > +         * This logic must MATCH what is done in livepatch_elf_resolve_symbols.
> 
> Instead of such a comment, is it perhaps worth making an inline
> function or macro to cover the three instances where these
> checks need to match up?
> 
> > @@ -374,14 +381,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >  
> >      for ( i = 1; i < elf->hdr->e_shnum; i++ )
> >      {
> > -        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
> > +        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC && elf->sec[i].sec->sh_size )
> 
> Please parenthesize the & in cases like this.


Thanks!

Please see the updated patch:
From 829aa410c06231906b5ee786b10ede343ac39884 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 13 Sep 2016 12:02:20 -0400
Subject: [PATCH v8] livepatch: Disallow applying after an revert

On general this is unhealthy - as the payload's .bss (definitly)
or .data (maybe) will be modified once the payload is running.

Doing an revert and then re-applying the payload with a non-pristine
.bss or .data can lead to unforseen consequences (.bss are assumed
to always contain zero value but now they may have a different value).

There is one exception - if the payload contains only one .data section
- the .livepatch.funcs, then it is OK to re-apply an revert.
We detect this rather simply (if there is one RW section and its name
is .livepatch.funcs) - but the payload may have many other RW sections
that are not used at all (such as .bss or .data sections with zero
length). To not account those we also ignore sections with sh_size
being zero.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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

v6: New submission.
v7: Use 'bool' instead of 'bool_t'
  - Ignore sh_size==0 sections as a way to make the exception check work.
  - Also add the sh_size==0 check in livepatch_elf_resolve_symbols.
  - Update comment in load move_payload to mention
    "livepatch_elf_resolve_symbols"
v8: Expand the comment
  - Parenthesize the &
  - Introduce livepatch_elf_ignore_section which does the sh_size and
    SHF_ALLOC check
---
 docs/misc/livepatch.markdown    |  7 +++++++
 xen/common/livepatch.c          | 34 +++++++++++++++++++++++++++++++---
 xen/common/livepatch_elf.c      |  3 +--
 xen/include/xen/livepatch_elf.h |  4 ++++
 4 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 89c1050..a674037 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -1061,6 +1061,13 @@ depending on the current state of data. As such it should not be attempted.
 That said we should provide hook functions so that the existing data
 can be changed during payload application.
 
+To guarantee safety we disallow re-applying an payload after it has been
+reverted. This is because we cannot guarantee that the state of .bss
+and .data to be exactly as it was during loading. Hence the administrator
+MUST unload the payload and upload it again to apply it.
+
+There is an exception to this: if the payload only has .livepatch.funcs;
+and the .data or .bss sections are of zero length.
 
 ### Inline patching
 
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 23e4d51..912729e 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -52,6 +52,8 @@ struct livepatch_build_id {
 struct payload {
     uint32_t state;                      /* One of the LIVEPATCH_STATE_*. */
     int32_t rc;                          /* 0 or -XEN_EXX. */
+    bool reverted;                       /* Whether it was reverted. */
+    bool 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. */
@@ -308,7 +310,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;
@@ -325,8 +327,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
          * and .shstrtab. For the non-relocate we allocate and copy these
          * via other means - and the .rel we can ignore as we only use it
          * once during loading.
+         *
+         * Also ignore sections with zero size. Those can be for example:
+         * data, or .bss.
          */
-        if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) )
+        if ( livepatch_elf_ignore_section(elf->sec[i].sec) )
             offset[i] = UINT_MAX;
         else if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
                    !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
@@ -374,14 +379,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
 
     for ( i = 1; i < elf->hdr->e_shnum; i++ )
     {
-        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
+        if ( !livepatch_elf_ignore_section(elf->sec[i].sec) )
         {
             void *buf;
 
             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;
 
@@ -402,6 +411,10 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
         }
     }
 
+    /* Only one RW section with non-zero size: .livepatch.funcs */
+    if ( rw_buf_cnt == 1 &&
+         !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) )
+        payload->safe_to_reapply = true;
  out:
     xfree(offset);
 
@@ -1057,6 +1070,7 @@ static int revert_payload(struct payload *data)
     list_del_rcu(&data->applied_list);
     unregister_virtual_region(&data->region);
 
+    data->reverted = true;
     return 0;
 }
 
@@ -1438,6 +1452,20 @@ 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 payload as the .data (or .bss)
+             * may not be in in pristine condition. Hence MUST unload and then
+             * apply patch again. Unless the payload has only one
+             * RW section (.livepatch.funcs).
+             */
+            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/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index cda9b27..6c7773b 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -310,8 +310,7 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf *elf)
                 break;
             }
 
-            /* Matches 'move_payload' which ignores such sections. */
-            if ( !(elf->sec[idx].sec->sh_flags & SHF_ALLOC) )
+            if ( livepatch_elf_ignore_section(elf->sec[idx].sec) )
                 break;
 
             st_value += (unsigned long)elf->sec[idx].load_addr;
diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
index 7e7c86e..c3c1595 100644
--- a/xen/include/xen/livepatch_elf.h
+++ b/xen/include/xen/livepatch_elf.h
@@ -46,6 +46,10 @@ void livepatch_elf_free(struct livepatch_elf *elf);
 int livepatch_elf_resolve_symbols(struct livepatch_elf *elf);
 int livepatch_elf_perform_relocs(struct livepatch_elf *elf);
 
+static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec)
+{
+    return ( !(sec->sh_flags & SHF_ALLOC) || sec->sh_size == 0 );
+}
 #endif /* __XEN_LIVEPATCH_ELF_H__ */
 
 /*
-- 
2.4.11


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

Re: [PATCH v7 1/5] livepatch: Disallow applying after an revert

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 09/23/16 1:56 AM >>>
>@@ -46,6 +46,10 @@ void livepatch_elf_free(struct livepatch_elf *elf);
>int livepatch_elf_resolve_symbols(struct livepatch_elf *elf);
>int livepatch_elf_perform_relocs(struct livepatch_elf *elf);
 >
>+static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec)
>+{
>+    return ( !(sec->sh_flags & SHF_ALLOC) || sec->sh_size == 0 );

With the stray blanks (and perhaps even the outer parentheses) removed,
Reviewed-by: Jan Beulich <jbeulich@suse.com>


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