[Xen-devel] [PATCH v3 07/18] livepatch: ARM/x86: Check displacement of old_addr and new_addr

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

If the distance is too great we are in trouble - as our relocation
distance can surely be clipped, or still have a valid width - but
cause an overflow of distance.

On various architectures the maximum displacement for a unconditional
branch/jump varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86
for 32-bit relocations is +/- 2G.

Note: On x86 we could use the 64-bit jmpq instruction which
would provide much bigger displacement to do a jump, but we would
still have issues with the new function not being able to reach
any of the old functions (as all the relocations would assume 32-bit
displacement).

On ARM the conditional branch supports even a smaller displacement
but fortunatly we are not using that.

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

---
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>

v3: New submission.
---
 docs/misc/livepatch.markdown    | 14 +++++++++++++-
 xen/arch/arm/arm64/livepatch.c  |  1 +
 xen/arch/x86/livepatch.c        |  2 +-
 xen/common/livepatch.c          |  4 ++++
 xen/include/asm-arm/livepatch.h | 11 +++++++++++
 xen/include/asm-x86/livepatch.h |  3 +++
 xen/include/xen/livepatch.h     | 22 +++++++++++++++++++---
 7 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 53b5371..52c281d 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -1099,7 +1099,7 @@ can be changed during payload application.
 The hypervisor should verify that the in-place patching would fit within
 the code or data.
 
-### Trampoline (e9 opcode)
+### Trampoline (e9 opcode), x86
 
 The e9 opcode used for jmpq uses a 32-bit signed displacement. That means
 we are limited to up to 2GB of virtual address to place the new code
@@ -1133,3 +1133,15 @@ that in the hypervisor is advised.
 The tool for generating payloads currently does perform a compile-time
 check to ensure that the function to be replaced is large enough.
 
+The hypervisor also checks the displacement during loading of the payload.
+
+#### Trampoline (ea opcode), ARM
+
+The 0xea000000 instruction (with proper offset) is used for an unconditional
+branch to the new code. This means we are limited on ARM32 to +/- 32MB
+displacement and on ARM64 to +/- 128MB displacement.
+
+The new code is placed in the 8M - 10M virtual address space while the
+Xen code is in 2M - 4M. That gives us enough space.
+
+The hypervisor also checks the displacement during loading of the payload.
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index ecaf3f6..f072671 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -40,6 +40,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
     else
         insn = aarch64_insn_gen_nop();
 
+    /* Verified in arch_livepatch_verify_distance. */
     ASSERT(insn != AARCH64_BREAK_FAULT);
 
     new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index c4d4b4d..cf3eb22 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -44,7 +44,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
 {
     uint8_t *old_ptr;
     uint8_t insn[sizeof(func->opaque)];
-    size_t len;
+    unsigned int len;
 
     old_ptr = func->old_addr;
     len = arch_livepatch_insn_len(func);
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 5baa418..4f156c2 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -530,6 +530,10 @@ static int prepare_payload(struct payload *payload,
         rc = resolve_old_address(f, elf);
         if ( rc )
             return rc;
+
+        rc = arch_livepatch_verify_distance(f);
+        if ( rc )
+            return rc;
     }
 
     sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h
index 8c8d625..a632cfa 100644
--- a/xen/include/asm-arm/livepatch.h
+++ b/xen/include/asm-arm/livepatch.h
@@ -6,6 +6,8 @@
 #ifndef __XEN_ARM_LIVEPATCH_H__
 #define __XEN_ARM_LIVEPATCH_H__
 
+#include <xen/sizes.h> /* For SZ_* macros. */
+
 /* On ARM32,64 instructions are always 4 bytes long. */
 #define PATCH_INSN_SIZE 4
 
@@ -15,6 +17,15 @@
  */
 extern void *vmap_of_xen_text;
 
+/* These ranges are only for unconditional branches. */
+#ifdef CONFIG_ARM_32
+/* ARM32: A4.3 IN ARM DDI 0406C.j -  we are using only ARM instructions in 
Xen.*/
+#define LIVEPATCH_ARCH_RANGE SZ_32M
+#else
+/* ARM64: C1.3.2 in ARM DDI 0487A.j */
+#define LIVEPATCH_ARCH_RANGE SZ_128M
+#endif
+
 #endif /* __XEN_ARM_LIVEPATCH_H__ */
 
 /*
diff --git a/xen/include/asm-x86/livepatch.h b/xen/include/asm-x86/livepatch.h
index 63ea079..0cae242 100644
--- a/xen/include/asm-x86/livepatch.h
+++ b/xen/include/asm-x86/livepatch.h
@@ -6,7 +6,10 @@
 #ifndef __XEN_X86_LIVEPATCH_H__
 #define __XEN_X86_LIVEPATCH_H__
 
+#include <xen/sizes.h> /* For SZ_* macros. */
+
 #define PATCH_INSN_SIZE 5
+#define LIVEPATCH_ARCH_RANGE SZ_2G
 
 #endif /* __XEN_X86_LIVEPATCH_H__ */
 
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 8258e02..904785a 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -12,6 +12,7 @@ struct livepatch_elf_sym;
 struct xen_sysctl_livepatch_op;
 
 #include <xen/elfstructs.h>
+#include <xen/errno.h> /* For -ENOSYS or -EOVERFLOW */
 #ifdef CONFIG_LIVEPATCH
 
 /*
@@ -66,16 +67,32 @@ 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. */
+#include <asm/livepatch.h> /* For LIVEPATCH_ARCH_RANGE and 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)
+static inline
+unsigned int arch_livepatch_insn_len(const struct livepatch_func *func)
 {
     if ( !func->new_addr )
         return func->new_size;
 
     return PATCH_INSN_SIZE;
 }
+
+static inline int arch_livepatch_verify_distance(const struct livepatch_func 
*func)
+{
+    long offset;
+    long range = (long)LIVEPATCH_ARCH_RANGE;
+
+    if ( !func->new_addr ) /* Ignore NOPs. */
+        return 0;
+
+    offset = ((long)func->old_addr - (long)func->new_addr);
+    if ( offset < -range || offset >= range )
+        return -EOVERFLOW;
+
+    return 0;
+}
 /*
  * These functions are called around the critical region patching live code,
  * for an architecture to take make appropratie global state adjustments.
@@ -100,7 +117,6 @@ void arch_livepatch_unmask(void);
 #define init_or_livepatch_data        __initdata
 #define init_or_livepatch             __init
 
-#include <xen/errno.h> /* For -ENOSYS */
 static inline int livepatch_op(struct xen_sysctl_livepatch_op *op)
 {
     return -ENOSYS;
-- 
2.4.11


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