[PATCH v4] Livepatching patch set for 4.10

[PATCH v4] Livepatching patch set for 4.10

[Konrad Rzeszutek Wilk]

Hey,

As I was trying to port livepatch-build-tools.git to work under ARM32 and ARM64
(still ongoing, if somebody wants to help/take over would appreciate it)
I found some inconsistencies compared to the x86 and test-cases:

 - The .livepatch.funcs in the test-cases are in RW section but the livepatch-build-tools
   puts them in the RO sections. That works on x86 as arch_livepatch_quiesce
   turns of WP globally during the livepatching.
   But not on ARM.. and to make it work there I ended up using modify_xen_mappings

 - Cross compiling ARM32 introduces subtle alignment issues. Mainly
   both .altinstructions and .livepatch.depends end up with the
   wrong alingment and the hypervisor blows up. Both fixes are
   in the patchset.

I am also including in this patchset:

 - Declare the livepatch supported on x86 (but not ARM). I will
   shortly check it in if nobody screams.


I am NOT including in this patchset:

 - The local/global symbol patchset functionality. I am still
   working on Jan's feedback to use STV_INTERNAL.

Lastly, I tested this on ARM32 (Cubietruck), ARM64 (HiKey960) and
on x86.

Patches are in 

  git://xenbits.xen.org/people/konradwilk/xen.git staging-for-4.10.v4

Konrad Rzeszutek Wilk (10):
      livepatch: Expand check for safe_for_reapply if livepatch has only .rodata.
      livepatch: Tighten alignment checks.
      livepatch: Include sizes when an mismatch occurs
      livepatch/arm[32,64]: Don't load and crash on livepatches loaded with wrong text alignment.
      alternative/x86/arm32: Align altinstructions (and altinstr_replacement) sections.
      mkhex: Move it to tools/misc
      livepatch/x86/arm[32,64]: Force .livepatch.depends section to be uint32_t aligned.
      livepatch/arm/x86: Rename note_depends symbol from test-cases.
      livepatch/tests: Make sure all .livepatch.funcs sections are read-only
      livepatch/arm[32,64]: Modify .livepatch.funcs section to be RW when patching

Ross Lagerwall (1):
      livepatch: Declare live patching as a supported feature

 docs/features/livepatch.pandoc           | 106 +++++++++++++++++++++++++++++++
 docs/misc/livepatch.markdown             |   4 ++
 tools/firmware/hvmloader/Makefile        |   8 +--
 tools/{firmware/hvmloader => misc}/mkhex |   0
 xen/arch/arm/arm32/livepatch.c           |  13 +++-
 xen/arch/arm/kernel.c                    |   2 +-
 xen/arch/arm/livepatch.c                 |  54 +++++++++++++++-
 xen/arch/arm/mm.c                        |  28 ++++----
 xen/arch/arm/platforms/vexpress.c        |   2 +-
 xen/arch/arm/xen.lds.S                   |   1 -
 xen/arch/x86/livepatch.c                 |   8 ++-
 xen/arch/x86/xen.lds.S                   |   2 +-
 xen/common/Kconfig                       |   4 +-
 xen/common/livepatch.c                   |  71 ++++++++++++---------
 xen/common/livepatch_elf.c               |  13 ++++
 xen/drivers/video/arm_hdlcd.c            |   2 +-
 xen/include/asm-arm/alternative.h        |   4 ++
 xen/include/asm-arm/livepatch.h          |  13 ++++
 xen/include/asm-arm/page.h               |  51 ++++++++-------
 xen/include/asm-x86/alternative.h        |   2 +
 xen/include/xen/elfstructs.h             |   2 +
 xen/include/xen/livepatch.h              |   3 +-
 xen/test/livepatch/Makefile              |  64 ++++++++++---------
 xen/test/livepatch/xen_bye_world.c       |   1 +
 xen/test/livepatch/xen_hello_world.c     |   1 +
 xen/test/livepatch/xen_nop.c             |   1 +
 xen/test/livepatch/xen_replace_world.c   |   1 +
 27 files changed, 347 insertions(+), 114 deletions(-)


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

[PATCH v4 01/11] livepatch: Expand check for safe_for_reapply if livepatch has only .rodata.

[Konrad Rzeszutek Wilk]

If the livepatch has only .rodata sections then it is OK to also
apply/revert/apply the livepatch without having to worry about the
unforseen consequences.

See commit 98b728a7b235c67e210f67f789db5d9eb38ca00c
"livepatch: Disallow applying after an revert" for details.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>

v3: First posting.
---
 xen/common/livepatch.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 66167a5573..b0dcd415ba 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -417,9 +417,12 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
         }
     }
 
-    /* Only one RW section with non-zero size: .livepatch.funcs */
-    if ( rw_buf_cnt == 1 &&
-         !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) )
+    /*
+     * Only one RW section with non-zero size: .livepatch.funcs,
+     * or only RO sections.
+     */
+    if ( !rw_buf_cnt || (rw_buf_cnt == 1 &&
+         !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC)) )
         payload->safe_to_reapply = true;
  out:
     xfree(offset);
-- 
2.13.3


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

[PATCH v4 02/11] livepatch: Tighten alignment checks.

[Konrad Rzeszutek Wilk]

The ELF specification mentions nothing about the sh_size being
modulo the sh_addralign. Only that sh_addr MUST be aligned on
sh_addralign if sh_addralign is not zero or one.

We on loading did not take this in-to account so this patch adds
a check on the ELF file as it is being parsed.

Reviewed-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>

v1: Initial patch
v2: Drop the check when loading it in memory
    Add check for alignment being anything but power of two (ignoring 0, and 1)
    Change dprintk to include hex values and print addr not size.
v3: Change the two checks to be per Jan's recommendations.
v4: Add Jan's Reviewed-by.
    Swap the two conditionals around.
---
 xen/common/livepatch_elf.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index b69e2718dd..dd8b47a1fa 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -86,6 +86,19 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
                     delta < sizeof(Elf_Ehdr) ? "at ELF header" : "is past end");
             return -EINVAL;
         }
+        else if ( sec[i].sec->sh_addralign & (sec[i].sec->sh_addralign - 1) )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section [%u] alignment (%#"PRIxElfAddr") is not supported\n",
+                    elf->name, i, sec[i].sec->sh_addralign);
+            return -EOPNOTSUPP;
+        }
+        else if ( sec[i].sec->sh_addralign &&
+                  sec[i].sec->sh_addr % sec[i].sec->sh_addralign )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section [%u] addr (%#"PRIxElfAddr") is not aligned properly (%#"PRIxElfAddr")\n",
+                    elf->name, i, sec[i].sec->sh_addr, sec[i].sec->sh_addralign);
+            return -EINVAL;
+        }
         else if ( (sec[i].sec->sh_flags & (SHF_WRITE | SHF_ALLOC)) &&
                   sec[i].sec->sh_type == SHT_NOBITS &&
                   sec[i].sec->sh_size > LIVEPATCH_MAX_SIZE )
-- 
2.13.3


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

[PATCH v4 03/11] livepatch: Include sizes when an mismatch occurs

[Konrad Rzeszutek Wilk]

If the .bug.frames.X or .livepatch.funcs sizes are different
than what the hypervisor expects - we fail the payload. To help
in diagnosing this include the expected and the payload
sizes.

Also make it more natural by having "Multiples" in the warning.

Also fix one case where we would fail if the size of the .ex_table
was being zero - but that is OK.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>

v1: First posting.
v2: - Changed to 'Multiple' per Jan's recommendation.
    - Folded the checks in 'check_size' function and removed all the other
      parts of code that checked for this.
v3: - Drop bool zero_ok
    - Return bool instead of int (and invert the return condition)
    - Change name of the function to be more clear
---
 xen/common/livepatch.c       | 46 +++++++++++++++++++++-----------------------
 xen/include/xen/elfstructs.h |  2 ++
 2 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index b0dcd415ba..b9376c94e9 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -460,6 +460,22 @@ static int secure_payload(struct payload *payload, struct livepatch_elf *elf)
     return rc;
 }
 
+static bool section_ok(const struct livepatch_elf *elf,
+                       const struct livepatch_elf_sec *sec, size_t sz)
+{
+    if ( !elf || !sec )
+        return false;
+
+    if ( sec->sec->sh_size % sz )
+    {
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: Wrong size %"PRIuElfWord" of %s (must be multiple of %zu)\n",
+                elf->name, sec->sec->sh_size, sec->name, sz);
+        return false;
+    }
+
+    return true;
+}
+
 static int check_special_sections(const struct livepatch_elf *elf)
 {
     unsigned int i;
@@ -509,12 +525,8 @@ static int prepare_payload(struct payload *payload,
 
     sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_FUNC);
     ASSERT(sec);
-    if ( sec->sec->sh_size % sizeof(*payload->funcs) )
-    {
-        dprintk(XENLOG_ERR, LIVEPATCH "%s: Wrong size of "ELF_LIVEPATCH_FUNC"!\n",
-                elf->name);
+    if ( !section_ok(elf, sec, sizeof(*payload->funcs)) )
         return -EINVAL;
-    }
 
     payload->funcs = sec->load_addr;
     payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);
@@ -556,7 +568,7 @@ static int prepare_payload(struct payload *payload,
     sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
     if ( sec )
     {
-        if ( sec->sec->sh_size % sizeof(*payload->load_funcs) )
+        if ( !section_ok(elf, sec, sizeof(*payload->load_funcs)) )
             return -EINVAL;
 
         payload->load_funcs = sec->load_addr;
@@ -566,7 +578,7 @@ static int prepare_payload(struct payload *payload,
     sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.unload");
     if ( sec )
     {
-        if ( sec->sec->sh_size % sizeof(*payload->unload_funcs) )
+        if ( !section_ok(elf, sec, sizeof(*payload->unload_funcs)) )
             return -EINVAL;
 
         payload->unload_funcs = sec->load_addr;
@@ -637,12 +649,8 @@ static int prepare_payload(struct payload *payload,
         if ( !sec )
             continue;
 
-        if ( sec->sec->sh_size % sizeof(*region->frame[i].bugs) )
-        {
-            dprintk(XENLOG_ERR, LIVEPATCH "%s: Wrong size of .bug_frames.%u!\n",
-                    elf->name, i);
+        if ( !section_ok(elf, sec, sizeof(*region->frame[i].bugs)) )
             return -EINVAL;
-        }
 
         region->frame[i].bugs = sec->load_addr;
         region->frame[i].n_bugs = sec->sec->sh_size /
@@ -655,12 +663,8 @@ static int prepare_payload(struct payload *payload,
 #ifdef CONFIG_HAS_ALTERNATIVE
         struct alt_instr *a, *start, *end;
 
-        if ( sec->sec->sh_size % sizeof(*a) )
-        {
-            dprintk(XENLOG_ERR, LIVEPATCH "%s: Size of .alt_instr is not multiple of %zu!\n",
-                    elf->name, sizeof(*a));
+        if ( !section_ok(elf, sec, sizeof(*a)) )
             return -EINVAL;
-        }
 
         start = sec->load_addr;
         end = sec->load_addr + sec->sec->sh_size;
@@ -692,14 +696,8 @@ static int prepare_payload(struct payload *payload,
 #ifdef CONFIG_HAS_EX_TABLE
         struct exception_table_entry *s, *e;
 
-        if ( !sec->sec->sh_size ||
-             (sec->sec->sh_size % sizeof(*region->ex)) )
-        {
-            dprintk(XENLOG_ERR, LIVEPATCH "%s: Wrong size of .ex_table (exp:%lu vs %lu)!\n",
-                    elf->name, sizeof(*region->ex),
-                    sec->sec->sh_size);
+        if ( !section_ok(elf, sec, sizeof(*region->ex)) )
             return -EINVAL;
-        }
 
         s = sec->load_addr;
         e = sec->load_addr + sec->sec->sh_size;
diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
index 950e1492e5..726ca8f60d 100644
--- a/xen/include/xen/elfstructs.h
+++ b/xen/include/xen/elfstructs.h
@@ -555,6 +555,7 @@ typedef struct {
 
 #if defined(ELFSIZE) && (ELFSIZE == 32)
 #define PRIxElfAddr	"08x"
+#define PRIuElfWord	"8u"
 
 #define Elf_Ehdr	Elf32_Ehdr
 #define Elf_Phdr	Elf32_Phdr
@@ -582,6 +583,7 @@ typedef struct {
 #define AuxInfo		Aux32Info
 #elif defined(ELFSIZE) && (ELFSIZE == 64)
 #define PRIxElfAddr	PRIx64
+#define PRIuElfWord	PRIu64
 
 #define Elf_Ehdr	Elf64_Ehdr
 #define Elf_Phdr	Elf64_Phdr
-- 
2.13.3


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/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]

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@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>

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@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v4 05/11] alternative/x86/arm32: Align altinstructions (and altinstr_replacement) sections.

[Konrad Rzeszutek Wilk]

This is very similar to 137c59b9ff3f7a214f03b52d9c00a0a02374af1f
"bug/x86/arm: Align bug_frames sections."

On ARM and on x86 the C and assembler macros don't include
any alignment information - hence they end up being the default
byte granularity.

On ARM32 it is paramount that the alignment is word-size (4)
otherwise if one tries to use (uint32_t*) access (such
as livepatch ELF relocations) we get a Data Abort.

Specifically this issue was observed on ARM32 with a cross compiler for
the livepatches. Mainly the livepatches .data section size was not
padded to the section alignment:

ARM32 native:
Contents of section .rodata:
 0000 68695f66 756e6300 63686563 6b5f666e  hi_func.check_fn
 0010 63000000 78656e5f 65787472 615f7665  c...xen_extra_ve
 0020 7273696f 6e000000                    rsion...

ARM32 cross compiler:
Contents of section .rodata:
 0000 68695f66 756e6300 63686563 6b5f666e  hi_func.check_fn
 0010 63000000 78656e5f 65787472 615f7665  c...xen_extra_ve
 0020 7273696f 6e00                        rsion.

And when we loaded it the next section would be put right behind it:

native:

(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .text at 00a02000
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .rodata at 00a04024
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .altinstructions at 00a0404c

cross compiler:
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .text at 00a02000
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .rodata at 00a04024
(XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .altinstructions at 00a0404a

(See 4a vs 4c)

native readelf:
  [ 4] .rodata           PROGBITS        00000000 000164 000028 00   A  0   0  4
  [ 5] .altinstructions  PROGBITS        00000000 00018c 00000c 00   A  0   0  1

cross compiler readelf --sections:
  [ 4] .rodata           PROGBITS        00000000 000164 000026 00   A  0   0  4
  [ 5] .altinstructions  PROGBITS        00000000 00018a 00000c 00   A  0   0  1

And as can be seen the .altinstructions have alignment of 1 which from
'man elf' is: "Values of zero and one mean no alignment is required."
which means we can ignore it.

Enforcing .altinstructions (and also .altinstr_replacement for
completness on ARM) to have the proper alignment across all
architectures and in both C and x86 makes them all the same.

On x86 the bloat-o-meter detects that with this change the file shrinks:
add/remove: 1/0 grow/shrink: 0/2 up/down: 156/-367 (-211)
function                                     old     new   delta
get_page_from_gfn                              -     156    +156
do_mmu_update                               4578    4569      -9
do_mmuext_op                                5604    5246    -358
Total: Before=3170439, After=3170228, chg -0.01%

But as found adding even "#Hi!\n" will casue this optimization, so the
bloat-o-meter value here is useless.

While on ARM 32/64:
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
function                                     old     new   delta
Total: Before=822563, After=822563, chg +0.00%

Also since the macros have the alignment coded in them there is no need
to do that for the xen.lds.S anymore.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v2: - First posting.
v3: - Figured out the x86 bloat-o-meter results.
    - Removed the .ALIGN from xen.lds.S
    - Removed the .p2align on .altinstr_replacement per Jan's request.
    - Put most of the commit description for the original issue
v4: - Added one .ALIGN back on xen.lds.S (arm)
    - Changed the .ALIGN(8) to ALIGN(4) on xen.lds.S (x86)
    - Moved p2align inside of the macros (ALTINSTR_ENTRY and altinstruction_entry)
---
 xen/arch/arm/xen.lds.S            | 1 -
 xen/arch/x86/xen.lds.S            | 2 +-
 xen/include/asm-arm/alternative.h | 4 ++++
 xen/include/asm-x86/alternative.h | 2 ++
 4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index c9b9546435..84ee475405 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -159,7 +159,6 @@ SECTIONS
        __alt_instructions = .;
        *(.altinstructions)
        __alt_instructions_end = .;
-       . = ALIGN(4);
        *(.altinstr_replacement)
 #endif
 
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index d5e8821d41..b03cca011b 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -202,7 +202,7 @@ SECTIONS
         * "Alternative instructions for different CPU types or capabilities"
         * Think locking instructions on spinlocks.
         */
-       . = ALIGN(8);
+       . = ALIGN(4);
         __alt_instructions = .;
         *(.altinstructions)
         __alt_instructions_end = .;
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index 6cc9d0dc5f..5e0d2b39a5 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -28,6 +28,7 @@ void __init apply_alternatives_all(void);
 int apply_alternatives(const struct alt_instr *start, const struct alt_instr *end);
 
 #define ALTINSTR_ENTRY(feature)						      \
+	" .p2align 2\n"							      \
 	" .word 661b - .\n"				/* label           */ \
 	" .word 663f - .\n"				/* new instruction */ \
 	" .hword " __stringify(feature) "\n"		/* feature bit     */ \
@@ -57,6 +58,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
 	ALTINSTR_ENTRY(feature)						\
 	".popsection\n"							\
 	".pushsection .altinstr_replacement, \"a\"\n"			\
+	".p2align 2\n"							\
 	"663:\n\t"							\
 	newinstr "\n"							\
 	"664:\n\t"							\
@@ -73,6 +75,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
 #include <asm/asm_defns.h>
 
 .macro altinstruction_entry orig_offset alt_offset feature orig_len alt_len
+	.p2align 2
 	.word \orig_offset - .
 	.word \alt_offset - .
 	.hword \feature
@@ -103,6 +106,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
 .macro alternative_if_not cap, enable = 1
 	.if \enable
 	.pushsection .altinstructions, "a"
+	.p2align 2
 	altinstruction_entry 661f, 663f, \cap, 662f-661f, 664f-663f
 	.popsection
 661:
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index db4f08e0e7..56574ceb0d 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -5,6 +5,7 @@
 
 #ifdef __ASSEMBLY__
 .macro altinstruction_entry orig alt feature orig_len alt_len
+        .p2align 2
         .long \orig - .
         .long \alt - .
         .word \feature
@@ -42,6 +43,7 @@ extern void alternative_instructions(void);
 #define alt_rlen(number) e_replacement(number)"f-"b_replacement(number)"f"
 
 #define ALTINSTR_ENTRY(feature, number)                                       \
+        " .p2align 2\n"                                                       \
         " .long 661b - .\n"                             /* label           */ \
         " .long " b_replacement(number)"f - .\n"        /* new instruction */ \
         " .word " __stringify(feature) "\n"             /* feature bit     */ \
-- 
2.13.3


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

[PATCH v4 06/11] mkhex: Move it to tools/misc

[Konrad Rzeszutek Wilk]

It makes more sense to put a tool to be used by other subsystems
to be in 'tools/misc' along 'mkrpm','mkdeb', etc.

The patch titled "xen/livepatch/x86/arm32: Force .livepatch.depends
section to be uint32_t aligned" uses mkhex.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v4: First posting.
---
 tools/firmware/hvmloader/Makefile        | 8 ++++----
 tools/{firmware/hvmloader => misc}/mkhex | 0
 2 files changed, 4 insertions(+), 4 deletions(-)
 rename tools/{firmware/hvmloader => misc}/mkhex (100%)

diff --git a/tools/firmware/hvmloader/Makefile b/tools/firmware/hvmloader/Makefile
index 7c4c0ce535..a5b4c32c1a 100644
--- a/tools/firmware/hvmloader/Makefile
+++ b/tools/firmware/hvmloader/Makefile
@@ -91,23 +91,23 @@ roms.inc: $(ROMS)
 
 ifneq ($(ROMBIOS_ROM),)
 	echo "#ifdef ROM_INCLUDE_ROMBIOS" >> $@.new
-	sh ./mkhex rombios $(ROMBIOS_ROM) >> $@.new
+	sh ../../misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
 
 ifneq ($(STDVGA_ROM),)
 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
-	sh ./mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
+	sh ../../misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
 ifneq ($(CIRRUSVGA_ROM),)
 	echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
-	sh ./mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
+	sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
 	echo "#endif" >> $@.new
 endif
 ifneq ($(ETHERBOOT_ROMS),)
 	echo "#ifdef ROM_INCLUDE_ETHERBOOT" >> $@.new
-	sh ./mkhex etherboot $(ETHERBOOT_ROMS) >> $@.new
+	sh ../../misc/mkhex etherboot $(ETHERBOOT_ROMS) >> $@.new
 	echo "#endif" >> $@.new
 endif
 
diff --git a/tools/firmware/hvmloader/mkhex b/tools/misc/mkhex
similarity index 100%
rename from tools/firmware/hvmloader/mkhex
rename to tools/misc/mkhex
-- 
2.13.3


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

[PATCH v4 07/11] livepatch/x86/arm[32, 64]: Force .livepatch.depends section to be uint32_t aligned.

[Konrad Rzeszutek Wilk]

By default when using objcopy we lose the alignment when we copy it from xen-syms -
with the result that alignment (on ARM32 for example) can be 1:

  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
..
  [ 6] .livepatch.depend PROGBITS        00000000 000093 000024 00   A  0   0  1

That, combined with wacky offset means it will be loaded in
memory with the wrong alignment:

(XEN) livepatch.c:425: livepatch: xen_bye_world: Loaded .livepatch.depends at 000a08043

And later we crash as the .livepatch.depends is not aligned to four bytes, while
the xen_build_id_check expects the code to be four byte aligned and we
get an hypervisor crash (on ARM32):

(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.10Hello World  arm32  debug=y   Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     002400a0 xen_build_id_check+0x8/0xe8
..snip..
(XEN) Xen call trace:
(XEN)    [<002400a0>] xen_build_id_check+0x8/0xe8 (PC)
(XEN)    [<0021a9c0>] livepatch_op+0x768/0x1610 (LR)
(XEN)    [<0023bbe4>] do_sysctl+0x9c8/0xa9c
(XEN)    [<002673c4>] do_trap_guest_sync+0x11e0/0x177c
(XEN)    [<0026b6a0>] entry.o#return_from_trap+0/0x4
(XEN)
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Data Abort

This fix forces all the test-cases to be built with a
.livepatch.depends structure containing the build-id extracted from
the hypervisor (except the xen_bye_world test-case).

We use the 'mkhex' tool instead of 'xxd' as the end result is an 'unsigned'
instead of 'char' type array - which naturally forces the alignment to be of four.
Also the 'mkhex' tools allows us to pass the section name as parameter.

The end result is much better alignment:

  [ 7] .livepatch.depend PROGBITS        00000000 000094 000024 00   A  0   0  4

Note that thanks to 'unsigned int .. __note_depends' the symbol becomes
global:

$ readelf --symbols *.livepatch | grep depen
    23: 0000000000000000    36 OBJECT  GLOBAL HIDDEN     6 note_depends
    49: 0000000000000000    36 OBJECT  GLOBAL HIDDEN    17 note_depends
    16: 0000000000000000    36 OBJECT  GLOBAL HIDDEN     3 note_depends
    21: 0000000000000000    36 OBJECT  GLOBAL HIDDEN     6 note_depends

See patch titled: "livepatch/arm/x86: Rename note_depends symbol from test-cases."
which fixes this.

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

---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>

v2: First posting.
v3: - Used mkhex from tools/misc instead of tools/firmware/hvmloader/
    - Include the XEN crash
---
 docs/misc/livepatch.markdown           |  2 ++
 xen/test/livepatch/Makefile            | 56 +++++++++++++++-------------------
 xen/test/livepatch/xen_bye_world.c     |  1 +
 xen/test/livepatch/xen_hello_world.c   |  1 +
 xen/test/livepatch/xen_nop.c           |  1 +
 xen/test/livepatch/xen_replace_world.c |  1 +
 6 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 59f89aa292..091029781e 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -430,6 +430,8 @@ checksum, MD5 checksum or any unique value.
 
 The size of these structures varies with the --build-id linker option.
 
+On ARM32 this section must by four-byte aligned.
+
 ## Hypercalls
 
 We will employ the sub operations of the system management hypercall (sysctl).
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 6831383db1..d23833e36f 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -1,15 +1,7 @@
 include $(XEN_ROOT)/Config.mk
 
-ifeq ($(XEN_TARGET_ARCH),x86_64)
-OBJCOPY_MAGIC := -I binary -O elf64-x86-64 -B i386:x86-64
-endif
-ifeq ($(XEN_TARGET_ARCH),arm64)
-OBJCOPY_MAGIC := -I binary -O elf64-littleaarch64 -B aarch64
-endif
-ifeq ($(XEN_TARGET_ARCH),arm32)
-OBJCOPY_MAGIC := -I binary -O elf32-littlearm -B arm
-endif
-
+NOTE_SYMBOL = "note_depends"
+NOTE_DEPENDS = "const  __section(\".livepatch.depends\") $(NOTE_SYMBOL)"
 CODE_ADDR=$(shell nm --defined $(1) | grep $(2) | awk '{print "0x"$$1}')
 CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}')
 
@@ -38,7 +30,7 @@ uninstall:
 
 .PHONY: clean
 clean::
-	rm -f *.o .*.o.d *.livepatch config.h
+	rm -f *.o .*.o.d *.livepatch config.h livepatch_depends.h hello_world_livepatch_depends.h *.bin
 
 #
 # To compute these values we need the binary files: xen-syms
@@ -56,10 +48,10 @@ config.h: xen_hello_world_func.o
 	 echo "#define MINOR_VERSION_ADDR $(MINOR_VERSION_ADDR)"; \
 	 echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)") > $@
 
-xen_hello_world.o: config.h
+xen_hello_world.o: config.h livepatch_depends.h
 
 .PHONY: $(LIVEPATCH)
-$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
+$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
 
 #
@@ -71,40 +63,42 @@ $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
 # not be built (it is for EFI builds), and that we do not have
 # the note.o.bin to muck with (as it gets deleted)
 #
-.PHONY: note.o
-note.o:
-	$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin
-	$(OBJCOPY) $(OBJCOPY_MAGIC) \
-		   --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@
-	rm -f $@.bin
+.PHONY: note.bin
+note.bin:
+	$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@
+
+.PHONY: livepatch_depends.h
+livepatch_depends.h: note.bin
+	$(shell (../../../tools/misc/mkhex $(NOTE_DEPENDS) $^ > $@))
 
 #
 # Extract the build-id of the xen_hello_world.livepatch
 # (which xen_bye_world will depend on).
 #
-.PHONY: hello_world_note.o
-hello_world_note.o: $(LIVEPATCH)
-	$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) $@.bin
-	$(OBJCOPY) $(OBJCOPY_MAGIC) \
-		   --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@
-	rm -f $@.bin
+.PHONY: hello_world_note.bin
+hello_world_note.bin: $(LIVEPATCH)
+	$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) $@
+
+.PHONY: hello_world_livepatch_depends.h
+hello_world_livepatch_depends.h: hello_world_note.bin
+	$(shell (../../../tools/misc/mkhex $(NOTE_DEPENDS) $^ > $@))
 
-xen_bye_world.o: config.h
+xen_bye_world.o: config.h hello_world_livepatch_depends.h
 
 .PHONY: $(LIVEPATCH_BYE)
-$(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o hello_world_note.o
+$(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_BYE) $^
 
-xen_replace_world.o: config.h
+xen_replace_world.o: config.h livepatch_depends.h
 
 .PHONY: $(LIVEPATCH_REPLACE)
-$(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o note.o
+$(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^
 
-xen_nop.o: config.h
+xen_nop.o: config.h livepatch_depends.h
 
 .PHONY: $(LIVEPATCH_NOP)
-$(LIVEPATCH_NOP): xen_nop.o note.o
+$(LIVEPATCH_NOP): xen_nop.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^
 
 .PHONY: livepatch
diff --git a/xen/test/livepatch/xen_bye_world.c b/xen/test/livepatch/xen_bye_world.c
index 2700f0eedd..935e76ca8b 100644
--- a/xen/test/livepatch/xen_bye_world.c
+++ b/xen/test/livepatch/xen_bye_world.c
@@ -10,6 +10,7 @@
 #include <xen/livepatch.h>
 
 #include <public/sysctl.h>
+#include "hello_world_livepatch_depends.h"
 
 static const char bye_world_patch_this_fnc[] = "xen_extra_version";
 extern const char *xen_bye_world(void);
diff --git a/xen/test/livepatch/xen_hello_world.c b/xen/test/livepatch/xen_hello_world.c
index 02f3f85dc0..988a3b14f4 100644
--- a/xen/test/livepatch/xen_hello_world.c
+++ b/xen/test/livepatch/xen_hello_world.c
@@ -11,6 +11,7 @@
 #include <xen/livepatch_payload.h>
 
 #include <public/sysctl.h>
+#include "livepatch_depends.h"
 
 static const char hello_world_patch_this_fnc[] = "xen_extra_version";
 extern const char *xen_hello_world(void);
diff --git a/xen/test/livepatch/xen_nop.c b/xen/test/livepatch/xen_nop.c
index a224b7c670..8d0c8f5097 100644
--- a/xen/test/livepatch/xen_nop.c
+++ b/xen/test/livepatch/xen_nop.c
@@ -7,6 +7,7 @@
 #include <xen/types.h>
 
 #include <public/sysctl.h>
+#include "livepatch_depends.h"
 
 /*
  * All of the .new_size and .old_addr are based on assumptions that the
diff --git a/xen/test/livepatch/xen_replace_world.c b/xen/test/livepatch/xen_replace_world.c
index 78a8f528b3..a653cc4268 100644
--- a/xen/test/livepatch/xen_replace_world.c
+++ b/xen/test/livepatch/xen_replace_world.c
@@ -9,6 +9,7 @@
 #include <xen/livepatch.h>
 
 #include <public/sysctl.h>
+#include "livepatch_depends.h"
 
 static const char xen_replace_world_name[] = "xen_extra_version";
 extern const char *xen_replace_world(void);
-- 
2.13.3


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

[PATCH v4 08/11] livepatch/arm/x86: Rename note_depends symbol from test-cases.

[Konrad Rzeszutek Wilk]

This surfaced due to "xen/livepatch/x86/arm32: Force
.livepatch.depends section to be uint32_t aligned." which switched
to a different way of including the build-id.

Each livepatch ends with a global:

    30: 00000000     1 OBJECT  GLOBAL HIDDEN     7 note_depends

which will cause collision when loading.

One attempted solution was to add in the Makefile stanza:
 @sed -i '/unsigned/static unsinged/' $@

But that resulted in the note_depends being omitted from the livepatch
(as it was static and not used) which meant we would not have an
.livepatch_depends section which we require.

One solution is to remove the symbol via the --strip-symbols
after generating the livepatch.

However that fails as note_depends is in use by .rel.debug_info:
Relocation section '.rel.debug_info' at offset 0x151c contains 113 entries:
 Offset     Info    Type            Sym.Value  Sym. Name
..
00000625  00001e02 R_ARM_ABS32       00000000   note_depends

And the solution to that is to also slap on --strip-debug which removes
various .debug* sections (which livepatch ignores anyhow):
.debug_aranges, .debug_info, .debug_abbrev, .debug_line, .debug_frame,
.debug_str, and their .rel.* sections. And that will remove that.

Alternatively we could also use --localize-symbol so that note_depends
is not globally visible. But that won't help as hypervisor treats
both local and global symbols as global when resolving them.

This patch decides to pick an easier path, just rename the symbol
by prefixing it with the name of the livepatch:

$ nm *.livepatch | grep depend
0000000000000000 R xen_bye_world.livepatch_note_depends
0000000000000000 R xen_hello_world.livepatch_note_depends
0000000000000000 R xen_local_symbols.livepatch_note_depends
0000000000000000 R xen_nop.livepatch_note_depends
0000000000000000 R xen_replace_world.livepatch_note_depends

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>

v3: First posting.
v4: Instead of stripping the symbol (and also using --strip-debug),
    just rename the symbol.
---
 xen/test/livepatch/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index d23833e36f..b48669cc13 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -53,6 +53,7 @@ xen_hello_world.o: config.h livepatch_depends.h
 .PHONY: $(LIVEPATCH)
 $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
+	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) $@
 
 #
 # This target is only accessible if CONFIG_LIVEPATCH is defined, which
@@ -88,18 +89,21 @@ xen_bye_world.o: config.h hello_world_livepatch_depends.h
 .PHONY: $(LIVEPATCH_BYE)
 $(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_BYE) $^
+	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) $@
 
 xen_replace_world.o: config.h livepatch_depends.h
 
 .PHONY: $(LIVEPATCH_REPLACE)
 $(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^
+	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) $@
 
 xen_nop.o: config.h livepatch_depends.h
 
 .PHONY: $(LIVEPATCH_NOP)
 $(LIVEPATCH_NOP): xen_nop.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^
+	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) $@
 
 .PHONY: livepatch
 livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP)
-- 
2.13.3


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

[PATCH v4 09/11] livepatch/tests: Make sure all .livepatch.funcs sections are read-only

[Konrad Rzeszutek Wilk]

Instead of being writable (.data). This mimics the behavior of what
livepatch-build-tools do.

Other approaches such as 'struct const livepatch_funcs' still result
in the WA section attributes.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>

v2: First posting.
v3: Don't do multiple objcopy invocations.
---
 xen/test/livepatch/Makefile | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index b48669cc13..9e45a81452 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -53,7 +53,8 @@ xen_hello_world.o: config.h livepatch_depends.h
 .PHONY: $(LIVEPATCH)
 $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
-	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) $@
+	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) \
+		   --set-section-flags .livepatch.funcs=alloc,readonly $@
 
 #
 # This target is only accessible if CONFIG_LIVEPATCH is defined, which
@@ -89,21 +90,24 @@ xen_bye_world.o: config.h hello_world_livepatch_depends.h
 .PHONY: $(LIVEPATCH_BYE)
 $(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_BYE) $^
-	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) $@
+	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) \
+		   --set-section-flags .livepatch.funcs=alloc,readonly $@
 
 xen_replace_world.o: config.h livepatch_depends.h
 
 .PHONY: $(LIVEPATCH_REPLACE)
 $(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^
-	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) $@
+	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) \
+		   --set-section-flags .livepatch.funcs=alloc,readonly $@
 
 xen_nop.o: config.h livepatch_depends.h
 
 .PHONY: $(LIVEPATCH_NOP)
 $(LIVEPATCH_NOP): xen_nop.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^
-	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) $@
+	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) \
+		   --set-section-flags .livepatch.funcs=alloc,readonly $@
 
 .PHONY: livepatch
 livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP)
-- 
2.13.3


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

[PATCH v4 10/11] livepatch/arm[32, 64]: Modify .livepatch.funcs section to be RW when patching

[Konrad Rzeszutek Wilk]

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 (as the
secure_payload changes the VA to RO).

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_livepatch_revive
we re-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 modify the VA of where
the .livepatch.funcs is to RW. After we are done with patching
we change it back to RO.

To do this we need to stash during livepatching the va of the page
in which the .livepatch.func resides, and the number of pages said
section uses (or 1 at min).

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.

Suggested-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>

v4: First posting
---
 xen/arch/arm/livepatch.c        | 45 ++++++++++++++++++++++++++++++++++++++---
 xen/arch/x86/livepatch.c        |  2 +-
 xen/common/livepatch.c          |  9 +++++++--
 xen/include/asm-arm/livepatch.h | 13 ++++++++++++
 xen/include/xen/livepatch.h     |  2 +-
 5 files changed, 64 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 76723f1f1a..f23df8597d 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>
@@ -17,13 +18,15 @@
 #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
 
 void *vmap_of_xen_text;
+struct livepatch_va livepatch_stash;
 
-int arch_livepatch_quiesce(void)
+int arch_livepatch_quiesce(struct livepatch_func *func, size_t nfuncs)
 {
     mfn_t text_mfn;
     unsigned int text_order;
+    int rc = 0;
 
-    if ( vmap_of_xen_text )
+    if ( vmap_of_xen_text || livepatch_stash.va )
         return -EINVAL;
 
     text_mfn = virt_to_mfn(_start);
@@ -43,7 +46,29 @@ int arch_livepatch_quiesce(void)
         return -ENOMEM;
     }
 
-    return 0;
+    if ( nfuncs )
+    {
+        unsigned long va = (unsigned long)func;
+        unsigned int offs = va & (PAGE_SIZE - 1);
+        unsigned int pages = PFN_UP(offs + nfuncs * sizeof(*func));
+
+        va &= PAGE_MASK;
+
+        rc = modify_xen_mappings(va, va + (pages * PAGE_SIZE), PTE_NX);
+        if ( rc )
+        {
+            printk(XENLOG_ERR LIVEPATCH "Failed to modify 0x%lx to RW\n", va);
+            vunmap(vmap_of_xen_text);
+            vmap_of_xen_text = NULL;
+        }
+        else
+        {
+            livepatch_stash.va = va;
+            livepatch_stash.pages = pages;
+        }
+    }
+
+    return rc;
 }
 
 void arch_livepatch_revive(void)
@@ -58,6 +83,20 @@ void arch_livepatch_revive(void)
         vunmap(vmap_of_xen_text);
 
     vmap_of_xen_text = NULL;
+
+    if ( livepatch_stash.va )
+    {
+        unsigned long va = livepatch_stash.va;
+
+        int rc = modify_xen_mappings(va, va + (livepatch_stash.pages * PAGE_SIZE),
+                                     PTE_NX | PTE_RO);
+        if ( rc )
+            printk(XENLOG_ERR LIVEPATCH "Failed to modify %lx to RO!\n", va);
+
+        livepatch_stash.va = 0;
+        livepatch_stash.pages = 0;
+
+    }
 }
 
 int arch_livepatch_verify_func(const struct livepatch_func *func)
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 48d20fdacd..f5865370d5 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, size_t 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 f736c3a7ea..af8998ec8d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -56,6 +56,7 @@ struct payload {
     int32_t rc;                          /* 0 or -XEN_EXX. */
     bool reverted;                       /* Whether it was reverted. */
     bool safe_to_reapply;                /* Can apply safely after revert. */
+    bool funcs_ro;                       /* Are livepatch.funcs in .rodata section. */
     struct list_head list;               /* Linked to 'payload_list'. */
     const void *text_addr;               /* Virtual address of .text. */
     size_t text_size;                    /* .. and its size. */
@@ -538,6 +539,10 @@ static int prepare_payload(struct payload *payload,
     payload->funcs = sec->load_addr;
     payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);
 
+    if ( sec->load_addr >= payload->ro_addr &&
+         sec->load_addr < (payload->ro_addr + payload->ro_size) )
+        payload->funcs_ro = true;
+
     for ( i = 0; i < payload->nfuncs; i++ )
     {
         int rc;
@@ -1070,7 +1075,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->funcs_ro ? data->nfuncs : 0);
     if ( rc )
     {
         printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
@@ -1111,7 +1116,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->funcs_ro ? data->nfuncs : 0);
     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..cd13ca786d 100644
--- a/xen/include/asm-arm/livepatch.h
+++ b/xen/include/asm-arm/livepatch.h
@@ -17,6 +17,19 @@
  */
 extern void *vmap_of_xen_text;
 
+/*
+ * The va of the livepatch .livepatch.funcs. Only used if said
+ * region is in RO VA and we need to modify it to RW during
+ * livepatching.
+ */
+struct livepatch_va
+{
+    unsigned long va;
+    unsigned int pages;
+};
+
+extern struct livepatch_va livepatch_stash;
+
 /* These ranges are only for unconditional branches. */
 #ifdef CONFIG_ARM_32
 /* ARM32: A4.3 IN ARM DDI 0406C.c -  we are using only ARM instructions in Xen.*/
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index e9bab87f28..4aceaab637 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 *, size_t nfunc);
 void arch_livepatch_revive(void);
 
 void arch_livepatch_apply(struct livepatch_func *func);
-- 
2.13.3


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

[PATCH v4 11/11] livepatch: Declare live patching as a supported feature

[Konrad Rzeszutek Wilk]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

See docs/features/livepatch.pandoc for the details.

Reviewed-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad@kernel.org>

--
v2:
 - Moved it into a feature document.
 - Clarified a few bits and pieces based on feedback.
v3:
 - default X86
 - added Jan's Reviewed-by
 - Added tech preview for ARM.
 - Cut down the 3) paragraph per George's input
---
 docs/features/livepatch.pandoc | 106 +++++++++++++++++++++++++++++++++++++++++
 xen/common/Kconfig             |   4 +-
 2 files changed, 108 insertions(+), 2 deletions(-)
 create mode 100644 docs/features/livepatch.pandoc

diff --git a/docs/features/livepatch.pandoc b/docs/features/livepatch.pandoc
new file mode 100644
index 0000000000..17f1cd0d05
--- /dev/null
+++ b/docs/features/livepatch.pandoc
@@ -0,0 +1,106 @@
+% Live Patching
+% Revision 1
+
+\clearpage
+
+# Basics
+
+---------------- ----------------------------------------------------
+         Status: **Supported**
+
+   Architecture: x86
+
+         Status: **Tech Preview/Experimental**
+
+   Architecture: ARM
+
+      Component: Hypervisor, toolstack
+---------------- ----------------------------------------------------
+
+
+# Details
+
+Xen Live Patching has been available as tech preview feature since Xen
+4.7 and has now had a couple of releases to stabilize. Xen Live patching
+has been used by multiple vendors to fix several real-world security
+issues without any severe bugs encountered. Additionally, there are now
+tests in OSSTest that test live patching to ensure that no regressions
+are introduced.
+
+Based on the amount of testing and usage it has had, we are ready to
+declare live patching as a 'Supported' feature on x86.
+
+Live patching is slightly peculiar when it comes to support because it
+allows the host administrator to break their system rather easily
+depending on the content of the live patch. Because of this, it is
+worth detailing the scope of security support:
+
+1) Unprivileged access to live patching operations:
+   Live patching operations should only be accessible to privileged
+   guests and it shall be treated as a security issue if this is not
+   the case.
+
+2) Bugs in the patch-application code such that vulnerabilities exist
+   after application:
+   If a correct live patch is loaded but it is not applied correctly
+   such that it might result in an insecure system (e.g. not all
+   functions are patched), it shall be treated as a security issue.
+
+3) Bugs in livepatch-build-tools creating an incorrect live patch that
+   results in an insecure host:
+   If livepatch-build-tools creates an incorrect live patch that
+   results in an insecure host, this shall not be considered a security
+   issue. A live patch should be checked to verify that it is valid
+   before loading.
+
+4) Loading an incorrect live patch that results in an insecure host or
+   host crash:
+   If a live patch (whether created using livepatch-build-tools or some
+   alternative) is loaded and it results in an insecure host or host
+   crash due to the content of the live patch being incorrect or the
+   issue being inappropriate to live patch, this is not considered as a
+   security issue.
+
+5) Bugs in the live patch parsing code (the ELF loader):
+   Bugs in the live patch parsing code such as out-of-bounds reads
+   caused by invalid ELF files are not considered to be security issues
+   because the it can only be triggered by a privileged domain.
+
+6) Bugs which allow a guest to prevent the application of a livepatch:
+   A guest should not be able to prevent the application of a live
+   patch. If an unprivileged guest can somehow prevent the application
+   of a live patch despite pausing it (xl pause ...), it shall be
+   treated as a security issue.
+
+Note: It is expected that live patches are tested in a test environment
+before being used in production to avoid unexpected issues. In
+particular, to avoid the issues described by (3), (4), & (5).
+
+There are also some generic security questions which are worth asking:
+
+1) Is guest->host privilege escalation possible?
+
+The new live patching sysctl subops are only accessible to privileged
+domains and this is tested by OSSTest with an XTF test.
+There is a caveat -- an incorrect live patch can introduce a guest->host
+privilege escalation.
+
+2) Is guest user->guest kernel escalation possible?
+
+No, although an incorrect live patch can introduce a guest user->guest
+kernel privilege escalation.
+
+3) Is there any information leakage?
+
+The new live patching sysctl subops are only accessible to privileged
+domains so it is not possible for an unprivileged guest to access the
+list of loaded live patches. This is tested by OSSTest with an XTF test.
+There is a caveat -- an incorrect live patch can introduce an
+information leakage.
+
+4) Can a Denial-of-Service be triggered?
+
+There are no known ways that an unprivileged guest can prevent a live
+patch from being loaded.
+Once again, there is a caveat that an incorrect live patch can introduce
+an arbitrary denial of service.
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index dc8e876439..e9bb849298 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -226,8 +226,8 @@ config CRYPTO
 	bool
 
 config LIVEPATCH
-	bool "Live patching support (TECH PREVIEW)"
-	default n
+	bool "Live patching support"
+	default X86
 	depends on HAS_BUILD_ID = "y"
 	---help---
 	  Allows a running Xen hypervisor to be dynamically patched using
-- 
2.13.3


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

Re: [PATCH v4 06/11] mkhex: Move it to tools/misc

[Wei Liu]

On Wed, Sep 20, 2017 at 06:31:43PM -0400, Konrad Rzeszutek Wilk wrote:
> It makes more sense to put a tool to be used by other subsystems
> to be in 'tools/misc' along 'mkrpm','mkdeb', etc.
> 
> The patch titled "xen/livepatch/x86/arm32: Force .livepatch.depends
> section to be uint32_t aligned" uses mkhex.
> 
> Suggested-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH v4 03/11] livepatch: Include sizes when an mismatch occurs

[Jan Beulich]

>>> On 21.09.17 at 00:31, <konrad@kernel.org> wrote:
> If the .bug.frames.X or .livepatch.funcs sizes are different
> than what the hypervisor expects - we fail the payload. To help
> in diagnosing this include the expected and the payload
> sizes.
> 
> Also make it more natural by having "Multiples" in the warning.
> 
> Also fix one case where we would fail if the size of the .ex_table
> was being zero - but that is OK.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH v4 05/11] alternative/x86/arm32: Align altinstructions (and altinstr_replacement) sections.

[Jan Beulich]

>>> On 21.09.17 at 00:31, <konrad@kernel.org> wrote:
> @@ -73,6 +75,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
>  #include <asm/asm_defns.h>
>  
>  .macro altinstruction_entry orig_offset alt_offset feature orig_len alt_len
> +	.p2align 2
>  	.word \orig_offset - .
>  	.word \alt_offset - .
>  	.hword \feature
> @@ -103,6 +106,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
>  .macro alternative_if_not cap, enable = 1
>  	.if \enable
>  	.pushsection .altinstructions, "a"
> +	.p2align 2
>  	altinstruction_entry 661f, 663f, \cap, 662f-661f, 664f-663f
>  	.popsection

Why? altinstruction_entry already does what you want.

x86 parts
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v4 08/11] livepatch/arm/x86: Rename note_depends symbol from test-cases.

[Jan Beulich]

>>> On 21.09.17 at 00:31, <konrad@kernel.org> wrote:
> --- a/xen/test/livepatch/Makefile
> +++ b/xen/test/livepatch/Makefile
> @@ -53,6 +53,7 @@ xen_hello_world.o: config.h livepatch_depends.h
>  .PHONY: $(LIVEPATCH)
>  $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o
>  	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
> +	$(OBJCOPY) --redefine-sym $(NOTE_SYMBOL)=$@_$(NOTE_SYMBOL) $@

I should have paid attention to this earlier: This model of modifying
in place the target file of a rule doesn't work well when taking into
account someone hitting Ctrl-C in the middle of the build process.
The linker output should go to an intermediate file, and objcopy
should then produce the final one.

Jan


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

Re: [PATCH v4 10/11] livepatch/arm[32, 64]: Modify .livepatch.funcs section to be RW when patching

[Jan Beulich]

>>> On 21.09.17 at 00:31, <konrad@kernel.org> wrote:
> @@ -43,7 +46,29 @@ int arch_livepatch_quiesce(void)
>          return -ENOMEM;
>      }
>  
> -    return 0;
> +    if ( nfuncs )
> +    {
> +        unsigned long va = (unsigned long)func;
> +        unsigned int offs = va & (PAGE_SIZE - 1);
> +        unsigned int pages = PFN_UP(offs + nfuncs * sizeof(*func));
> +
> +        va &= PAGE_MASK;
> +
> +        rc = modify_xen_mappings(va, va + (pages * PAGE_SIZE), PTE_NX);
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR LIVEPATCH "Failed to modify 0x%lx to RW\n", va);

%#lx ?

> +            vunmap(vmap_of_xen_text);
> +            vmap_of_xen_text = NULL;
> +        }
> +        else
> +        {
> +            livepatch_stash.va = va;
> +            livepatch_stash.pages = pages;
> +        }
> +    }

You're effectively doing all this behind the back of vmalloc() / vmap();
I'm not sure this is a good idea, but I'm also not a maintainer of this
code.

Jan


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

Re: [PATCH v4 10/11] livepatch/arm[32, 64]: Modify .livepatch.funcs section to be RW when patching

[Julien Grall]

Hi Jan,

On 21/09/17 13:16, Jan Beulich wrote:
>>>> On 21.09.17 at 00:31, <konrad@kernel.org> wrote:
>> @@ -43,7 +46,29 @@ int arch_livepatch_quiesce(void)
>>           return -ENOMEM;
>>       }
>>   
>> -    return 0;
>> +    if ( nfuncs )
>> +    {
>> +        unsigned long va = (unsigned long)func;
>> +        unsigned int offs = va & (PAGE_SIZE - 1);
>> +        unsigned int pages = PFN_UP(offs + nfuncs * sizeof(*func));
>> +
>> +        va &= PAGE_MASK;
>> +
>> +        rc = modify_xen_mappings(va, va + (pages * PAGE_SIZE), PTE_NX);
>> +        if ( rc )
>> +        {
>> +            printk(XENLOG_ERR LIVEPATCH "Failed to modify 0x%lx to RW\n", va);
> 
> %#lx ?
> 
>> +            vunmap(vmap_of_xen_text);
>> +            vmap_of_xen_text = NULL;
>> +        }
>> +        else
>> +        {
>> +            livepatch_stash.va = va;
>> +            livepatch_stash.pages = pages;
>> +        }
>> +    }
> 
> You're effectively doing all this behind the back of vmalloc() / vmap();
> I'm not sure this is a good idea, but I'm also not a maintainer of this
> code.

We already have place in the code (both x86 and Arm) modifying memory 
attributes on the back of vmalloc/vmap. See arch_livepatch_secure for 
instance.

I suggested this solution because it avoids to create a temporary 
mapping for the .livepatch.funcs section.

Do you foresee potentially issue of temporarily modifying permissions of 
a mapping?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 10/11] livepatch/arm[32, 64]: Modify .livepatch.funcs section to be RW when patching

[Jan Beulich]

>>> On 21.09.17 at 16:58, <julien.grall@arm.com> wrote:
> Do you foresee potentially issue of temporarily modifying permissions of 
> a mapping?

It's generally not a good idea imo, but perhaps it's fine here. I
assume the page permissions can't be adversely affected despite
their hard coding in the function invocations.

Jan


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

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

[Jan Beulich]

>>> On 21.09.17 at 00:31, <konrad@kernel.org> wrote:

> @@ -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;
> +        }

And no similar check being added to ARM64? Looking at that code I
also notice that the general "minimum 32-bit width" there is likely
wrong for at least ABS16 and PREL16.

> --- 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);

If you really mean to say "text section" here, then the SHF_EXECINSTR
check should move here too.

Jan


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

Re: [PATCH v4 01/11] livepatch: Expand check for safe_for_reapply if livepatch has only .rodata.

[Ross Lagerwall]

On 09/20/2017 11:31 PM, Konrad Rzeszutek Wilk wrote:
> If the livepatch has only .rodata sections then it is OK to also
> apply/revert/apply the livepatch without having to worry about the
> unforseen consequences.
> 
> See commit 98b728a7b235c67e210f67f789db5d9eb38ca00c
> "livepatch: Disallow applying after an revert" for details.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> 

The patch looks OK, but what is the use case for a live patch with only 
.rodata?

Regards,
-- 
Ross Lagerwall

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

Re: [PATCH v4 01/11] livepatch: Expand check for safe_for_reapply if livepatch has only .rodata.

[Konrad Rzeszutek Wilk]

On Thu, Oct 05, 2017 at 02:47:30PM +0100, Ross Lagerwall wrote:
> On 09/20/2017 11:31 PM, Konrad Rzeszutek Wilk wrote:
> > If the livepatch has only .rodata sections then it is OK to also
> > apply/revert/apply the livepatch without having to worry about the
> > unforseen consequences.
> > 
> > See commit 98b728a7b235c67e210f67f789db5d9eb38ca00c
> > "livepatch: Disallow applying after an revert" for details.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> > Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> > 
> 
> The patch looks OK, but what is the use case for a live patch with only
> .rodata?

A NOP one. This hasn't been an issue in the past, but further patches
change the .livepatch_funcs from RW to RO (to be in line with what
livepatch-build-tools.git do) - and then the xen_nop testcase does not
work anymore.


> 
> Regards,
> -- 
> Ross Lagerwall
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [PATCH v4 03/11] livepatch: Include sizes when an mismatch occurs

[Ross Lagerwall]

On 09/20/2017 11:31 PM, Konrad Rzeszutek Wilk wrote:
> If the .bug.frames.X or .livepatch.funcs sizes are different
> than what the hypervisor expects - we fail the payload. To help
> in diagnosing this include the expected and the payload
> sizes.
> 
> Also make it more natural by having "Multiples" in the warning.
> 
> Also fix one case where we would fail if the size of the .ex_table
> was being zero - but that is OK.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> 

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [PATCH v4 01/11] livepatch: Expand check for safe_for_reapply if livepatch has only .rodata.

[Ross Lagerwall]

On 10/05/2017 02:51 PM, Konrad Rzeszutek Wilk wrote:
> On Thu, Oct 05, 2017 at 02:47:30PM +0100, Ross Lagerwall wrote:
>> On 09/20/2017 11:31 PM, Konrad Rzeszutek Wilk wrote:
>>> If the livepatch has only .rodata sections then it is OK to also
>>> apply/revert/apply the livepatch without having to worry about the
>>> unforseen consequences.
>>>
>>> See commit 98b728a7b235c67e210f67f789db5d9eb38ca00c
>>> "livepatch: Disallow applying after an revert" for details.
>>>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> ---
>>> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
>>>
>>
>> The patch looks OK, but what is the use case for a live patch with only
>> .rodata?
> 
> A NOP one. This hasn't been an issue in the past, but further patches
> change the .livepatch_funcs from RW to RO (to be in line with what
> livepatch-build-tools.git do) - and then the xen_nop testcase does not
> work anymore.
> 


Thanks, that makes sense.

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [PATCH v4 07/11] livepatch/x86/arm[32, 64]: Force .livepatch.depends section to be uint32_t aligned.

[Ross Lagerwall]

On 09/20/2017 11:31 PM, Konrad Rzeszutek Wilk wrote:
> By default when using objcopy we lose the alignment when we copy it from xen-syms -
> with the result that alignment (on ARM32 for example) can be 1:
> 
>    [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
> ..
>    [ 6] .livepatch.depend PROGBITS        00000000 000093 000024 00   A  0   0  1
> 
> That, combined with wacky offset means it will be loaded in
> memory with the wrong alignment:
> 
> (XEN) livepatch.c:425: livepatch: xen_bye_world: Loaded .livepatch.depends at 000a08043
> 
> And later we crash as the .livepatch.depends is not aligned to four bytes, while
> the xen_build_id_check expects the code to be four byte aligned and we
> get an hypervisor crash (on ARM32):
> 
> (XEN) CPU0: Unexpected Trap: Data Abort
> (XEN) ----[ Xen-4.10Hello World  arm32  debug=y   Not tainted ]----
> (XEN) CPU:    0
> (XEN) PC:     002400a0 xen_build_id_check+0x8/0xe8
> ..snip..
> (XEN) Xen call trace:
> (XEN)    [<002400a0>] xen_build_id_check+0x8/0xe8 (PC)
> (XEN)    [<0021a9c0>] livepatch_op+0x768/0x1610 (LR)
> (XEN)    [<0023bbe4>] do_sysctl+0x9c8/0xa9c
> (XEN)    [<002673c4>] do_trap_guest_sync+0x11e0/0x177c
> (XEN)    [<0026b6a0>] entry.o#return_from_trap+0/0x4
> (XEN)
> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) CPU0: Unexpected Trap: Data Abort
> 
> This fix forces all the test-cases to be built with a
> .livepatch.depends structure containing the build-id extracted from
> the hypervisor (except the xen_bye_world test-case).
> 
> We use the 'mkhex' tool instead of 'xxd' as the end result is an 'unsigned'
> instead of 'char' type array - which naturally forces the alignment to be of four.
> Also the 'mkhex' tools allows us to pass the section name as parameter.
> 
> The end result is much better alignment:
> 
>    [ 7] .livepatch.depend PROGBITS        00000000 000094 000024 00   A  0   0  4
> 
> Note that thanks to 'unsigned int .. __note_depends' the symbol becomes
> global:
> 
> $ readelf --symbols *.livepatch | grep depen
>      23: 0000000000000000    36 OBJECT  GLOBAL HIDDEN     6 note_depends
>      49: 0000000000000000    36 OBJECT  GLOBAL HIDDEN    17 note_depends
>      16: 0000000000000000    36 OBJECT  GLOBAL HIDDEN     3 note_depends
>      21: 0000000000000000    36 OBJECT  GLOBAL HIDDEN     6 note_depends
> 
> See patch titled: "livepatch/arm/x86: Rename note_depends symbol from test-cases."
> which fixes this.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> ---
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> 
Acked-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

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

[Ross Lagerwall]

On 09/20/2017 11:31 PM, 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@oracle.com>
snip
> +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;
> +};
> +

There is a stray semicolon here.

If I understand correctly, this patch seems to be doing two separate things:
* Checking section alignment when loading the livepatch on ARM32/64.
* Checking that relocation destinations are aligned when applying 
relocations (on ARM32 only).

Surely these are separate and should it should be split into two patches?

Is the latter check not needed for ARM64?

--
Ross Lagerwall

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