[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.

On 09/16/2016 04:29 PM, Konrad Rzeszutek Wilk 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

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

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 81f4fc9..a8e70a8 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 
+  at `old_addr` (and `new_size` **MUST** not be zero).

The preceding untouched lines "...is the address of the function that is replacing the old function. The address is filled in during relocation..." are a bit stale now. Perhaps the whole paragraph needs to be replaced?

 * `old_size` and `new_size` contain the sizes of the respective functions in 
-   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).

This line is probably also stale now: "...contain the sizes of the respective functions in bytes..."

 * `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 )
 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;
+        val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
+        memcpy(&insn[1], &val, sizeof(val));
+    }
+    else
+        add_nops(&insn, len);

It probably doesn't make a difference, but I'd prefer this to be "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));

Ross Lagerwall

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.