[PATCH v3] Livepatching patch set for 4.10

[PATCH v3] 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 the vmap functionality.
   Which then I made common on x86 as well (so no more CR4 mucking).
   This meant however meant mucking with x86 code page walker to
   have a do_page_walk functionality.

 - 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). It has latest
   feedback from Andrew and George (I hope!)

 - Post the local/global symbol patchset functionality. Only Jan had
   commented and I would appreciate other folks feedback - perhaps
   folks have better ideas on this?

Lastly, I tested this on ARM32 (Cubietruck), ARM64 (HiKey960) and
on x86. All looked good until I enabled CONFIG_DEBUG_SCRUB - which
is reported in:
https://lists.xen.org/archives/html/xen-devel/2017-09/msg01147.html

Patches are in 

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

 docs/features/livepatch.pandoc         | 106 ++++++++++++++++
 docs/misc/livepatch.markdown           |   4 +
 xen/arch/arm/arm32/livepatch.c         |  33 ++++-
 xen/arch/arm/arm64/livepatch.c         |  17 ++-
 xen/arch/arm/livepatch.c               |  49 +-------
 xen/arch/arm/xen.lds.S                 |   2 -
 xen/arch/x86/livepatch.c               |  36 ++++--
 xen/arch/x86/x86_64/mm.c               |  33 +++--
 xen/arch/x86/xen.lds.S                 |   1 -
 xen/common/Kconfig                     |   4 +-
 xen/common/livepatch.c                 | 219 +++++++++++++++++++++++++--------
 xen/common/livepatch_elf.c             |  13 ++
 xen/include/asm-arm/alternative.h      |   4 +
 xen/include/asm-arm/livepatch.h        |   6 -
 xen/include/asm-x86/alternative.h      |   1 +
 xen/include/asm-x86/mm.h               |   1 +
 xen/include/xen/elfstructs.h           |   2 +
 xen/include/xen/livepatch.h            |  23 +++-
 xen/include/xen/livepatch_elf.h        |   7 ++
 xen/test/livepatch/Makefile            |  76 +++++++-----
 xen/test/livepatch/xen_bye_world.c     |   1 +
 xen/test/livepatch/xen_hello_world.c   |   1 +
 xen/test/livepatch/xen_local_symbols.c |  53 ++++++++
 xen/test/livepatch/xen_nop.c           |   1 +
 xen/test/livepatch/xen_replace_world.c |   1 +
 25 files changed, 521 insertions(+), 173 deletions(-)

Konrad Rzeszutek Wilk (16):
      livepatch: Expand check for safe_for_reapply if livepatch has only .rodata.
      livepatch: Tighten alignment checks.
      livepatch: Include sizes when an mismatch occurs
      xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong text alignment.
      alternative/x86/arm32: Align altinstructions (and altinstr_replacement) sections.
      xen/livepatch/x86/arm32: Force .livepatch.depends section to be uint32_t aligned.
      livepatch/arm/x86: Strip note_depends symbol from test-cases.
      livepatch/tests: Make sure all .livepatch.funcs sections are read-only
      livepatch/arm[32,64]: Modify livepatch_funcs
      livepatch/x86/arm[32,64]: Use common vmap code for applying.
      livepatch/x86/arm[32,64]: Unify arch_livepatch_revert
      livepatch: Expand spin_debug_disable in [apply|revert]_payload
      livepatch/x86/arm: arch/x86/mm: generalize do_page_walk() and implement arch_livepatch_lookup_mfn
      livepatch/x86/arm: Utilize the arch_livepatch_lookup_mfn
      livepatch: Add local and global symbol resolution.
      livepatch: Add xen_local_symbols test-case

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


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

[PATCH v3 01/17] 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>
---
v3: New patch
---
 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 66d532db14..a1f54c42d3 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 v3 02/17] 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.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.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.
---
 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..7839913ff5 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_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_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_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

Re: [PATCH v3 02/17] livepatch: Tighten alignment checks.

[Jan Beulich]

>>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote:
> 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.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit generally I'd recommend the check to be done in the
opposite order.

Jan


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

[PATCH v3 03/17] 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>
---
v1: Initial version
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 a1f54c42d3..c6ee95fbcf 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 v3 04/17] xen/livepatch/ARM32: 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>
---
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 &&
+         ((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. */
+    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)
 {
-- 
2.13.3


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

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

[Julien Grall]

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

[PATCH v3 05/17] 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>
---
v2: - First version
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
---
 xen/arch/arm/xen.lds.S            | 2 --
 xen/arch/x86/xen.lds.S            | 1 -
 xen/include/asm-arm/alternative.h | 4 ++++
 xen/include/asm-x86/alternative.h | 1 +
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index c9b9546435..447d33888f 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -155,11 +155,9 @@ SECTIONS
        __initcall_end = .;
 
 #ifdef CONFIG_HAS_ALTERNATIVE
-       . = ALIGN(4);
        __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..9eb42048d5 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -202,7 +202,6 @@ SECTIONS
         * "Alternative instructions for different CPU types or capabilities"
         * Think locking instructions on spinlocks.
         */
-       . = ALIGN(8);
         __alt_instructions = .;
         *(.altinstructions)
         __alt_instructions_end = .;
diff --git a/xen/include/asm-arm/alternative.h b/xen/include/asm-arm/alternative.h
index 6cc9d0dc5f..cd1373fdd5 100644
--- a/xen/include/asm-arm/alternative.h
+++ b/xen/include/asm-arm/alternative.h
@@ -54,9 +54,11 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
 	oldinstr "\n"							\
 	"662:\n"							\
 	".pushsection .altinstructions,\"a\"\n"				\
+	".p2align 2\n"							\
 	ALTINSTR_ENTRY(feature)						\
 	".popsection\n"							\
 	".pushsection .altinstr_replacement, \"a\"\n"			\
+	".p2align 2\n"							\
 	"663:\n\t"							\
 	newinstr "\n"							\
 	"664:\n\t"							\
@@ -84,6 +86,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en
 	.if \enable
 661:	\insn1
 662:	.pushsection .altinstructions, "a"
+	.p2align 2
 	altinstruction_entry 661b, 663f, \cap, 662b-661b, 664f-663f
 	.popsection
 	.pushsection .altinstr_replacement, "ax"
@@ -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..79430fdf05 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -56,6 +56,7 @@ extern void alternative_instructions(void);
 
 #define ALTERNATIVE_N(newinstr, feature, number)	\
 	".pushsection .altinstructions,\"a\"\n"		\
+	".p2align 2\n"					\
 	ALTINSTR_ENTRY(feature, number)			\
 	".section .discard,\"a\",@progbits\n"		\
 	DISCARD_ENTRY(number)				\
-- 
2.13.3


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

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

[Jan Beulich]

>>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote:
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -155,11 +155,9 @@ SECTIONS
>         __initcall_end = .;
>  
>  #ifdef CONFIG_HAS_ALTERNATIVE
> -       . = ALIGN(4);

I think this one needs to say, while ...

>         __alt_instructions = .;
>         *(.altinstructions)
>         __alt_instructions_end = .;
> -       . = ALIGN(4);

... this one can go and ...

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -202,7 +202,6 @@ SECTIONS
>          * "Alternative instructions for different CPU types or capabilities"
>          * Think locking instructions on spinlocks.
>          */
> -       . = ALIGN(8);
>          __alt_instructions = .;

... this one again needs to stay, but the 8 can be replaced by a 4.
That's because otherwise there's the risk of the __alt_instructions
label to not be at the start of the first .altinstructions (solely
depending on what happens to be immediately before this script
section). I'm sorry for the earlier mis-guiding of mine.

> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -56,6 +56,7 @@ extern void alternative_instructions(void);
>  
>  #define ALTERNATIVE_N(newinstr, feature, number)	\
>  	".pushsection .altinstructions,\"a\"\n"		\
> +	".p2align 2\n"					\
>  	ALTINSTR_ENTRY(feature, number)			\

I think the would better go into ALTINSTR_ENTRY(), and then also
into the altinstruction_entry assembler macro.

Jan


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

[PATCH v3 06/17] xen/livepatch/x86/arm32: 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 get:
(XEN) livepatch.c:501: livepatch: xen_bye_world: .livepatch.depends is not aligned properly!

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: Strip note_depends symbol from test-cases."
which fixes this.

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

---
v2: First version
---
 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 505dc37cda..922a64436f 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..89ad89dfd5 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/firmware/hvmloader/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/firmware/hvmloader/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

Re: [PATCH v3 06/17] xen/livepatch/x86/arm32: Force .livepatch.depends section to be uint32_t aligned.

[Julien Grall]

Hi Konrad,

On 12/09/17 01:37, 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 get:
> (XEN) livepatch.c:501: livepatch: xen_bye_world: .livepatch.depends is not aligned properly!
> 
> 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: Strip note_depends symbol from test-cases."
> which fixes this.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> ---
> v2: First version
> ---
>   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 505dc37cda..922a64436f 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..89ad89dfd5 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/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@))

It looks quite odd to use a file in firmware/hvmloader for livepatch. 
Would it be possible to move mkhex to a generic place?

Cheers,

-- 
Julien Grall

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

Re: [PATCH v3 06/17] xen/livepatch/x86/arm32: Force .livepatch.depends section to be uint32_t aligned.

[Konrad Rzeszutek Wilk]

> > +.PHONY: livepatch_depends.h
> > +livepatch_depends.h: note.bin
> > +	$(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@))
> 
> It looks quite odd to use a file in firmware/hvmloader for livepatch. Would
> it be possible to move mkhex to a generic place?

Like so?

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
diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 8ac9f5e426..f0365305ba 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -73,7 +73,7 @@ note.bin:
 
 .PHONY: livepatch_depends.h
 livepatch_depends.h: note.bin
-	$(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@))
+	$(shell (../../../tools/misc/mkhex $(NOTE_DEPENDS) $^ > $@))
 
 #
 # Extract the build-id of the xen_hello_world.livepatch
@@ -85,7 +85,7 @@ hello_world_note.bin: $(LIVEPATCH)
 
 .PHONY: hello_world_livepatch_depends.h
 hello_world_livepatch_depends.h: hello_world_note.bin
-	$(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@))
+	$(shell (../../../tools/misc/mkhex $(NOTE_DEPENDS) $^ > $@))
 
 xen_bye_world.o: config.h hello_world_livepatch_depends.h
 

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

Re: [PATCH v3 06/17] xen/livepatch/x86/arm32: Force .livepatch.depends section to be uint32_t aligned.

[Julien Grall]

Hi Konrad,

On 19/09/17 01:32, Konrad Rzeszutek Wilk wrote:
>>> +.PHONY: livepatch_depends.h
>>> +livepatch_depends.h: note.bin
>>> +	$(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@))
>>
>> It looks quite odd to use a file in firmware/hvmloader for livepatch. Would
>> it be possible to move mkhex to a generic place?
> 
> Like so?

It is what I had in mind. I CCed Ian and Wei to get feedback from them.

Cheers,


> 
> 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
> diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
> index 8ac9f5e426..f0365305ba 100644
> --- a/xen/test/livepatch/Makefile
> +++ b/xen/test/livepatch/Makefile
> @@ -73,7 +73,7 @@ note.bin:
>   
>   .PHONY: livepatch_depends.h
>   livepatch_depends.h: note.bin
> -	$(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@))
> +	$(shell (../../../tools/misc/mkhex $(NOTE_DEPENDS) $^ > $@))
>   
>   #
>   # Extract the build-id of the xen_hello_world.livepatch
> @@ -85,7 +85,7 @@ hello_world_note.bin: $(LIVEPATCH)
>   
>   .PHONY: hello_world_livepatch_depends.h
>   hello_world_livepatch_depends.h: hello_world_note.bin
> -	$(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@))
> +	$(shell (../../../tools/misc/mkhex $(NOTE_DEPENDS) $^ > $@))
>   
>   xen_bye_world.o: config.h hello_world_livepatch_depends.h
>   
> 

-- 
Julien Grall

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

Re: [PATCH v3 06/17] xen/livepatch/x86/arm32: Force .livepatch.depends section to be uint32_t aligned.

[Wei Liu]

On Tue, Sep 19, 2017 at 12:05:16PM +0100, Julien Grall wrote:
> Hi Konrad,
> 
> On 19/09/17 01:32, Konrad Rzeszutek Wilk wrote:
> > > > +.PHONY: livepatch_depends.h
> > > > +livepatch_depends.h: note.bin
> > > > +	$(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@))
> > > 
> > > It looks quite odd to use a file in firmware/hvmloader for livepatch. Would
> > > it be possible to move mkhex to a generic place?
> > 
> > Like so?
> 
> It is what I had in mind. I CCed Ian and Wei to get feedback from them.
> 

Just move it to a directory called tools/scripts?

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

Re: [PATCH v3 06/17] xen/livepatch/x86/arm32: Force .livepatch.depends section to be uint32_t aligned.

[Konrad Rzeszutek Wilk]

On Wed, Sep 20, 2017 at 03:01:57PM +0100, Wei Liu wrote:
> On Tue, Sep 19, 2017 at 12:05:16PM +0100, Julien Grall wrote:
> > Hi Konrad,
> > 
> > On 19/09/17 01:32, Konrad Rzeszutek Wilk wrote:
> > > > > +.PHONY: livepatch_depends.h
> > > > > +livepatch_depends.h: note.bin
> > > > > +	$(shell (../../../tools/firmware/hvmloader/mkhex $(NOTE_DEPENDS) $^ > $@))
> > > > 
> > > > It looks quite odd to use a file in firmware/hvmloader for livepatch. Would
> > > > it be possible to move mkhex to a generic place?
> > > 
> > > Like so?
> > 
> > It is what I had in mind. I CCed Ian and Wei to get feedback from them.
> > 
> 
> Just move it to a directory called tools/scripts?

I spoke to Wei and Ian on IRC and they are OK with it being in tools/misc - especially
as 'mkrpm','mkdeb' are all in there.

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

[PATCH v3 07/17] livepatch/arm/x86: Strip 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.

The solution to this 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 is fixed in "livepatch: Add local and global symbol resolution."
but that patch is stuck in limbo).

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v3: First version
---
 xen/test/livepatch/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 89ad89dfd5..9e73861732 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) --strip-debug --strip-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) --strip-debug --strip-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) --strip-debug --strip-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) --strip-debug --strip-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

Re: [PATCH v3 07/17] livepatch/arm/x86: Strip note_depends symbol from test-cases.

[Jan Beulich]

>>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote:
> 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.

Did you consider using objcopy's --localize-symbol instead?

Jan


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

Re: [PATCH v3 07/17] livepatch/arm/x86: Strip note_depends symbol from test-cases.

[Konrad Rzeszutek Wilk]

On Tue, Sep 12, 2017 at 08:48:33AM -0600, Jan Beulich wrote:
> >>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote:
> > 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.
> 
> Did you consider using objcopy's --localize-symbol instead?

Yes, 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.

That is each of the livepatch has the node_depends in it, and we can't
load xen_hello_world, followed by xen_replace_world test-case (so
stacking them on top of each other) - as both have the same local
symbol.

(This is fixed in "livepatch: Add local and global symbol resolution."
on which you said:

	> All the 'GLOBAL' have to be unique per livepatch. But the
	> 'LOCAL' can all be the same which means the semantic of 'static'
	> on functions and data variables is the right one.

	I think this is wrong: Afaict your change results in main image and
	patch local symbols to now be treated differently. While this may
	indeed help patches which are meant to replace others, it is going
	to get in the way if a patch wants to reference a local symbol
	already altered (or newly introduced) by a prior one.

(https://www.mail-archive.com/xen-devel@lists.xen.org/msg111710.html)


> 
> Jan
> 

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

Re: [PATCH v3 07/17] livepatch/arm/x86: Strip note_depends symbol from test-cases.

[Jan Beulich]

>>> On 13.09.17 at 01:46, <konrad@kernel.org> wrote:
> On Tue, Sep 12, 2017 at 08:48:33AM -0600, Jan Beulich wrote:
>> >>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote:
>> > 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.
>> 
>> Did you consider using objcopy's --localize-symbol instead?
> 
> Yes, 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.
> 
> That is each of the livepatch has the node_depends in it, and we can't
> load xen_hello_world, followed by xen_replace_world test-case (so
> stacking them on top of each other) - as both have the same local
> symbol.

Oh, right. Then perhaps stripping the symbol is as good or as bad as
deriving the symbol name from e.g. the patch name, or putting some
randomized tag on it.

> (This is fixed in "livepatch: Add local and global symbol resolution."
> on which you said:
> 
> 	> All the 'GLOBAL' have to be unique per livepatch. But the
> 	> 'LOCAL' can all be the same which means the semantic of 'static'
> 	> on functions and data variables is the right one.
> 
> 	I think this is wrong: Afaict your change results in main image and
> 	patch local symbols to now be treated differently. While this may
> 	indeed help patches which are meant to replace others, it is going
> 	to get in the way if a patch wants to reference a local symbol
> 	already altered (or newly introduced) by a prior one.
> 
> (https://www.mail-archive.com/xen-devel@lists.xen.org/msg111710.html)

Right, this is a basically unresolvable ambiguity, I'm afraid. We'd
need a 3rd class of symbols. It may be worth considering to (ab)use
e.g. STV_INTERNAL for this purpose.

Jan


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

Re: [PATCH v3 07/17] livepatch/arm/x86: Strip note_depends symbol from test-cases.

[Konrad Rzeszutek Wilk]

On Wed, Sep 13, 2017 at 02:51:41AM -0600, Jan Beulich wrote:
> >>> On 13.09.17 at 01:46, <konrad@kernel.org> wrote:
> > On Tue, Sep 12, 2017 at 08:48:33AM -0600, Jan Beulich wrote:
> >> >>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote:
> >> > 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.
> >> 
> >> Did you consider using objcopy's --localize-symbol instead?
> > 
> > Yes, 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.
> > 
> > That is each of the livepatch has the node_depends in it, and we can't
> > load xen_hello_world, followed by xen_replace_world test-case (so
> > stacking them on top of each other) - as both have the same local
> > symbol.
> 
> Oh, right. Then perhaps stripping the symbol is as good or as bad as
> deriving the symbol name from e.g. the patch name, or putting some
> randomized tag on it.

Yes, and I had in mind changing the name of it (to prefix it with the
livepatch name) using --redefine-sym.

But then figured it may be just easier to ditch the symbol altogether.

Let me try it out - I do wonder if that would remove the need
for stripping the debug symbols or if that would still trip the issue
I keep on having - which is that the debug section would reference the original
symbol.


> 
> > (This is fixed in "livepatch: Add local and global symbol resolution."
> > on which you said:
> > 
> > 	> All the 'GLOBAL' have to be unique per livepatch. But the
> > 	> 'LOCAL' can all be the same which means the semantic of 'static'
> > 	> on functions and data variables is the right one.
> > 
> > 	I think this is wrong: Afaict your change results in main image and
> > 	patch local symbols to now be treated differently. While this may
> > 	indeed help patches which are meant to replace others, it is going
> > 	to get in the way if a patch wants to reference a local symbol
> > 	already altered (or newly introduced) by a prior one.
> > 
> > (https://www.mail-archive.com/xen-devel@lists.xen.org/msg111710.html)
> 
> Right, this is a basically unresolvable ambiguity, I'm afraid. We'd
> need a 3rd class of symbols. It may be worth considering to (ab)use
> e.g. STV_INTERNAL for this purpose.

Oooooh. Let me look at that. Thank you!
> 
> Jan
> 

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

[PATCH v3 08/17] 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>
---
 xen/test/livepatch/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 9e73861732..6e5b9a3a75 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -54,6 +54,7 @@ xen_hello_world.o: config.h livepatch_depends.h
 $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
 	$(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@
+	$(OBJCOPY) --set-section-flags .livepatch.funcs=alloc,readonly $@
 
 #
 # This target is only accessible if CONFIG_LIVEPATCH is defined, which
@@ -90,6 +91,7 @@ xen_bye_world.o: config.h hello_world_livepatch_depends.h
 $(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_BYE) $^
 	$(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@
+	$(OBJCOPY) --set-section-flags .livepatch.funcs=alloc,readonly $@
 
 xen_replace_world.o: config.h livepatch_depends.h
 
@@ -97,6 +99,7 @@ xen_replace_world.o: config.h livepatch_depends.h
 $(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^
 	$(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@
+	$(OBJCOPY) --set-section-flags .livepatch.funcs=alloc,readonly $@
 
 xen_nop.o: config.h livepatch_depends.h
 
@@ -104,6 +107,7 @@ xen_nop.o: config.h livepatch_depends.h
 $(LIVEPATCH_NOP): xen_nop.o
 	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_NOP) $^
 	$(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@
+	$(OBJCOPY) --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

Re: [PATCH v3 08/17] livepatch/tests: Make sure all .livepatch.funcs sections are read-only

[Jan Beulich]

>>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote:
> --- a/xen/test/livepatch/Makefile
> +++ b/xen/test/livepatch/Makefile
> @@ -54,6 +54,7 @@ xen_hello_world.o: config.h livepatch_depends.h
>  $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o
>  	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
>  	$(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@
> +	$(OBJCOPY) --set-section-flags .livepatch.funcs=alloc,readonly $@

Why multiple objcopy invocations?

Jan


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

Re: [PATCH v3 08/17] livepatch/tests: Make sure all .livepatch.funcs sections are read-only

[Konrad Rzeszutek Wilk]

On Tue, Sep 12, 2017 at 08:49:55AM -0600, Jan Beulich wrote:
> >>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote:
> > --- a/xen/test/livepatch/Makefile
> > +++ b/xen/test/livepatch/Makefile
> > @@ -54,6 +54,7 @@ xen_hello_world.o: config.h livepatch_depends.h
> >  $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o
> >  	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^
> >  	$(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@
> > +	$(OBJCOPY) --set-section-flags .livepatch.funcs=alloc,readonly $@
> 
> Why multiple objcopy invocations?

<scratches his head> I honestly have no idea. I converted it over to be  \
and then have --set-section-flags on the same column as --strip-debug.
> 
> Jan
> 

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

[PATCH v3 09/17] livepatch/arm[32, 64]: Modify livepatch_funcs

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

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
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@oracle.com>
---
 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);
-- 
2.13.3


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

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

[Julien Grall]

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

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

[Konrad Rzeszutek Wilk]

On Thu, Sep 14, 2017 at 02:20:42PM +0100, Julien Grall wrote:
> 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?

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

This patch creates a temporary writeable virtual address space.

But the idea of making the region temporarily writeable is also possible.
Is there a specific register I can use for this?

> 
> > 
> > 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?

Yes, let me update that.
> 
> > 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.

And also this, as there is more to it (As Andrew pointed out).
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > ---
> >   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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

Hi Konrad,

On 19/09/17 01:35, Konrad Rzeszutek Wilk wrote:
> On Thu, Sep 14, 2017 at 02:20:42PM +0100, Julien Grall wrote:
>> 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?
> 
> Yes.
>>
>> I was wondering if we could either defer the call to secure_payload or make
>> the region temporarily writeable?
> 
> This patch creates a temporary writeable virtual address space.
> 
> But the idea of making the region temporarily writeable is also possible.
> Is there a specific register I can use for this?

There is no specific register. I was suggest to call modify_xen_mappings 
on the region with (RW) and then you are done switch back to RO/RX.

I can't see any implication on Arm to temporary switch a mapping from
read-only to read-write. I am not sure for x86.

Cheers,

-- 
Julien Grall

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

[PATCH v3 10/17] 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

[PATCH v3 11/17] livepatch/x86/arm[32, 64]: Use common vmap code for applying.

[Konrad Rzeszutek Wilk]

Patch titled "livepatch/arm[32,64]: Modify livepatch_funcs" added
the infrastructure on ARM [32,64] to use vmap as way to
map read-only regions. On x86 we use a global register.

But there is nothing wrong with using on x86 the same method
as on ARM[32,64] - which is exactly what this patch does.

As result the common code for setting up vmap is now
done in livepatch_quiesce and there is no arch specific
arch_livepatch_quiesce anymore.

The same treatment is applied to arch_livepatch_revive albeit
we still need arch specific code for ARM (to clear the i-cache).

Interestingly the arch_livepatch_revert looks almost the same
on x86 and ARM. See 'livepatch/x86/arm[32,64]: Unify arch_livepatch_revert'

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/arm/livepatch.c        | 64 --------------------------------
 xen/arch/x86/livepatch.c        | 32 +++++++++-------
 xen/common/livepatch.c          | 81 +++++++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/livepatch.h | 13 -------
 xen/include/xen/livepatch.h     | 13 +++++++
 5 files changed, 108 insertions(+), 95 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 2f9ae8e61e..2debb5368c 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -17,57 +17,6 @@
 #undef virt_to_mfn
 #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
 
-struct livepatch_vmap_stash livepatch_vmap;
-
-int arch_livepatch_quiesce(struct livepatch_func *funcs, unsigned int nfuncs)
-{
-    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 ( livepatch_vmap.text || livepatch_vmap.funcs )
-        return -EINVAL;
-
-    text_mfn = virt_to_mfn(_start);
-    text_order = get_order_from_bytes(_end - _start);
-
-    /*
-     * The text section is read-only. So re-map Xen to be able to patch
-     * the code.
-     */
-    vmap_addr = __vmap(&text_mfn, 1U << text_order, 1, 1, PAGE_HYPERVISOR,
-                       VMAP_DEFAULT);
-
-    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;
-}
-
 void arch_livepatch_revive(void)
 {
     /*
@@ -75,19 +24,6 @@ void arch_livepatch_revive(void)
      * arch_livepatch_[apply|revert].
      */
     invalidate_icache();
-
-    if ( livepatch_vmap.text )
-        vunmap(livepatch_vmap.text);
-
-    livepatch_vmap.text = NULL;
-
-    if ( livepatch_vmap.funcs )
-        vunmap(livepatch_vmap.funcs);
-
-    livepatch_vmap.funcs = NULL;
-
-    livepatch_vmap.va = NULL;
-    livepatch_vmap.offset = 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 8522fcbd36..5273f5a176 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -14,18 +14,9 @@
 #include <asm/nmi.h>
 #include <asm/livepatch.h>
 
-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);
-
-    return 0;
-}
-
 void arch_livepatch_revive(void)
 {
-    /* Reinstate WP. */
-    write_cr0(read_cr0() | X86_CR0_WP);
+    /* Nothing to do. */
 }
 
 int arch_livepatch_verify_func(const struct livepatch_func *func)
@@ -54,14 +45,21 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
 {
     uint8_t *old_ptr;
     uint8_t insn[sizeof(func->opaque)];
-    unsigned int len;
+    unsigned int i, len;
+    struct livepatch_func *f;
 
-    old_ptr = func->old_addr;
+    /* Recompute using the vmap. */
+    old_ptr = func->old_addr - (void *)_start + livepatch_vmap.text;
     len = livepatch_insn_len(func);
     if ( !len )
         return;
 
-    memcpy(func->opaque, old_ptr, len);
+    /* Index in the vmap region. */
+    i = livepatch_vmap.va - func;
+    f = (struct livepatch_func *)(livepatch_vmap.funcs + livepatch_vmap.offset) + i;
+
+    memcpy(f->opaque, old_ptr, len);
+
     if ( func->new_addr )
     {
         int32_t val;
@@ -85,7 +83,13 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
  */
 void noinline arch_livepatch_revert(const struct livepatch_func *func)
 {
-    memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
+    uint32_t *new_ptr;
+    unsigned int len;
+
+    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/common/livepatch.c b/xen/common/livepatch.c
index e707802279..eb7d4098fd 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -104,6 +104,12 @@ static struct livepatch_work livepatch_work;
  */
 static DEFINE_PER_CPU(bool_t, work_to_do);
 
+/*
+ * The va of the hypervisor .text region and the livepatch_funcs.
+ * We need this as the normal va are write protected.
+ */
+struct livepatch_vmap_stash livepatch_vmap;
+
 static int get_name(const xen_livepatch_name_t *name, char *n)
 {
     if ( !name->size || name->size > XEN_LIVEPATCH_NAME_SIZE )
@@ -1055,6 +1061,73 @@ static int livepatch_list(xen_sysctl_livepatch_list_t *list)
     return rc ? : idx;
 }
 
+static int livepatch_quiesce(struct livepatch_func *funcs, unsigned int nfuncs)
+{
+    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 ( livepatch_vmap.text || livepatch_vmap.funcs )
+        return -EINVAL;
+
+    text_mfn = _mfn(virt_to_mfn(_start));
+    text_order = get_order_from_bytes(_end - _start);
+
+    /*
+     * The text section is read-only. So re-map Xen to be able to patch
+     * the code.
+     */
+    vmap_addr = __vmap(&text_mfn, 1U << text_order, 1, 1, PAGE_HYPERVISOR,
+                       VMAP_DEFAULT);
+
+    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 = _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;
+}
+
+static void livepatch_revive(void)
+{
+    arch_livepatch_revive();
+
+    if ( livepatch_vmap.text )
+        vunmap(livepatch_vmap.text);
+
+    livepatch_vmap.text = NULL;
+
+    if ( livepatch_vmap.funcs )
+        vunmap(livepatch_vmap.funcs);
+
+    livepatch_vmap.funcs = NULL;
+
+    livepatch_vmap.va = NULL;
+    livepatch_vmap.offset = 0;
+}
+
 /*
  * The following functions get the CPUs into an appropriate state and
  * apply (or revert) each of the payload's functions. This is needed
@@ -1069,7 +1142,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(data->funcs, data->nfuncs);
+    rc = livepatch_quiesce(data->funcs, data->nfuncs);
     if ( rc )
     {
         printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
@@ -1091,7 +1164,7 @@ static int apply_payload(struct payload *data)
     for ( i = 0; i < data->nfuncs; i++ )
         arch_livepatch_apply(&data->funcs[i]);
 
-    arch_livepatch_revive();
+    livepatch_revive();
 
     /*
      * We need RCU variant (which has barriers) in case we crash here.
@@ -1110,7 +1183,7 @@ static int revert_payload(struct payload *data)
 
     printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name);
 
-    rc = arch_livepatch_quiesce(data->funcs, data->nfuncs);
+    rc = livepatch_quiesce(data->funcs, data->nfuncs);
     if ( rc )
     {
         printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
@@ -1132,7 +1205,7 @@ static int revert_payload(struct payload *data)
 
     ASSERT(!local_irq_is_enabled());
 
-    arch_livepatch_revive();
+    livepatch_revive();
 
     /*
      * We need RCU variant (which has barriers) in case we crash here.
diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h
index e030aedced..1d746161a9 100644
--- a/xen/include/asm-arm/livepatch.h
+++ b/xen/include/asm-arm/livepatch.h
@@ -11,19 +11,6 @@
 /* On ARM32,64 instructions are always 4 bytes long. */
 #define ARCH_PATCH_INSN_SIZE 4
 
-/*
- * The va of the hypervisor .text region and the livepatch_funcs.
- * We need this as the normal va are write protected.
- */
-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
 /* 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 a97afb92f9..1659ffcdf0 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -100,6 +100,19 @@ static inline int livepatch_verify_distance(const struct livepatch_func *func)
 
     return 0;
 }
+
+/*
+ * The va of the hypervisor .text region and the livepatch_funcs.
+ * We need this as the normal va are write protected.
+ */
+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 functions are called around the critical region patching live code,
  * for an architecture to take make appropratie global state adjustments.
-- 
2.13.3


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

Re: [PATCH v3 11/17] livepatch/x86/arm[32, 64]: Use common vmap code for applying.

[Andrew Cooper]

On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote:
> Patch titled "livepatch/arm[32,64]: Modify livepatch_funcs" added
> the infrastructure on ARM [32,64] to use vmap as way to
> map read-only regions. On x86 we use a global register.
>
> But there is nothing wrong with using on x86 the same method
> as on ARM[32,64] - which is exactly what this patch does.

Yes there is :)

If you don't make updates to the .text section via the same linear
address as the instructions are being fetched, the Icache doesn't stay
synchronised.  This is VeryBad(tm) when patching the entry paths.

If you want to continue down this route, you need to reintroduce
sync_core() and call it appropriately on all cpus after patching is
complete, and take care to ensure that any spliced patch on the NMI/MCE
handlers still results in valid x86 opcode.

~Andrew

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

[PATCH v3 12/17] livepatch/x86/arm[32, 64]: Unify arch_livepatch_revert

[Konrad Rzeszutek Wilk]

The arch_livepatch_revert is very similar between the platforms.
Lets unify it as much as possible.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/arm/livepatch.c    | 10 +---------
 xen/arch/x86/livepatch.c    | 10 ++--------
 xen/common/livepatch.c      | 14 +++++++++++++-
 xen/include/xen/livepatch.h |  3 +--
 4 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 2debb5368c..e1d5d58f97 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -39,16 +39,8 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
     return 0;
 }
 
-void arch_livepatch_revert(const struct livepatch_func *func)
+void arch_livepatch_revert(uint32_t *new_ptr, unsigned int len)
 {
-    uint32_t *new_ptr;
-    unsigned int len;
-
-    new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text;
-
-    len = livepatch_insn_len(func);
-    memcpy(new_ptr, func->opaque, len);
-
     clean_and_invalidate_dcache_va_range(new_ptr, len);
 }
 
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 5273f5a176..12287d445f 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -81,15 +81,9 @@ void noinline arch_livepatch_apply(struct livepatch_func *func)
  * "noinline" to cause control flow change and thus invalidate I$ and
  * cause refetch after modification.
  */
-void noinline arch_livepatch_revert(const struct livepatch_func *func)
+void noinline arch_livepatch_revert(uint32_t *new_ptr, unsigned int len)
 {
-    uint32_t *new_ptr;
-    unsigned int len;
-
-    new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text;
-
-    len = livepatch_insn_len(func);
-    memcpy(new_ptr, func->opaque, len);
+    /* Nothing to do. */
 }
 
 /*
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index eb7d4098fd..93083cda1a 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1128,6 +1128,18 @@ static void livepatch_revive(void)
     livepatch_vmap.offset = 0;
 }
 
+static void livepatch_revert(const struct livepatch_func *func)
+{
+    uint32_t *new_ptr;
+    unsigned int len;
+
+    new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text;
+
+    len = livepatch_insn_len(func);
+    memcpy(new_ptr, func->opaque, len);
+
+    arch_livepatch_revert(new_ptr, len);
+}
 /*
  * The following functions get the CPUs into an appropriate state and
  * apply (or revert) each of the payload's functions. This is needed
@@ -1191,7 +1203,7 @@ static int revert_payload(struct payload *data)
     }
 
     for ( i = 0; i < data->nfuncs; i++ )
-        arch_livepatch_revert(&data->funcs[i]);
+        livepatch_revert(&data->funcs[i]);
 
     /*
      * Since we are running with IRQs disabled and the hooks may call common
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 1659ffcdf0..065c1a323a 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -117,11 +117,10 @@ extern struct livepatch_vmap_stash livepatch_vmap;
  * 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(struct livepatch_func *func, unsigned int nfuncs);
 void arch_livepatch_revive(void);
 
 void arch_livepatch_apply(struct livepatch_func *func);
-void arch_livepatch_revert(const struct livepatch_func *func);
+void arch_livepatch_revert(uint32_t *new_ptr, unsigned int len);
 void arch_livepatch_post_action(void);
 
 void arch_livepatch_mask(void);
-- 
2.13.3


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

Re: [PATCH v3 12/17] livepatch/x86/arm[32, 64]: Unify arch_livepatch_revert

[Julien Grall]

Hi Konrad,

On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote:
> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
> index 1659ffcdf0..065c1a323a 100644
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -117,11 +117,10 @@ extern struct livepatch_vmap_stash livepatch_vmap;
>    * 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(struct livepatch_func *func, unsigned int nfuncs);

I guess this should belonged to the previous patch?

>   void arch_livepatch_revive(void);
>   
>   void arch_livepatch_apply(struct livepatch_func *func);
> -void arch_livepatch_revert(const struct livepatch_func *func);
> +void arch_livepatch_revert(uint32_t *new_ptr, unsigned int len);
>   void arch_livepatch_post_action(void);
>   
>   void arch_livepatch_mask(void);
> 

Cheers,

-- 
Julien Grall

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

[PATCH v3 13/17] livepatch: Expand spin_debug_disable in [apply|revert]_payload

[Konrad Rzeszutek Wilk]

Under ARM64 the vmap calls were all done with IRQs disabled which
didn't trip the spinlock debug check (as seen on x86):

livepatch.c:1330: livepatch: xen_hello_world: timeout is 30000000ns
livepatch.c:1437: livepatch: xen_hello_world: CPU3 - IPIing the other 3 CPUs
Applying xen_hello_world... (XEN) livepatch: xen_hello_world: Applying 1 functions
Xen BUG at spinlock.c:47
..snip..
    [<ffff82d0802379b1>] spinlock.c#check_lock+0x3e/0x44
    [<ffff82d080237a70>] _spin_lock+0x11/0x4a
    [<ffff82d08023e2de>] __vmap+0x78/0x381
    [<ffff82d080219c13>] livepatch.c#livepatch_quiesce+0xc4/0x1bf
    [<ffff82d080219ee3>] livepatch.c#apply_payload+0x3a/0x102
    [<ffff82d08021a19e>] check_for_livepatch_work+0x1f3/0x390
    [<ffff82d08027b7f4>] domain.c#continue_idle_domain+0x1b/0x22

But are definitly the case on x86 - so we expand the scope
of the spin_debug_disable.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/livepatch.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 93083cda1a..2f5ee1ae75 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1154,22 +1154,22 @@ static int apply_payload(struct payload *data)
     printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
             data->name, data->nfuncs);
 
+    /*
+     * Since we are running with IRQs disabled and the hooks may call common
+     * code - which expects certain spinlocks to run with IRQs enabled - we
+     * temporarily disable the spin locks IRQ state checks.
+     */
+    spin_debug_disable();
     rc = livepatch_quiesce(data->funcs, data->nfuncs);
     if ( rc )
     {
+        spin_debug_enable();
         printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
         return rc;
     }
 
-    /*
-     * Since we are running with IRQs disabled and the hooks may call common
-     * code - which expects certain spinlocks to run with IRQs enabled - we
-     * temporarily disable the spin locks IRQ state checks.
-     */
-    spin_debug_disable();
     for ( i = 0; i < data->n_load_funcs; i++ )
         data->load_funcs[i]();
-    spin_debug_enable();
 
     ASSERT(!local_irq_is_enabled());
 
@@ -1177,6 +1177,7 @@ static int apply_payload(struct payload *data)
         arch_livepatch_apply(&data->funcs[i]);
 
     livepatch_revive();
+    spin_debug_enable();
 
     /*
      * We need RCU variant (which has barriers) in case we crash here.
@@ -1195,9 +1196,16 @@ static int revert_payload(struct payload *data)
 
     printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name);
 
+    /*
+     * Since we are running with IRQs disabled and the hooks may call common
+     * code - which expects certain spinlocks to run with IRQs enabled - we
+     * temporarily disable the spin locks IRQ state checks.
+     */
+    spin_debug_disable();
     rc = livepatch_quiesce(data->funcs, data->nfuncs);
     if ( rc )
     {
+        spin_debug_enable();
         printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
         return rc;
     }
@@ -1205,19 +1213,13 @@ static int revert_payload(struct payload *data)
     for ( i = 0; i < data->nfuncs; i++ )
         livepatch_revert(&data->funcs[i]);
 
-    /*
-     * Since we are running with IRQs disabled and the hooks may call common
-     * code - which expects certain spinlocks to run with IRQs enabled - we
-     * temporarily disable the spin locks IRQ state checks.
-     */
-    spin_debug_disable();
     for ( i = 0; i < data->n_unload_funcs; i++ )
         data->unload_funcs[i]();
-    spin_debug_enable();
 
     ASSERT(!local_irq_is_enabled());
 
     livepatch_revive();
+    spin_debug_enable();
 
     /*
      * We need RCU variant (which has barriers) in case we crash here.
-- 
2.13.3


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

Re: [PATCH v3 13/17] livepatch: Expand spin_debug_disable in [apply|revert]_payload

[Julien Grall]

Hi Konrad,

On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote:
> Under ARM64 the vmap calls were all done with IRQs disabled which
> didn't trip the spinlock debug check (as seen on x86):

Well, I think it does not happen because spin_debug_enable() is never 
called on ARM at boot. So atomic_read(&spin_debug) can never return a 
positive value and hence the check will not be performed.

We should probably enable spin_debug on ARM.

> livepatch.c:1330: livepatch: xen_hello_world: timeout is 30000000ns
> livepatch.c:1437: livepatch: xen_hello_world: CPU3 - IPIing the other 3 CPUs
> Applying xen_hello_world... (XEN) livepatch: xen_hello_world: Applying 1 functions
> Xen BUG at spinlock.c:47
> ..snip..
>      [<ffff82d0802379b1>] spinlock.c#check_lock+0x3e/0x44
>      [<ffff82d080237a70>] _spin_lock+0x11/0x4a
>      [<ffff82d08023e2de>] __vmap+0x78/0x381
>      [<ffff82d080219c13>] livepatch.c#livepatch_quiesce+0xc4/0x1bf
>      [<ffff82d080219ee3>] livepatch.c#apply_payload+0x3a/0x102
>      [<ffff82d08021a19e>] check_for_livepatch_work+0x1f3/0x390
>      [<ffff82d08027b7f4>] domain.c#continue_idle_domain+0x1b/0x22
> 
> But are definitly the case on x86 - so we expand the scope

s/definitly/definitely/

> of the spin_debug_disable.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>   xen/common/livepatch.c | 30 ++++++++++++++++--------------
>   1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 93083cda1a..2f5ee1ae75 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -1154,22 +1154,22 @@ static int apply_payload(struct payload *data)
>       printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
>               data->name, data->nfuncs);
>   
> +    /*
> +     * Since we are running with IRQs disabled and the hooks may call common
> +     * code - which expects certain spinlocks to run with IRQs enabled - we
> +     * temporarily disable the spin locks IRQ state checks.
> +     */
> +    spin_debug_disable();
>       rc = livepatch_quiesce(data->funcs, data->nfuncs);
>       if ( rc )
>       {
> +        spin_debug_enable();
>           printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
>           return rc;
>       }
>   
> -    /*
> -     * Since we are running with IRQs disabled and the hooks may call common
> -     * code - which expects certain spinlocks to run with IRQs enabled - we
> -     * temporarily disable the spin locks IRQ state checks.
> -     */
> -    spin_debug_disable();
>       for ( i = 0; i < data->n_load_funcs; i++ )
>           data->load_funcs[i]();
> -    spin_debug_enable();
>   
>       ASSERT(!local_irq_is_enabled());
>   
> @@ -1177,6 +1177,7 @@ static int apply_payload(struct payload *data)
>           arch_livepatch_apply(&data->funcs[i]);
>   
>       livepatch_revive();
> +    spin_debug_enable();
>   
>       /*
>        * We need RCU variant (which has barriers) in case we crash here.
> @@ -1195,9 +1196,16 @@ static int revert_payload(struct payload *data)
>   
>       printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name);
>   
> +    /*
> +     * Since we are running with IRQs disabled and the hooks may call common
> +     * code - which expects certain spinlocks to run with IRQs enabled - we
> +     * temporarily disable the spin locks IRQ state checks.
> +     */
> +    spin_debug_disable();
>       rc = livepatch_quiesce(data->funcs, data->nfuncs);
>       if ( rc )
>       {
> +        spin_debug_enable();
>           printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
>           return rc;
>       }
> @@ -1205,19 +1213,13 @@ static int revert_payload(struct payload *data)
>       for ( i = 0; i < data->nfuncs; i++ )
>           livepatch_revert(&data->funcs[i]);
>   
> -    /*
> -     * Since we are running with IRQs disabled and the hooks may call common
> -     * code - which expects certain spinlocks to run with IRQs enabled - we
> -     * temporarily disable the spin locks IRQ state checks.
> -     */
> -    spin_debug_disable();
>       for ( i = 0; i < data->n_unload_funcs; i++ )
>           data->unload_funcs[i]();
> -    spin_debug_enable();
>   
>       ASSERT(!local_irq_is_enabled());
>   
>       livepatch_revive();
> +    spin_debug_enable();
>   
>       /*
>        * We need RCU variant (which has barriers) in case we crash here.
> 

Cheers,

-- 
Julien Grall

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

[PATCH v3 14/17] livepatch/x86/arm: arch/x86/mm: generalize do_page_walk() and implement arch_livepatch_lookup_mfn

[Konrad Rzeszutek Wilk]

With this change we can use _do_page_walk() to implement
arch_livepatch_lookup_mfn() which can be used to find out
vmap virtual addresses (as under x86 virt_to_mfn won't work
for vmap, but it does for arm!).

Signed-off-by: Blaise Boscaccy <blaise.boscaccy@oracle.com>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/arm/livepatch.c    |  7 ++++---
 xen/arch/x86/livepatch.c    | 10 ++++++++++
 xen/arch/x86/x86_64/mm.c    | 33 ++++++++++++++++++++++++---------
 xen/include/asm-x86/mm.h    |  1 +
 xen/include/xen/livepatch.h |  3 +++
 5 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index e1d5d58f97..1771b3c558 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -13,9 +13,10 @@
 #include <asm/livepatch.h>
 #include <asm/mm.h>
 
-/* Override macros from asm/page.h to make them work with mfn_t */
-#undef virt_to_mfn
-#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
+mfn_t arch_livepatch_lookup_mfn(unsigned long addr)
+{
+    return _mfn(__virt_to_mfn(addr));
+}
 
 void arch_livepatch_revive(void)
 {
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 12287d445f..667573c6de 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -14,6 +14,16 @@
 #include <asm/nmi.h>
 #include <asm/livepatch.h>
 
+mfn_t arch_livepatch_lookup_mfn(unsigned long addr)
+{
+    unsigned long cr3 = read_cr3() >> PAGE_SHIFT;
+
+    if ( !mfn_valid(_mfn(cr3)) )
+        return INVALID_MFN;
+
+    return _do_page_walk(cr3, addr);
+}
+
 void arch_livepatch_revive(void)
 {
     /* Nothing to do. */
diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
index 11746730b4..f8a963bbba 100644
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -44,29 +44,28 @@ unsigned int __read_mostly m2p_compat_vstart = __HYPERVISOR_COMPAT_VIRT_START;
 
 l2_pgentry_t *compat_idle_pg_table_l2;
 
-void *do_page_walk(struct vcpu *v, unsigned long addr)
+mfn_t _do_page_walk(unsigned long mfn, unsigned long addr)
 {
-    unsigned long mfn = pagetable_get_pfn(v->arch.guest_table);
     l4_pgentry_t l4e, *l4t;
     l3_pgentry_t l3e, *l3t;
     l2_pgentry_t l2e, *l2t;
     l1_pgentry_t l1e, *l1t;
 
-    if ( !is_pv_vcpu(v) || !is_canonical_address(addr) )
-        return NULL;
+    if ( !is_canonical_address(addr) )
+        return INVALID_MFN;
 
     l4t = map_domain_page(_mfn(mfn));
     l4e = l4t[l4_table_offset(addr)];
     unmap_domain_page(l4t);
     if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
-        return NULL;
+        return INVALID_MFN;
 
     l3t = map_l3t_from_l4e(l4e);
     l3e = l3t[l3_table_offset(addr)];
     unmap_domain_page(l3t);
     mfn = l3e_get_pfn(l3e);
     if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) || !mfn_valid(_mfn(mfn)) )
-        return NULL;
+        return INVALID_MFN;
     if ( (l3e_get_flags(l3e) & _PAGE_PSE) )
     {
         mfn += PFN_DOWN(addr & ((1UL << L3_PAGETABLE_SHIFT) - 1));
@@ -78,7 +77,7 @@ void *do_page_walk(struct vcpu *v, unsigned long addr)
     unmap_domain_page(l2t);
     mfn = l2e_get_pfn(l2e);
     if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) || !mfn_valid(_mfn(mfn)) )
-        return NULL;
+        return INVALID_MFN;
     if ( (l2e_get_flags(l2e) & _PAGE_PSE) )
     {
         mfn += PFN_DOWN(addr & ((1UL << L2_PAGETABLE_SHIFT) - 1));
@@ -90,10 +89,26 @@ void *do_page_walk(struct vcpu *v, unsigned long addr)
     unmap_domain_page(l1t);
     mfn = l1e_get_pfn(l1e);
     if ( !(l1e_get_flags(l1e) & _PAGE_PRESENT) || !mfn_valid(_mfn(mfn)) )
-        return NULL;
+        return INVALID_MFN;
 
  ret:
-    return map_domain_page(_mfn(mfn)) + (addr & ~PAGE_MASK);
+    return _mfn(mfn);
+}
+
+void *do_page_walk(struct vcpu *v, unsigned long addr)
+{
+    mfn_t mfn;
+    unsigned long cr3;
+
+    if ( !is_pv_vcpu(v) )
+        return NULL;
+
+    cr3 = pagetable_get_pfn(v->arch.guest_table);
+    mfn = _do_page_walk(cr3, addr);
+    if ( mfn_eq(mfn, INVALID_MFN) )
+        return NULL;
+
+    return map_domain_page(mfn) + (addr & ~PAGE_MASK);
 }
 
 /*
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index bef45e8e9f..224a94494a 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -540,6 +540,7 @@ int new_guest_cr3(mfn_t mfn);
 void make_cr3(struct vcpu *v, mfn_t mfn);
 void update_cr3(struct vcpu *v);
 int vcpu_destroy_pagetables(struct vcpu *);
+mfn_t _do_page_walk(unsigned long mfn, unsigned long addr);
 void *do_page_walk(struct vcpu *v, unsigned long addr);
 
 int __sync_local_execstate(void);
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 065c1a323a..e529f0e7c3 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -72,6 +72,9 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type types
 
 void arch_livepatch_init(void);
 
+#include <xen/mm.h> /* For mfn_t decleration. */
+mfn_t arch_livepatch_lookup_mfn(unsigned long addr);
+
 #include <public/sysctl.h> /* For struct livepatch_func. */
 #include <asm/livepatch.h>
 int arch_livepatch_verify_func(const struct livepatch_func *func);
-- 
2.13.3


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

Re: [PATCH v3 14/17] livepatch/x86/arm: arch/x86/mm: generalize do_page_walk() and implement arch_livepatch_lookup_mfn

[Jan Beulich]

>>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote:
> With this change we can use _do_page_walk() to implement
> arch_livepatch_lookup_mfn() which can be used to find out
> vmap virtual addresses (as under x86 virt_to_mfn won't work
> for vmap, but it does for arm!).

How about using domain_page_map_to_mfn() instead?

Jan


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

Re: [PATCH v3 14/17] livepatch/x86/arm: arch/x86/mm: generalize do_page_walk() and implement arch_livepatch_lookup_mfn

[Konrad Rzeszutek Wilk]

On Tue, Sep 12, 2017 at 08:54:34AM -0600, Jan Beulich wrote:
> >>> On 12.09.17 at 02:37, <konrad@kernel.org> wrote:
> > With this change we can use _do_page_walk() to implement
> > arch_livepatch_lookup_mfn() which can be used to find out
> > vmap virtual addresses (as under x86 virt_to_mfn won't work
> > for vmap, but it does for arm!).
> 
> How about using domain_page_map_to_mfn() instead?

It is very colorfull:

diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 667573c..f90d4c8 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -10,6 +10,7 @@
 #include <xen/vmap.h>
 #include <xen/livepatch_elf.h>
 #include <xen/livepatch.h>
+#include <xen/domain_page.h>
 
 #include <asm/nmi.h>
 #include <asm/livepatch.h>
@@ -17,11 +18,18 @@
 mfn_t arch_livepatch_lookup_mfn(unsigned long addr)
 {
     unsigned long cr3 = read_cr3() >> PAGE_SHIFT;
+    mfn_t r, r2;
+    uint64_t mfn;
 
     if ( !mfn_valid(_mfn(cr3)) )
         return INVALID_MFN;
 
-    return _do_page_walk(cr3, addr);
+    r = _do_page_walk(cr3, addr);
+    mfn = domain_page_map_to_mfn((void *)addr);
+    r2 = _mfn(mfn);
+    WARN_ON( !mfn_eq(r, r2) );
+
+    return r;
 }
 
 void arch_livepatch_revive(void)

..
(XEN) livepatch.c:1450: livepatch: xen_hello_world: CPU1 - IPIing the other 3 CPUs
Applying xen_hello_world... (XEN) livepatch: xen_hello_world: Applying 1 functions
(XEN) Assertion 'va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END' failed at domain_page.c:349
(XEN) ----[ Xen-4.10-unstable  x86_64  debug=y   Tainted:  C   ]----
(XEN) CPU:    1
(XEN) RIP:    e008:[<ffff82d080281d68>] domain_page_map_to_mfn+0x98/0xc8
(XEN) RFLAGS: 0000000000010012   CONTEXT: hypervisor
(XEN) rax: 000000d040200000   rbx: 000000000009e400   rcx: 0000000000000000
(XEN) rdx: 0000001080200000   rsi: 000000009e677000   rdi: ffff82d080200000
(XEN) rbp: ffff84842789fe18   rsp: ffff84842789fe18   r8:  ffff8484278d83c0
(XEN) r9:  0000000000000004   r10: 0000000000000001   r11: 0000000000000001
(XEN) r12: ffff82d080200000   r13: ffff84843e220ab0   r14: 00000009b3af6068
(XEN) r15: 00000000ffffffff   cr0: 000000008005003b   cr4: 00000000000426e0
(XEN) cr3: 000000009e677000   cr2: ffff8800078d4328
(XEN) ds: 002b   es: 002b   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen code around <ffff82d080281d68> (domain_page_map_to_mfn+0x98/0xc8):
(XEN)  48 3d ff ff ff 1f 76 04 <0f> 0b eb fe 48 c1 e7 10 48 c1 ef 1c 48 b8 00 00
(XEN) Xen stack trace from rsp=ffff84842789fe18:
(XEN)    ffff84842789fe38 ffff82d080286d05 0000000000000001 ffff82d08060b180
(XEN)    ffff84842789fe88 ffff82d08021b8b6 ffff84842789fe58 ffff82d0802395a5
(XEN)    000000000000000d ffff84843e220bf0 ffff84843e220ab0 ffff84843e220bf0
(XEN)    ffff84843e220ab0 00000009b3af6068 ffff84842789feb8 ffff82d08021bc38
(XEN)    0000000000000001 0000000000000001 ffff84843e220ab0 ffff84843e220ab0
(XEN)    ffff84842789ff08 ffff82d08021befa ffff84842789fed8 0000000000000246
(XEN)    0000000000000000 ffff84809e9a5000 0000000000000001 ffff84809e6de000
(XEN)    ffff84842788c000 00000000ffffffff ffff84842789fd78 ffff82d08027d4e4
(XEN)    ffff8800394b6000 ffff8800394b6002 0000000000000000 0000000000000000
(XEN)    ffffc900005f3e60 0000000000000001 0000000000000246 0000000000000000
(XEN)    0000000000000000 000000009b810048 0000000000000000 ffffffff810013aa
(XEN)    0000000000000001 deadbeefdeadf00d deadbeefdeadf00d 0000010000000000
(XEN)    ffffffff810013aa 000000000000e033 0000000000000246 ffffc900005f3e48
(XEN)    000000000000e02b 42f0e7438e1e7fc8 06c64a65b2da83f9 6ddeafb16f2755f2
(XEN)    97e5fcb4dae8d065 ea89d67a00000001 ffff84809e9a5000 000001b3b6129680
(XEN)    00000000000426e0
(XEN) Xen call trace:
(XEN)    [<ffff82d080281d68>] domain_page_map_to_mfn+0x98/0xc8
(XEN)    [<ffff82d080286d05>] arch_livepatch_lookup_mfn+0x46/0x61
(XEN)    [<ffff82d08021b8b6>] livepatch.c#livepatch_quiesce+0x45/0x1e4
(XEN)    [<ffff82d08021bc38>] livepatch.c#apply_payload+0x3f/0x109
(XEN)    [<ffff82d08021befa>] check_for_livepatch_work+0x1f8/0x395
(XEN)    [<ffff82d08027d4e4>] domain.c#continue_idle_domain+0x1b/0x22
(XEN) 
(XEN) 
(XEN) ****************************************
(XEN) Panic on CPU 1:
(XEN) Assertion 'va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END' failed at domain_page.c:349
(XEN) ****************************************
(XEN) 
(XEN) Reboot in five seconds...

Which is due to:

287     start = (void *)xen_virt_end;
288     end = (void *)(XEN_VIRT_END - NR_CPUS * PAGE_SIZE);
289 
290     BUG_ON(end <= start);
291 
292     vm_init_type(VMAP_XEN, start, end);


If I modify it a bit:


diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
index 3432a854dd..a95e95b372 100644
--- a/xen/arch/x86/domain_page.c
+++ b/xen/arch/x86/domain_page.c
@@ -344,6 +344,11 @@ unsigned long domain_page_map_to_mfn(const void *ptr)
         pl1e = virt_to_xen_l1e(va);
         BUG_ON(!pl1e);
     }
+    else if ( va >= xen_virt_end && va < (XEN_VIRT_END - NR_CPUS * PAGE_SIZE) )
+    {
+        pl1e = virt_to_xen_l1e(va);
+        BUG_ON(!pl1e);
+    }
     else
     {
         ASSERT(va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END);

And then some extra debug:

(XEN) ptr=0xffff82004000d000, DIRECTMAP_VIRT_START=0xffff848000000000, VMAP_VIRT_START=0xffff82c000000000, VMAP_VIRT_END=0xffff82d000000000, xen_virt_end=0xffff82d080600000, end=0xffff82d0bfe00000
(XEN) ptr=0xffff820040014000, DIRECTMAP_VIRT_START=0xffff848000000000, VMAP_VIRT_START=0xffff82c000000000, VMAP_VIRT(XEN) 
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Assertion 'va >= MAPCACHE_VIRT_START && va < MAPCACHE_VIRT_END' failed at domain_page.c:358
(XEN) ****************************************
(XEN) 
(XEN) Reboot in five seconds...


I will take a look at it later this week, but regardless it will require
some tweaking as well. Do you prefer the domain_page_map_to_mfn path
in instead of  walk_do_page one?

> 
> Jan
> 

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

Re: [PATCH v3 14/17] livepatch/x86/arm: arch/x86/mm: generalize do_page_walk() and implement arch_livepatch_lookup_mfn

[Jan Beulich]

>>> On 13.09.17 at 02:23, <konrad@kernel.org> wrote:
> I will take a look at it later this week, but regardless it will require
> some tweaking as well. Do you prefer the domain_page_map_to_mfn path
> in instead of  walk_do_page one?

Yes - do_page_walk() is really only a debugging helper, not something
to be used for normal production purposes.

Jan


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

Re: [PATCH v3 14/17] livepatch/x86/arm: arch/x86/mm: generalize do_page_walk() and implement arch_livepatch_lookup_mfn

[Julien Grall]

Hi Konrad,

On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote:
> With this change we can use _do_page_walk() to implement
> arch_livepatch_lookup_mfn() which can be used to find out
> vmap virtual addresses (as under x86 virt_to_mfn won't work
> for vmap, but it does for arm!).

The reason is on ARM, virt_to_mfn is taking advantage of the hardware to 
perform the translation.

I am wondering if on x86 you could use vmap_to_mfn.

Cheers,

> 
> Signed-off-by: Blaise Boscaccy <blaise.boscaccy@oracle.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>   xen/arch/arm/livepatch.c    |  7 ++++---
>   xen/arch/x86/livepatch.c    | 10 ++++++++++
>   xen/arch/x86/x86_64/mm.c    | 33 ++++++++++++++++++++++++---------
>   xen/include/asm-x86/mm.h    |  1 +
>   xen/include/xen/livepatch.h |  3 +++
>   5 files changed, 42 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index e1d5d58f97..1771b3c558 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -13,9 +13,10 @@
>   #include <asm/livepatch.h>
>   #include <asm/mm.h>
>   
> -/* Override macros from asm/page.h to make them work with mfn_t */
> -#undef virt_to_mfn
> -#define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
> +mfn_t arch_livepatch_lookup_mfn(unsigned long addr)
> +{
> +    return _mfn(__virt_to_mfn(addr));
> +}
>   
>   void arch_livepatch_revive(void)
>   {
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 12287d445f..667573c6de 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -14,6 +14,16 @@
>   #include <asm/nmi.h>
>   #include <asm/livepatch.h>
>   
> +mfn_t arch_livepatch_lookup_mfn(unsigned long addr)
> +{
> +    unsigned long cr3 = read_cr3() >> PAGE_SHIFT;
> +
> +    if ( !mfn_valid(_mfn(cr3)) )
> +        return INVALID_MFN;
> +
> +    return _do_page_walk(cr3, addr);
> +}
> +
>   void arch_livepatch_revive(void)
>   {
>       /* Nothing to do. */
> diff --git a/xen/arch/x86/x86_64/mm.c b/xen/arch/x86/x86_64/mm.c
> index 11746730b4..f8a963bbba 100644
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -44,29 +44,28 @@ unsigned int __read_mostly m2p_compat_vstart = __HYPERVISOR_COMPAT_VIRT_START;
>   
>   l2_pgentry_t *compat_idle_pg_table_l2;
>   
> -void *do_page_walk(struct vcpu *v, unsigned long addr)
> +mfn_t _do_page_walk(unsigned long mfn, unsigned long addr)
>   {
> -    unsigned long mfn = pagetable_get_pfn(v->arch.guest_table);
>       l4_pgentry_t l4e, *l4t;
>       l3_pgentry_t l3e, *l3t;
>       l2_pgentry_t l2e, *l2t;
>       l1_pgentry_t l1e, *l1t;
>   
> -    if ( !is_pv_vcpu(v) || !is_canonical_address(addr) )
> -        return NULL;
> +    if ( !is_canonical_address(addr) )
> +        return INVALID_MFN;
>   
>       l4t = map_domain_page(_mfn(mfn));
>       l4e = l4t[l4_table_offset(addr)];
>       unmap_domain_page(l4t);
>       if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
> -        return NULL;
> +        return INVALID_MFN;
>   
>       l3t = map_l3t_from_l4e(l4e);
>       l3e = l3t[l3_table_offset(addr)];
>       unmap_domain_page(l3t);
>       mfn = l3e_get_pfn(l3e);
>       if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) || !mfn_valid(_mfn(mfn)) )
> -        return NULL;
> +        return INVALID_MFN;
>       if ( (l3e_get_flags(l3e) & _PAGE_PSE) )
>       {
>           mfn += PFN_DOWN(addr & ((1UL << L3_PAGETABLE_SHIFT) - 1));
> @@ -78,7 +77,7 @@ void *do_page_walk(struct vcpu *v, unsigned long addr)
>       unmap_domain_page(l2t);
>       mfn = l2e_get_pfn(l2e);
>       if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) || !mfn_valid(_mfn(mfn)) )
> -        return NULL;
> +        return INVALID_MFN;
>       if ( (l2e_get_flags(l2e) & _PAGE_PSE) )
>       {
>           mfn += PFN_DOWN(addr & ((1UL << L2_PAGETABLE_SHIFT) - 1));
> @@ -90,10 +89,26 @@ void *do_page_walk(struct vcpu *v, unsigned long addr)
>       unmap_domain_page(l1t);
>       mfn = l1e_get_pfn(l1e);
>       if ( !(l1e_get_flags(l1e) & _PAGE_PRESENT) || !mfn_valid(_mfn(mfn)) )
> -        return NULL;
> +        return INVALID_MFN;
>   
>    ret:
> -    return map_domain_page(_mfn(mfn)) + (addr & ~PAGE_MASK);
> +    return _mfn(mfn);
> +}
> +
> +void *do_page_walk(struct vcpu *v, unsigned long addr)
> +{
> +    mfn_t mfn;
> +    unsigned long cr3;
> +
> +    if ( !is_pv_vcpu(v) )
> +        return NULL;
> +
> +    cr3 = pagetable_get_pfn(v->arch.guest_table);
> +    mfn = _do_page_walk(cr3, addr);
> +    if ( mfn_eq(mfn, INVALID_MFN) )
> +        return NULL;
> +
> +    return map_domain_page(mfn) + (addr & ~PAGE_MASK);
>   }
>   
>   /*
> diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
> index bef45e8e9f..224a94494a 100644
> --- a/xen/include/asm-x86/mm.h
> +++ b/xen/include/asm-x86/mm.h
> @@ -540,6 +540,7 @@ int new_guest_cr3(mfn_t mfn);
>   void make_cr3(struct vcpu *v, mfn_t mfn);
>   void update_cr3(struct vcpu *v);
>   int vcpu_destroy_pagetables(struct vcpu *);
> +mfn_t _do_page_walk(unsigned long mfn, unsigned long addr);
>   void *do_page_walk(struct vcpu *v, unsigned long addr);
>   
>   int __sync_local_execstate(void);
> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
> index 065c1a323a..e529f0e7c3 100644
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -72,6 +72,9 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type types
>   
>   void arch_livepatch_init(void);
>   
> +#include <xen/mm.h> /* For mfn_t decleration. */
> +mfn_t arch_livepatch_lookup_mfn(unsigned long addr);
> +
>   #include <public/sysctl.h> /* For struct livepatch_func. */
>   #include <asm/livepatch.h>
>   int arch_livepatch_verify_func(const struct livepatch_func *func);
> 

-- 
Julien Grall

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

[PATCH v3 15/17] livepatch/x86/arm: Utilize the arch_livepatch_lookup_mfn

[Konrad Rzeszutek Wilk]

Without this patch on x86 we would get a DOUBLE FAULT
as the virt_to_mfn does not lookup virtual addresses
that are in vmap region. This means that the livepatch_vmap.funcs
would point to an incorrect MFN (with either garbage or all
zeros).

We only use the livepatch_vmap.funcs to save the old contents
of the instruction (f->opaque) so during patching all works fine.

But when we revert and copy the contents of f->opaque we would
either get the right values, or zeros (again, depending on where the
MFN is) - and then starting instructions in the unpatched function
would end up with 00000000 .. causing a double fault.

Using the arch_livepatch_lookup_mfn solves the problem and
the applying/reverting works on all platforms properly.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/livepatch.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 2f5ee1ae75..2526d3a0ca 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1073,7 +1073,10 @@ static int livepatch_quiesce(struct livepatch_func *funcs, unsigned int nfuncs)
     if ( livepatch_vmap.text || livepatch_vmap.funcs )
         return -EINVAL;
 
-    text_mfn = _mfn(virt_to_mfn(_start));
+    text_mfn = arch_livepatch_lookup_mfn((unsigned long)_start);
+    if ( mfn_eq(text_mfn, INVALID_MFN) )
+        return -EINVAL;
+
     text_order = get_order_from_bytes(_end - _start);
 
     /*
@@ -1093,7 +1096,14 @@ static int livepatch_quiesce(struct livepatch_func *funcs, unsigned int nfuncs)
     livepatch_vmap.text = vmap_addr;
     livepatch_vmap.offset = offs;
 
-    rodata_mfn = _mfn(virt_to_mfn(va & PAGE_MASK));
+    rodata_mfn = arch_livepatch_lookup_mfn(va & PAGE_MASK);
+    if ( mfn_eq(rodata_mfn, INVALID_MFN) )
+    {
+        vunmap(livepatch_vmap.text);
+        livepatch_vmap.text = NULL;
+        return -EINVAL;
+    }
+
     vmap_addr  = __vmap(&rodata_mfn, size, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT);
     if ( !vmap_addr )
     {
-- 
2.13.3


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

[PATCH v3 16/17] livepatch: Add local and global symbol resolution.

[Konrad Rzeszutek Wilk]

This way we can load livepatches with symbol names that
are the same as long as they are local ('static').

The use case here is to replace an existing livepatch
with a newer one - and one which has the same local symbols.

Without this patch we get:
livepatch.c:819: livepatch: xen_local_symbols: duplicate new symbol: revert_hook

when loading the new livepatch (before doing the replace).

This due to livepatch assuming that all symbols are all
global. With this patch:

readelf --symbols xen_hello_world.livepatch
File: xen_hello_world.livepatch

Symbol table '.symtab' contains 55 entries:
   Num:    Value          Size Type    Bind   Vis      Ndx Name
..snip..
    34: 0000000000000000     4 OBJECT  LOCAL  DEFAULT   25 cnt
    35: 000000000000000a     8 OBJECT  LOCAL  DEFAULT    7 __func__.4654
    36: 0000000000000065    23 FUNC    LOCAL  DEFAULT    2 revert_hook
    37: 000000000000007c    23 FUNC    LOCAL  DEFAULT    2 apply_hook
    38: 0000000000000093    54 FUNC    LOCAL  DEFAULT    2 check_fnc
..snip..

    47: 0000000000000000    54 FUNC    GLOBAL HIDDEN     2 xen_hello_world
    48: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN   UND xen_extra_version
..snip..
    52: 0000000000000000     0 NOTYPE  GLOBAL HIDDEN   UND printk
    53: 0000000000000000    64 OBJECT  GLOBAL HIDDEN    23 livepatch_xen_hello_world

All the 'GLOBAL' have to be unique per livepatch. But the
'LOCAL' can all be the same which means the semantic of 'static'
on functions and data variables is the right one.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v1: New version
v2: No changes
---
 xen/common/livepatch.c          | 21 ++++++++++++++-------
 xen/include/xen/livepatch.h     |  3 ++-
 xen/include/xen/livepatch_elf.h |  7 +++++++
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 2526d3a0ca..5cfce1f2ee 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -187,7 +187,10 @@ unsigned long livepatch_symbols_lookup_by_name(const char *symname)
             if ( !data->symtab[i].new_symbol )
                 continue;
 
-            if ( !strcmp(data->symtab[i].name, symname) )
+            if ( strcmp(data->symtab[i].name, symname) )
+                continue;
+
+            if ( data->symtab[i].global_symbol )
                 return data->symtab[i].value;
         }
     }
@@ -804,6 +807,7 @@ static int build_symbol_table(struct payload *payload,
             symtab[nsyms].size = elf->sym[i].sym->st_size;
             symtab[nsyms].value = elf->sym[i].sym->st_value;
             symtab[nsyms].new_symbol = 0; /* May be overwritten below. */
+            symtab[nsyms].global_symbol = livepatch_sym_is_global(elf->sym[i].sym);
             strtab_len += strlcpy(strtab + strtab_len, elf->sym[i].name,
                                   KSYM_NAME_LEN) + 1;
             nsyms++;
@@ -828,21 +832,24 @@ static int build_symbol_table(struct payload *payload,
             if ( symbols_lookup_by_name(symtab[i].name) ||
                  livepatch_symbols_lookup_by_name(symtab[i].name) )
             {
-                dprintk(XENLOG_ERR, LIVEPATCH "%s: duplicate new symbol: %s\n",
-                        elf->name, symtab[i].name);
+                dprintk(XENLOG_ERR, LIVEPATCH "%s: duplicate new %s symbol: %s\n",
+                        elf->name, symtab[i].global_symbol ? "global" : "local",
+                        symtab[i].name);
                 xfree(symtab);
                 xfree(strtab);
                 return -EEXIST;
             }
             symtab[i].new_symbol = 1;
-            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: new symbol %s\n",
-                     elf->name, symtab[i].name);
+            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: new %s symbol %s\n",
+                     elf->name, symtab[i].global_symbol ? "global" : "local",
+                     symtab[i].name);
         }
         else
         {
             /* new_symbol is not set. */
-            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: overriding symbol %s\n",
-                    elf->name, symtab[i].name);
+            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: overriding %s symbol %s\n",
+                    elf->name, symtab[i].global_symbol ? "global" : "local",
+                    symtab[i].name);
         }
     }
 
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index e529f0e7c3..2f2d3f63e8 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -38,7 +38,8 @@ struct livepatch_symbol {
     const char *name;
     unsigned long value;
     unsigned int size;
-    bool_t new_symbol;
+    unsigned int new_symbol:1;
+    unsigned int global_symbol:1;
 };
 
 int livepatch_op(struct xen_sysctl_livepatch_op *);
diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
index 9ad499ee8b..4d443be1c0 100644
--- a/xen/include/xen/livepatch_elf.h
+++ b/xen/include/xen/livepatch_elf.h
@@ -50,6 +50,13 @@ static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec)
 {
     return !(sec->sh_flags & SHF_ALLOC) || sec->sh_size == 0;
 }
+
+static inline bool livepatch_sym_is_global(const Elf_Sym *sym)
+{
+    return ( (ELF_ST_BIND(sym->st_info) & STB_GLOBAL) &&
+             (sym->st_shndx != SHN_UNDEF) );
+}
+
 #endif /* __XEN_LIVEPATCH_ELF_H__ */
 
 /*
-- 
2.13.3


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

[PATCH v3 17/17] livepatch: Add xen_local_symbols test-case

[Konrad Rzeszutek Wilk]

To exercise the local/global visibility.

With "livepatch: Add local and global symbol resolution."
we can load both xen_hello_world and xen_local_symbols
without having to worry about:

-bash-4.1# xen-livepatch load xen_hello_world.livepatch
Uploading xen_hello_world.livepatch... completed
Applying xen_hello_world... completed
-bash-4.1# xen-livepatch list
 ID                                     | status
----------------------------------------+------------
xen_hello_world                         | APPLIED
-bash-4.1# xen-livepatch upload xen_local_symbols xen_local_symbols.livepatch
Uploading xen_local_symbols.livepatch... failed
(XEN) livepatch.c:819: livepatch: xen_local_symbols: duplicate new symbol: revert_hook

In fact you will see:

livepatch: xen_hello_world: new local symbol revert_hook
livepatch: xen_hello_world: new local symbol apply_hook
livepatch: xen_hello_world: new local symbol check_fnc
livepatch: xen_hello_world: new local symbol hello_world_patch_this_fnc

...
livepatch: xen_local_symbols: new local symbol revert_hook
livepatch: xen_local_symbols: new local symbol apply_hook
livepatch: xen_local_symbols: new local symbol hello_world_patch_this_fnc
..

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v1: First edition
v2: Build with mkhex version of build-id from hypervisor.
---
 xen/test/livepatch/Makefile            | 12 +++++++-
 xen/test/livepatch/xen_local_symbols.c | 53 ++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100644 xen/test/livepatch/xen_local_symbols.c

diff --git a/xen/test/livepatch/Makefile b/xen/test/livepatch/Makefile
index 6e5b9a3a75..f88c8f9c80 100644
--- a/xen/test/livepatch/Makefile
+++ b/xen/test/livepatch/Makefile
@@ -11,11 +11,13 @@ LIVEPATCH := xen_hello_world.livepatch
 LIVEPATCH_BYE := xen_bye_world.livepatch
 LIVEPATCH_REPLACE := xen_replace_world.livepatch
 LIVEPATCH_NOP := xen_nop.livepatch
+LIVEPATCH_LOCAL := xen_local_symbols.livepatch
 
 LIVEPATCHES += $(LIVEPATCH)
 LIVEPATCHES += $(LIVEPATCH_BYE)
 LIVEPATCHES += $(LIVEPATCH_REPLACE)
 LIVEPATCHES += $(LIVEPATCH_NOP)
+LIVEPATCHES += $(LIVEPATCH_LOCAL)
 
 LIVEPATCH_DEBUG_DIR ?= $(DEBUG_DIR)/xen-livepatch
 
@@ -109,5 +111,13 @@ $(LIVEPATCH_NOP): xen_nop.o
 	$(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@
 	$(OBJCOPY) --set-section-flags .livepatch.funcs=alloc,readonly $@
 
+xen_local_symbols.o: config.h livepatch_depends.h
+
+.PHONY: $(LIVEPATCH_LOCAL)
+$(LIVEPATCH_LOCAL): xen_hello_world_func.o xen_local_symbols.o
+	$(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_LOCAL) $^
+	$(OBJCOPY) --strip-debug --strip-symbol=$(NOTE_SYMBOL) $@
+	$(OBJCOPY) --set-section-flags .livepatch.funcs=alloc,readonly $@
+
 .PHONY: livepatch
-livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP)
+livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) $(LIVEPATCH_NOP) $(LIVEPATCH_LOCAL)
diff --git a/xen/test/livepatch/xen_local_symbols.c b/xen/test/livepatch/xen_local_symbols.c
new file mode 100644
index 0000000000..448c489818
--- /dev/null
+++ b/xen/test/livepatch/xen_local_symbols.c
@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) 2017 Oracle and/or its affiliates. All rights reserved.
+ */
+
+#include "config.h"
+#include <xen/lib.h>
+#include <xen/types.h>
+#include <xen/version.h>
+#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
+
+#include <public/sysctl.h>
+#include "livepatch_depends.h"
+
+/* Same name as in xen_hello_world */
+static const char hello_world_patch_this_fnc[] = "xen_extra_version";
+extern const char *xen_hello_world(void);
+
+/*
+ * The hooks are static here (LOCAL) and also in xen_hello_world.c
+ * and their name is exactly the same.
+ */
+static void apply_hook(void)
+{
+    printk(KERN_DEBUG "local_symbols: Hook executing.\n");
+}
+
+static void revert_hook(void)
+{
+    printk(KERN_DEBUG "local_symbols: Hook unloaded.\n");
+}
+
+LIVEPATCH_LOAD_HOOK(apply_hook);
+LIVEPATCH_UNLOAD_HOOK(revert_hook);
+
+struct livepatch_func __section(".livepatch.funcs") livepatch_xen_local_symbols = {
+    .version = LIVEPATCH_PAYLOAD_VERSION,
+    .name = hello_world_patch_this_fnc,
+    .new_addr = xen_hello_world,
+    .old_addr = xen_extra_version,
+    .new_size = NEW_CODE_SZ,
+    .old_size = OLD_CODE_SZ,
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.13.3


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