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

Re: [Xen-devel] [PATCH v3 09/17] livepatch/arm[32, 64]: Modify livepatch_funcs



Hi Konrad,

On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote:
This was found when porting livepatch-build-tools to ARM64/32.

When livepatch-build-tools are built (and test-case thanks to:
livepatch/tests: Make sure all .livepatch.funcs sections are read-only)
the .livepatch.funcs are in read-only section.

However the hypervisor uses the 'opaque' for its own purpose, that
is stashing the original code. But the .livepatch_funcs section is
in the RO vmap area so on ARM[32,64] we get a fault.

This is because the payload is secure at loading and therefore before it get applied, right?

I was wondering if we could either defer the call to secure_payload or make the region temporarily writeable?


On x86 the same protection is in place. In 'arch_livepatch_quiesce'
we disable WP to allow changes to read-only pages (and in arch_live_resume

I can't find any function call arch_live_resume in Xen code. Do you mean arch_livepatch_revive?

we enable the WP protection).

On ARM[32,64] we do not have the luxury of a global register that can
be changed after boot. In lieu of that we use the vmap to create
a temporary virtual address in which we can use instead.

To do this we need to stash during livepatch: vmap of the hypervisor
code, vmap of the .livepatch_funcs (vmap comes in page aligned virtual
addresses), offset in the vmap (in case it is not nicely aligned), and
the original first livepatch_funcs to figure out the index.

Equipped with that we can patch livepatch functions which have
  .livepatch_funcs in rodata section.

An alternative is to add the 'W' flag during loading of the
.livepatch_funcs which would result the section being in writeable
region from the gecko. >
Note that this vmap solution could be extended to x86 as well.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
---
  xen/arch/arm/arm32/livepatch.c  | 11 ++++++---
  xen/arch/arm/arm64/livepatch.c  | 11 ++++++---
  xen/arch/arm/livepatch.c        | 52 ++++++++++++++++++++++++++++++++---------
  xen/arch/x86/livepatch.c        |  2 +-
  xen/common/livepatch.c          |  5 ++--
  xen/include/asm-arm/livepatch.h | 13 ++++++++---
  xen/include/xen/livepatch.h     |  2 +-
  7 files changed, 71 insertions(+), 25 deletions(-)

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 10887ace81..d793ebcaad 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -16,18 +16,23 @@ void arch_livepatch_apply(struct livepatch_func *func)
      uint32_t insn;
      uint32_t *new_ptr;
      unsigned int i, len;
+    struct livepatch_func *f;
BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque));
      BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn));
- ASSERT(vmap_of_xen_text);
+    ASSERT(livepatch_vmap.text);
len = livepatch_insn_len(func);
      if ( !len )
          return;
+ /* Index in the vmap region. */
+    i = livepatch_vmap.va - func;
+    f = (struct livepatch_func *)(livepatch_vmap.funcs + 
livepatch_vmap.offset) + i;
+
      /* Save old ones. */
-    memcpy(func->opaque, func->old_addr, len);
+    memcpy(f->opaque, func->old_addr, len);
if ( func->new_addr )
      {
@@ -56,7 +61,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
      else
          insn = 0xe1a00000; /* mov r0, r0 */
- new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+    new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text;
      len = len / sizeof(uint32_t);
/* PATCH! */
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 2728e2a125..662bedabc3 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -20,18 +20,23 @@ void arch_livepatch_apply(struct livepatch_func *func)
      uint32_t insn;
      uint32_t *new_ptr;
      unsigned int i, len;
+    struct livepatch_func *f;
BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque));
      BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn));
- ASSERT(vmap_of_xen_text);
+    ASSERT(livepatch_vmap.text);
len = livepatch_insn_len(func);
      if ( !len )
          return;
+ /* Index in the vmap region. */
+    i = livepatch_vmap.va - func;
+    f = (struct livepatch_func *)(livepatch_vmap.funcs + 
livepatch_vmap.offset) + i;
+
      /* Save old ones. */
-    memcpy(func->opaque, func->old_addr, len);
+    memcpy(f->opaque, func->old_addr, len);
if ( func->new_addr )
          insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
@@ -43,7 +48,7 @@ void arch_livepatch_apply(struct livepatch_func *func)
      /* Verified in livepatch_verify_distance. */
      ASSERT(insn != AARCH64_BREAK_FAULT);
- new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+    new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text;
      len = len / sizeof(uint32_t);
/* PATCH! */
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 3e53524365..2f9ae8e61e 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -6,6 +6,7 @@
  #include <xen/lib.h>
  #include <xen/livepatch_elf.h>
  #include <xen/livepatch.h>
+#include <xen/pfn.h>
  #include <xen/vmap.h>
#include <asm/cpufeature.h>
@@ -16,14 +17,18 @@
  #undef virt_to_mfn
  #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
-void *vmap_of_xen_text;
+struct livepatch_vmap_stash livepatch_vmap;
-int arch_livepatch_quiesce(void)
+int arch_livepatch_quiesce(struct livepatch_func *funcs, unsigned int nfuncs)
  {
-    mfn_t text_mfn;
+    mfn_t text_mfn, rodata_mfn;
+    void *vmap_addr;
      unsigned int text_order;
+    unsigned long va = (unsigned long)(funcs);
+    unsigned int offs = va & (PAGE_SIZE - 1);
+    unsigned int size = PFN_UP(offs + nfuncs * sizeof(*funcs));
- if ( vmap_of_xen_text )
+    if ( livepatch_vmap.text || livepatch_vmap.funcs )
          return -EINVAL;
text_mfn = virt_to_mfn(_start);
@@ -33,16 +38,33 @@ int arch_livepatch_quiesce(void)
       * The text section is read-only. So re-map Xen to be able to patch
       * the code.
       */
-    vmap_of_xen_text = __vmap(&text_mfn, 1U << text_order, 1, 1, 
PAGE_HYPERVISOR,
-                              VMAP_DEFAULT);
+    vmap_addr = __vmap(&text_mfn, 1U << text_order, 1, 1, PAGE_HYPERVISOR,
+                       VMAP_DEFAULT);
- if ( !vmap_of_xen_text )
+    if ( !vmap_addr )
      {
          printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor! 
(order=%u)\n",
                 text_order);
          return -ENOMEM;
      }
+ livepatch_vmap.text = vmap_addr;
+    livepatch_vmap.offset = offs;
+
+    rodata_mfn = virt_to_mfn(va & PAGE_MASK);
+    vmap_addr  = __vmap(&rodata_mfn, size, 1, 1, PAGE_HYPERVISOR, 
VMAP_DEFAULT);
+    if ( !vmap_addr )
+    {
+        printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of livepatch_funcs! 
(mfn=%"PRI_mfn", size=%u)\n",
+               mfn_x(rodata_mfn), size);
+        vunmap(livepatch_vmap.text);
+        livepatch_vmap.text = NULL;
+        return -ENOMEM;
+    }
+
+    livepatch_vmap.funcs = vmap_addr;
+    livepatch_vmap.va = funcs;
+
      return 0;
  }
@@ -54,10 +76,18 @@ void arch_livepatch_revive(void)
       */
      invalidate_icache();
- if ( vmap_of_xen_text )
-        vunmap(vmap_of_xen_text);
+    if ( livepatch_vmap.text )
+        vunmap(livepatch_vmap.text);
+
+    livepatch_vmap.text = NULL;
+
+    if ( livepatch_vmap.funcs )
+        vunmap(livepatch_vmap.funcs);
+
+    livepatch_vmap.funcs = NULL;
- vmap_of_xen_text = NULL;
+    livepatch_vmap.va = NULL;
+    livepatch_vmap.offset = 0;
  }
int arch_livepatch_verify_func(const struct livepatch_func *func)
@@ -78,7 +108,7 @@ void arch_livepatch_revert(const struct livepatch_func *func)
      uint32_t *new_ptr;
      unsigned int len;
- new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
+    new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text;
len = livepatch_insn_len(func);
      memcpy(new_ptr, func->opaque, len);
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 48d20fdacd..8522fcbd36 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -14,7 +14,7 @@
  #include <asm/nmi.h>
  #include <asm/livepatch.h>
-int arch_livepatch_quiesce(void)
+int arch_livepatch_quiesce(struct livepatch_func *func, unsigned int nfuncs)
  {
      /* Disable WP to allow changes to read-only pages. */
      write_cr0(read_cr0() & ~X86_CR0_WP);
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index dbab8a3f6f..e707802279 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -571,7 +571,6 @@ static int prepare_payload(struct payload *payload,
          if ( rc )
              return rc;
      }
-
      sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
      if ( sec )
      {
@@ -1070,7 +1069,7 @@ static int apply_payload(struct payload *data)
      printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
              data->name, data->nfuncs);
- rc = arch_livepatch_quiesce();
+    rc = arch_livepatch_quiesce(data->funcs, data->nfuncs);
      if ( rc )
      {
          printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
@@ -1111,7 +1110,7 @@ static int revert_payload(struct payload *data)
printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name); - rc = arch_livepatch_quiesce();
+    rc = arch_livepatch_quiesce(data->funcs, data->nfuncs);
      if ( rc )
      {
          printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h
index 6bca79deb9..e030aedced 100644
--- a/xen/include/asm-arm/livepatch.h
+++ b/xen/include/asm-arm/livepatch.h
@@ -12,10 +12,17 @@
  #define ARCH_PATCH_INSN_SIZE 4
/*
- * The va of the hypervisor .text region. We need this as the
- * normal va are write protected.
+ * The va of the hypervisor .text region and the livepatch_funcs.
+ * We need this as the normal va are write protected.
   */
-extern void *vmap_of_xen_text;
+struct livepatch_vmap_stash {
+       void *text;                 /* vmap of hypervisor code. */
+       void *funcs;                /* vmap of the .livepatch.funcs. */
+       unsigned int offset;        /* Offset in 'funcs'. */
+       struct livepatch_func *va;  /* The original va. */
+};
+
+extern struct livepatch_vmap_stash livepatch_vmap;
/* These ranges are only for unconditional branches. */
  #ifdef CONFIG_ARM_32
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index e9bab87f28..a97afb92f9 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -104,7 +104,7 @@ static inline int livepatch_verify_distance(const struct 
livepatch_func *func)
   * These functions are called around the critical region patching live code,
   * for an architecture to take make appropratie global state adjustments.
   */
-int arch_livepatch_quiesce(void);
+int arch_livepatch_quiesce(struct livepatch_func *func, unsigned int nfuncs);
  void arch_livepatch_revive(void);
void arch_livepatch_apply(struct livepatch_func *func);


--
Julien Grall

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

 


Rackspace

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