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

Re: [Xen-devel] [PATCH v3 04/17] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong text alignment.



Hi Konrad,

On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote:
The ARM 32&64 ELF specification says "sections containing ARM
code must be at least 32-bit aligned." This patch adds the
check for that. We also make sure that this check is done
when doing relocations for the types that are considered
ARM code. However we don't have to check for all as we only
implement a small subset of them - as such we only check for
data types that are implemented - and if the type is anything else
and not aligned to 32-bit, then we error out.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
---
v1: Initial patch
v2: Redo the commit to include the commits which fix the alignment issues.
     Also mention the need in the docs
v3: Change the docs to explicitly mention text code section alignment 
requirements.
     Invert arch_livepatch_verify_alignment return value (true for alignment is 
ok).
     Drop the alignment check in check_special_sections.
     Make the alignment check in check_section only for executable sections.
     Rewrote the commit message as it is not applicable to v2 of the patch 
anymore.
---
  docs/misc/livepatch.markdown   |  2 ++
  xen/arch/arm/arm32/livepatch.c | 22 ++++++++++++++++++++--
  xen/arch/arm/arm64/livepatch.c |  6 ++++++
  xen/arch/x86/livepatch.c       |  6 ++++++
  xen/common/livepatch.c         |  7 +++++++
  xen/include/xen/livepatch.h    |  1 +
  6 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 54a6b850cb..505dc37cda 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -279,6 +279,8 @@ It may also have some architecture-specific sections. For 
example:
   * Exception tables.
   * Relocations for each of these sections.
+Note that on ARM 32 the sections containing code MUST be four byte aligned.
+
  The Xen Live Patch core code loads the payload as a standard ELF binary, 
relocates it
  and handles the architecture-specifc sections as needed. This process is much
  like what the Linux kernel module loader does.
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 41378a54ae..10887ace81 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -112,6 +112,15 @@ bool arch_livepatch_symbol_deny(const struct livepatch_elf 
*elf,
      return false;
  }
+bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec)
+{
+    if ( sec->sec->sh_flags & SHF_EXECINSTR &&

NIT: May I ask to add (...) here?

+         ((uint32_t)sec->load_addr % sizeof(uint32_t)) )
+        return false;
+
+    return true;
+};
+
  static s32 get_addend(unsigned char type, void *dest)
  {
      s32 addend = 0;
@@ -233,7 +242,7 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
          uint32_t val;
          void *dest;
          unsigned char type;
-        s32 addend;
+        s32 addend = 0;
if ( use_rela )
          {
@@ -251,7 +260,6 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
              symndx = ELF32_R_SYM(r->r_info);
              type = ELF32_R_TYPE(r->r_info);
              dest = base->load_addr + r->r_offset; /* P */
-            addend = get_addend(type, dest);
          }
if ( symndx == STN_UNDEF )
@@ -272,6 +280,16 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
                      elf->name, symndx);
              return -EINVAL;
          }
+        else if ( (type != R_ARM_ABS32 && type != R_ARM_REL32) /* Only check code. */ 
&&
+                  ((uint32_t)dest % sizeof(uint32_t)) )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: dest=%p (%s) is not aligned 
properly!\n",
+                    elf->name, dest, base->name);
+            return -EINVAL;
+        }
+
+        if ( !use_rela )
+            addend = get_addend(type, dest);
val = elf->sym[symndx].sym->st_value; /* S */ diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 2247b925a0..2728e2a125 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -86,6 +86,12 @@ bool arch_livepatch_symbol_deny(const struct livepatch_elf 
*elf,
      return false;
  }
+bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec)
+{
+    /* Unaligned access on ARM 64 is OK. */

This function will be called on every section, right? If so, the text segment still have to be aligned on Arm64. Instruction can not be unaligned.

The unaligned access is only OK for data section. And the only reason is because the memcpy implementation is performing unaligned access. It seems to have better performance on some core.

+    return true;
+}
+
  enum aarch64_reloc_op {
      RELOC_OP_NONE,
      RELOC_OP_ABS,
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 406eb910cc..48d20fdacd 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -148,6 +148,12 @@ bool arch_livepatch_symbol_deny(const struct livepatch_elf 
*elf,
      return false;
  }
+bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec)
+{
+    /* Unaligned access on x86 is fine. */
+    return true;
+}
+
  int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                 const struct livepatch_elf_sec *base,
                                 const struct livepatch_elf_sec *rela)
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index c6ee95fbcf..dbab8a3f6f 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -473,6 +473,13 @@ static bool section_ok(const struct livepatch_elf *elf,
          return false;
      }
+ if ( !arch_livepatch_verify_alignment(sec) )
+    {
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: %s text section is not aligned 
properly!\n",
+               elf->name, sec->name);
+        return false;
+    }
+
      return true;
  }
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 98ec01216b..e9bab87f28 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -76,6 +76,7 @@ void arch_livepatch_init(void);
  #include <asm/livepatch.h>
  int arch_livepatch_verify_func(const struct livepatch_func *func);
+bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec);
  static inline
  unsigned int livepatch_insn_len(const struct livepatch_func *func)
  {


Cheers,

--
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®.