[Xen-devel] [PATCH v4 04/11] livepatch/arm[32, 64]: Don't load and crash on livepatches loaded with wrong text alignment.

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

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>
---
Cc: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: George Dunlap <George.Dunlap@xxxxxxxxxxxxx>
Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Tim Deegan <tim@xxxxxxx>
Cc: Wei Liu <wei.liu2@xxxxxxxxxx>

v1: First posting.
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.
v4: Also do the check on ARM64
    Put () around the section check
    Use vaddr_t instead of uint32_t as that won't work on ARM64.
---
 docs/misc/livepatch.markdown   |  2 ++
 xen/arch/arm/arm32/livepatch.c | 13 +++++++++++--
 xen/arch/arm/livepatch.c       |  9 +++++++++
 xen/arch/x86/livepatch.c       |  6 ++++++
 xen/common/livepatch.c         |  7 +++++++
 xen/include/xen/livepatch.h    |  1 +
 6 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 54a6b850cb..59f89aa292 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 & 64 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..4fcbb59be5 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -233,7 +233,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 +251,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 +271,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/livepatch.c b/xen/arch/arm/livepatch.c
index 3e53524365..76723f1f1a 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -135,6 +135,15 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf 
*elf,
     return true;
 }
 
+bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec)
+{
+    if ( (sec->sec->sh_flags & SHF_EXECINSTR) &&
+         ((vaddr_t)sec->load_addr % sizeof(uint32_t)) )
+        return false;
+
+    return true;
+};
+
 int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type 
type)
 {
     unsigned long start = (unsigned long)va;
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 b9376c94e9..f736c3a7ea 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)
 {
-- 
2.13.3


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