livepatch: NOP if func->new_addr is zero.

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.
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

diff --git a/MAINTAINERS b/MAINTAINERS
index 97720a82b88792464ae79552d0e8091f0991a0f4..9b3060085af37459ed75a1a24c6c125592808c00 100644 (file)
--- 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 a674037e1566e8c1396f2f2c308095eed2a2fb28..f22eeabe626cf9a409e0de04716b63fe177351a9 100644 (file)
--- 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 05e3eb8cf87b3e07e2e4c430baaf928aeb6f16c9..6eaa10f858e57d95accfb59e342639086bbd861c 100644 (file)
--- 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 56da1545f2f53f949fea7c8372985221dfbd3b84..d5e7174fbcbbbf3b288f48c8830477f0984b021b 100644 (file)
--- 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 f5ce28c7263acca3be009976933536f0fc260660..13346d7956850362beefc691beceed84b253d288 100644 (file)
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -523,7 +523,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 67fc0d24185444c43ce6e076ad2e20ca2f4d34cb..db4f08e0e7e40c24fa47ced1bc457b8e2b220b94 100644 (file)
--- 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 (file)
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 29c9b3141b2dd729356f728daaf1dd71cb81304f..174af061039067c7ddb7f77a24ac3f58b1dc908c 100644 (file)
--- 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.