Re: [PATCH v2 13/20] livepatch: Initial ARM64 support.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

> > +void arch_livepatch_apply_jmp(struct livepatch_func *func)
> > +{
> > +    uint32_t insn;
> > +    uint32_t *old_ptr;
> > +    uint32_t *new_ptr;
> > +
> > +    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> > +    BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
> > +
> > +    ASSERT(vmap_of_xen_text);
> > +
> > +    /* Save old one. */
> > +    old_ptr = func->old_addr;
> > +    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> 
> I don't see any value to use a temporary variable (old_ptr) to hold
> func->old_addr here.
> 
> > +
> > +    if ( func->new_addr )
> > +        insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
> > +                                           (unsigned long)func->new_addr,
> > +                                           AARCH64_INSN_BRANCH_NOLINK);
> > +    else
> > +        insn = aarch64_insn_gen_nop();
> > +
> > +    ASSERT(insn != AARCH64_BREAK_FAULT);
> 
> Could you document in the code what prevents aarch64_insn_gen_branch_imm to
> not generate a break fault instruction?

In the excitment of getting ARM32 implementation working - forgot
to write the code that would have done the check for all the platforms
(32MB for ARM, 128MB for ARM64, and 2GB on x86).

It will be an followup patch, like so (compile tested):

From 508157d81eaacab7ff621a84d7d885fb1bf689cc Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 6 Sep 2016 20:14:18 -0400
Subject: [PATCH] livepatch: ARM/x86: Check range of old_addr between new_addr

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

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

---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>

v3: New submission.
---
 xen/arch/arm/arm64/livepatch.c  |  1 +
 xen/common/livepatch.c          |  4 ++++
 xen/include/asm-arm/livepatch.h |  8 ++++++++
 xen/include/asm-x86/livepatch.h | 23 +++++++++++++++++++++++
 xen/include/xen/livepatch.h     | 18 +++++++++++++++++-
 5 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100644 xen/include/asm-x86/livepatch.h

diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index e4d2926..27a68ab 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -35,6 +35,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/common/livepatch.c b/xen/common/livepatch.c
index 86232b8..1ba6ca9 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -587,6 +587,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..b1806d0 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,12 @@
  */
 extern void *vmap_of_xen_text;
 
+#ifdef CONFIG_ARM_32
+#define LIVEPATCH_ARCH_RANGE SZ_32M
+#else
+#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
new file mode 100644
index 0000000..4b77601
--- /dev/null
+++ b/xen/include/asm-x86/livepatch.h
@@ -0,0 +1,23 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#ifndef __XEN_X86_LIVEPATCH_H__
+#define __XEN_X86_LIVEPATCH_H__
+
+#include <xen/sizes.h> /* For SZ_* macros. */
+
+#define LIVEPATCH_ARCH_RANGE SZ_2G
+
+#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 243e240..55553806 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,7 +67,23 @@ 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 LIVEPATCH_ARCH_RANGE. */
 int arch_livepatch_verify_func(const struct livepatch_func *func);
+
+static inline int arch_livepatch_verify_distance(const struct livepatch_func *func)
+{
+    long offset;
+
+    if ( !func->new_addr ) /* Ignore NOPs. */
+        return 0;
+
+    offset = ((long)func->old_addr - (long)func->new_addr);
+    if ( offset < -LIVEPATCH_ARCH_RANGE || offset >= LIVEPATCH_ARCH_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.
@@ -91,7 +108,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

...snip..
> > +        case R_AARCH64_ABS32:
> > +            ovf = reloc_data(RELOC_OP_ABS, dest, val, 32);
> > +            break;
> 
> I have noticed that not all the relocations are implemented (e.g
> R_AARCH64_ABS16, R_AARCH64_MOVW_*...). I don't think there is anything
> preventing the compiler to use them. So is there any particular reasons to
> not include them?

I followed the same principle Ross did on x86 - only implement the ones that
the compiler has generated. And that is what I initially had in the v1, but expanded
it per request.

I can include more, but at what point should I stop?

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