[Xen-devel] [PATCH v4 7/9] livepatch: NOP if func->new_[addr] is zero.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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.
But on x86 we could NOP instructions that are much
shorter or longer (up to 15 bytes).

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.

If `name` is provided with the 'symbol+0x<offset' format
we figure out the EIP and then NOP 'new_size' amount of bytes
at the location.

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@xxxxxxxxxx>

---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Cc: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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.
---
 docs/misc/livepatch.markdown      |  7 +++++--
 xen/arch/x86/alternative.c        |  2 +-
 xen/arch/x86/livepatch.c          | 44 ++++++++++++++++++++++++++++++---------
 xen/common/livepatch.c            |  3 ++-
 xen/include/asm-x86/alternative.h |  1 +
 5 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 7e82047..ad0df26 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
+   platform limitations).
 
 * `version` is to be one.
 
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index be40b13..fd8528e 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 1023fab..952e897 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -14,6 +14,7 @@
 #include <asm/nmi.h>
 
 #define PATCH_INSN_SIZE 5
+#define MAX_INSN_SIZE 15
 
 void arch_livepatch_quiesce(void)
 {
@@ -29,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 instruction size. */
+    if ( !func->new_addr && func->new_size > MAX_INSN_SIZE )
         return -EOPNOTSUPP;
 
     if ( func->old_size < PATCH_INSN_SIZE )
@@ -39,25 +40,48 @@ int arch_livepatch_verify_func(const struct livepatch_func 
*func)
     return 0;
 }
 
+static size_t get_len(const struct livepatch_func *func)
+{
+    if ( !func->new_addr )
+        return func->new_size;
+
+    return PATCH_INSN_SIZE;
+}
+
 void arch_livepatch_apply_jmp(struct livepatch_func *func)
 {
-    int32_t val;
     uint8_t *old_ptr;
+    uint8_t insn[MAX_INSN_SIZE];
+    size_t len;
 
-    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
-    BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
+    BUILD_BUG_ON(MAX_INSN_SIZE > sizeof(func->opaque));
 
     old_ptr = func->old_addr;
-    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
+    len = get_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, get_len(func));
 }
 
 /* Serialise the CPU pipeline. */
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index c234424..dd24a07 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -566,7 +566,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 bce959f..acaeded 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -23,6 +23,7 @@ struct alt_instr {
     u8  replacementlen;     /* length of new instruction, <= instrlen */
 };
 
+extern void add_nops(void *insns, unsigned int len);
 /* Similar to apply_alternatives except it can be run with IRQs enabled. */
 extern void apply_alternatives_nocheck(struct alt_instr *start,
                                        struct alt_instr *end);
-- 
2.4.11


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