[PATCH v2] Livepatch fixes for v4.10 (v2).

[PATCH v2] Livepatch fixes for v4.10 (v2).

[Konrad Rzeszutek Wilk]

Since v1 (https://lists.xen.org/archives/html/xen-devel/2017-07/msg01127.html)
 - Redid the patches per review comments,
 - Included fixes for the errant sections causing the crash,
 - Rolled in patch from Ross in this patchset,

Hey,

I've been working towards making the livepatch-build-tools to work
on ARM32 and ARM64 - and as part of this I am sending some patches
that fix issues I saw on ARM 32.

The first two patches had been sent in the past (v1) and the one from Ross
as well.

Patches have been tested on ARM32 (CubieTruck), ARM64 (Hikey 960) and x86
with the livepatch tests:

http://xenbits.xen.org/gitweb/?p=xentesttools/bootstrap.git;a=blob_plain;f=root_image/debugspace/livepatch_test.pl;hb=HEAD

(which is a clone of what OSSTest has but without the OSSTest harness)

ARM32 was cross-compiled on Fedora 26 and Ubuntu Xenial - and the payloads
are now working (while with RELEASE-4.9.0 they would crash thanks to alignment
errors).

Thanks.

Konrad Rzeszutek Wilk (4):
      livepatch: Tighten alignment checks.
      livepatch: Include sizes when an mismatch occurs
      xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
      alternative/x86/arm32: Align altinstructions (and altinstr_replacement) sections.

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

 docs/features/livepatch.pandoc    | 103 ++++++++++++++++++++++++++++++++++++++
 docs/misc/livepatch.markdown      |   6 ++-
 xen/arch/arm/arm32/livepatch.c    |  15 ++++++
 xen/arch/arm/arm64/livepatch.c    |   6 +++
 xen/arch/x86/livepatch.c          |   6 +++
 xen/common/Kconfig                |   4 +-
 xen/common/livepatch.c            |  62 ++++++++++++++---------
 xen/common/livepatch_elf.c        |  13 +++++
 xen/include/asm-arm/alternative.h |   4 ++
 xen/include/asm-x86/alternative.h |   2 +
 xen/include/xen/elfstructs.h      |   2 +
 xen/include/xen/livepatch.h       |   1 +
 12 files changed, 196 insertions(+), 28 deletions(-)




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

[PATCH v2 1/5] livepatch: Tighten alignment checks.

[Konrad Rzeszutek Wilk]

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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.
---
 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..4dc1b68871 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 > 1 &&
+                  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 > 1 && sec[i].sec->sh_addralign % 2 )
+        {
+            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

[PATCH v2 2/5] livepatch: Include sizes when an mismatch occurs

[Konrad Rzeszutek Wilk]

From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

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.

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.
---
 xen/common/livepatch.c       | 48 ++++++++++++++++++++++----------------------
 xen/include/xen/elfstructs.h |  2 ++
 2 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 66d532db14..40ff6b3a27 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -457,6 +457,24 @@ static int secure_payload(struct payload *payload, struct livepatch_elf *elf)
     return rc;
 }
 
+static int check_section(const struct livepatch_elf *elf,
+                         const struct livepatch_elf_sec *sec,
+                         const size_t sz, bool zero_ok)
+{
+    if ( !elf || !sec )
+        return -EINVAL;
+
+    if ( (!sec->sec->sh_size && !zero_ok) ||
+        (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 -EINVAL;
+    }
+
+    return 0;
+}
+
 static int check_special_sections(const struct livepatch_elf *elf)
 {
     unsigned int i;
@@ -506,12 +524,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 ( check_section(elf, sec, sizeof(*payload->funcs), true) )
         return -EINVAL;
-    }
 
     payload->funcs = sec->load_addr;
     payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);
@@ -553,7 +567,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 ( check_section(elf, sec, sizeof(*payload->load_funcs), true) )
             return -EINVAL;
 
         payload->load_funcs = sec->load_addr;
@@ -563,7 +577,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 ( check_section(elf, sec, sizeof(*payload->unload_funcs), true) )
             return -EINVAL;
 
         payload->unload_funcs = sec->load_addr;
@@ -634,12 +648,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 ( check_section(elf, sec, sizeof(*region->frame[i].bugs), true) )
             return -EINVAL;
-        }
 
         region->frame[i].bugs = sec->load_addr;
         region->frame[i].n_bugs = sec->sec->sh_size /
@@ -652,12 +662,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 ( check_section(elf, sec, sizeof(*a), true) )
             return -EINVAL;
-        }
 
         start = sec->load_addr;
         end = sec->load_addr + sec->sec->sh_size;
@@ -689,14 +695,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 ( check_section(elf, sec, sizeof(*region->ex), false) )
             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 v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Konrad Rzeszutek Wilk]

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.

See patch titled "alternative/x86/arm32: Align altinstructions
(and altinstr_replacement) sections." which fixes the .altinstructions to
have the proper alignment.

Ignoring this will result in a crash as when we started processing
".rel.altinstructions" for ".altinstructions" with a cross-compiled payload
we would end up poking in an section that was not aligned properly in memory
and crash.

This allows us on ARM32 to error out with:

livepatch: xen_hello_world: dest=00a0404a (.altinstructions) is not aligned properly!

Furthermore we also observe that the alignment is not correct
for other sections (when cross compiling) as such we add the check
in various other places which allows us to get.

livepatch: xen_bye_world: .livepatch.depends alignment is wrong!

instead of a crash.
(See patch "xen/livepatch/x86/arm32: Force livepatch_depends to be uint32_t aligned"
 which fixes the .livepatch.depends alignment)

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
---
 docs/misc/livepatch.markdown   |  6 +++++-
 xen/arch/arm/arm32/livepatch.c | 15 +++++++++++++++
 xen/arch/arm/arm64/livepatch.c |  6 ++++++
 xen/arch/x86/livepatch.c       |  6 ++++++
 xen/common/livepatch.c         | 14 +++++++++++++-
 xen/include/xen/livepatch.h    |  1 +
 6 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 54a6b850cb..c2c4acd3d3 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -279,6 +279,10 @@ 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 SHOULD be four byte aligned. Otherwise
+we risk hitting Data Abort exception as un-aligned manipulation of data is
+prohibited on ARM 32.
+
 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.
@@ -341,7 +345,7 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be
 * `opaque` **MUST** be zero.
 
 The size of the `livepatch_func` array is determined from the ELF section
-size.
+size.  On ARM32 this section must by four-byte aligned.
 
 When applying the patch the hypervisor iterates over each `livepatch_func`
 structure and the core code inserts a trampoline at `old_addr` to `new_addr`.
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 41378a54ae..d48820f8ba 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -112,6 +112,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 32 will crash with Data Abort. */
+    return (uint32_t)sec->load_addr % sizeof(uint32_t);
+};
+
 static s32 get_addend(unsigned char type, void *dest)
 {
     s32 addend = 0;
@@ -266,6 +272,15 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
                     elf->name, symndx);
             return -EINVAL;
         }
+
+        if ( !use_rela )
+            addend = get_addend(type, dest);
+        else if ( (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;
+        }
         else if ( !elf->sym[symndx].sym )
         {
             dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n",
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 2247b925a0..7b36210ccd 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 false;
+}
+
 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..b3cbdac9b7 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 false;
+}
+
 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 40ff6b3a27..13d8f25a4b 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -472,6 +472,13 @@ static int check_section(const struct livepatch_elf *elf,
         return -EINVAL;
     }
 
+    if ( arch_livepatch_verify_alignment(sec) )
+    {
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: %s is not aligned properly!\n",
+               elf->name, sec->name);
+        return -EINVAL;
+    }
+
     return 0;
 }
 
@@ -501,7 +508,12 @@ static int check_special_sections(const struct livepatch_elf *elf)
                     elf->name, names[i]);
             return -EINVAL;
         }
-
+        if ( arch_livepatch_verify_alignment(sec) )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: %s is not aligned properly!\n",
+                    elf->name, names[i]);
+            return -EINVAL;
+        }
         if ( test_and_set_bit(i, found) )
         {
             dprintk(XENLOG_ERR, LIVEPATCH "%s: %s was seen more than once!\n",
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 98ec01216b..e9bab87f28 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -76,6 +76,7 @@ void arch_livepatch_init(void);
 #include <asm/livepatch.h>
 int arch_livepatch_verify_func(const struct livepatch_func *func);
 
+bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec);
 static inline
 unsigned int livepatch_insn_len(const struct livepatch_func *func)
 {
-- 
2.13.3


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

[PATCH v2 4/5] 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.

Enforcing .altinstructions (and also .altinstr_replacement for
completness) 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%

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%

Signed-off-by: Konrad Rzeszutek Wilk <konrad@kernel.org>
---
v2: First version
---
 xen/include/asm-arm/alternative.h | 4 ++++
 xen/include/asm-x86/alternative.h | 2 ++
 2 files changed, 6 insertions(+)

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..e667ccfbdb 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -56,10 +56,12 @@ 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)				\
 	".section .altinstr_replacement, \"ax\"\n"	\
+	".p2align 2\n"					\
 	ALTINSTR_REPLACEMENT(newinstr, feature, number)	\
 	".popsection\n"
 
-- 
2.13.3


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

[PATCH v2 5/5] 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.

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
---
 docs/features/livepatch.pandoc | 103 +++++++++++++++++++++++++++++++++++++++++
 xen/common/Kconfig             |   4 +-
 2 files changed, 105 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..faaf2d1e77
--- /dev/null
+++ b/docs/features/livepatch.pandoc
@@ -0,0 +1,103 @@
+% Live Patching
+% Revision 1
+
+\clearpage
+
+# Basics
+
+---------------- ----------------------------------------------------
+         Status: **Supported**
+
+   Architecture: x86
+
+      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. There are too many OSes and toolchains to consider supporting
+   this. A live patch should be checked to verify that it is valid
+   before loading.
+
+4) Loading an incorrect live patch that results in an insecure host or
+   host crash:
+   If a live patch (whether created using livepatch-build-tools or some
+   alternative) is loaded and it results in an insecure host or host
+   crash due to the content of the live patch being incorrect or the
+   issue being inappropriate to live patch, this is not considered as a
+   security issue.
+
+5) Bugs in the live patch parsing code (the ELF loader):
+   Bugs in the live patch parsing code such as out-of-bounds reads
+   caused by invalid ELF files are not considered to be security issues
+   because the it can only be triggered by a privileged domain.
+
+6) Bugs which allow a guest to prevent the application of a livepatch:
+   A guest should not be able to prevent the application of a live
+   patch. If an unprivileged guest can somehow prevent the application
+   of a live patch despite pausing it (xl pause ...), it shall be
+   treated as a security issue.
+
+Note: It is expected that live patches are tested in a test environment
+before being used in production to avoid unexpected issues. In
+particular, to avoid the issues described by (3), (4), & (5).
+
+There are also some generic security questions which are worth asking:
+
+1) Is guest->host privilege escalation possible?
+
+The new live patching sysctl subops are only accessible to privileged
+domains and this is tested by OSSTest with an XTF test.
+There is a caveat -- an incorrect live patch can introduce a guest->host
+privilege escalation.
+
+2) Is guest user->guest kernel escalation possible?
+
+No, although an incorrect live patch can introduce a guest user->guest
+kernel privilege escalation.
+
+3) Is there any information leakage?
+
+The new live patching sysctl subops are only accessible to privileged
+domains so it is not possible for an unprivileged guest to access the
+list of loaded live patches. This is tested by OSSTest with an XTF test.
+There is a caveat -- an incorrect live patch can introduce an
+information leakage.
+
+4) Can a Denial-of-Service be triggered?
+
+There are no known ways that an unprivileged guest can prevent a live
+patch from being loaded.
+Once again, there is a caveat that an incorrect live patch can introduce
+an arbitrary denial of service.
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index dc8e876439..e9bb849298 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -226,8 +226,8 @@ config CRYPTO
 	bool
 
 config LIVEPATCH
-	bool "Live patching support (TECH PREVIEW)"
-	default n
+	bool "Live patching support"
+	default X86
 	depends on HAS_BUILD_ID = "y"
 	---help---
 	  Allows a running Xen hypervisor to be dynamically patched using
-- 
2.13.3


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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Andrew Cooper]

On 26/07/2017 20:47, Konrad Rzeszutek Wilk wrote:
> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> index 2247b925a0..7b36210ccd 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)
> +{

Semantically, "verify_alignment" implies "the alignment is correct", but
the return value is the opposite.

I'd recommend inverting the sense of these functions, returning true for
x86/arm64, and == 0 for arm32...

> +    /* Unaligned access on ARM 64 is OK. */
> +    return false;
> +}
> +
>  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..b3cbdac9b7 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 false;
> +}
> +
>  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 40ff6b3a27..13d8f25a4b 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -472,6 +472,13 @@ static int check_section(const struct livepatch_elf *elf,
>          return -EINVAL;
>      }
>  
> +    if ( arch_livepatch_verify_alignment(sec) )
> +    {
> +        dprintk(XENLOG_ERR, LIVEPATCH "%s: %s is not aligned properly!\n",
> +               elf->name, sec->name);

It also means this would read as

if ( !arch_livepatch_verify_alignment(sec) )
{
    "$blah not properly aligned"

which is also the usual way around to think.

(Also, bool functions and int functions really do have opposite success
cases, and should be kept that way.)

~Andrew

> +        return -EINVAL;
> +    }
> +
>      return 0;
>  }
>  


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

Re: [PATCH v2 1/5] livepatch: Tighten alignment checks.

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
>--- 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 > 1 &&

As said before, to me this check looks confusing. I'd recommend to only check
for the field to be non-zero.

>+                  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 > 1 && sec[i].sec->sh_addralign % 2 )

What use is this one? Do you perhaps mean to check that the alignment is
a power of 2? In that case a single check of sh_addralign & (sh_addralign - 1)
against zero would be what you want.

Jan


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

Re: [PATCH v2 2/5] livepatch: Include sizes when an mismatch occurs

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:48 PM >>>
>--- a/xen/common/livepatch.c
>+++ b/xen/common/livepatch.c
>@@ -457,6 +457,24 @@ static int secure_payload(struct payload *payload, struct livepatch_elf *elf)
>return rc;
>}
 >
>+static int check_section(const struct livepatch_elf *elf,
>+                         const struct livepatch_elf_sec *sec,
>+                         const size_t sz, bool zero_ok)

I guess you want to drop the const here (or else for consistency add one to the
last parameter). As to the last parameter - I doubt its usefulness: There's one
place where you pass false, and that place looks bogus. I don't see anything
wrong with an empty .ex_table section.

>+{
>+    if ( !elf || !sec )
>+        return -EINVAL;

None of the callers actually uses the return value. Perhaps the function should
return bool?

Jan


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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
>--- a/docs/misc/livepatch.markdown
>+++ b/docs/misc/livepatch.markdown
>@@ -279,6 +279,10 @@ 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 SHOULD be four byte aligned. Otherwise
>+we risk hitting Data Abort exception as un-aligned manipulation of data is
>+prohibited on ARM 32.

This (and hence the rest of the patch) is not in line with the outcome of the
earlier discussion we had. Nothing is wrong with a section having smaller
alignment, as long as there are no 32-bit (or wider, but I don't think there
are any such) relocations against such a section. And even if there were, I
think it should rather be the code doing the relocations needing to cope, as
I don't think the ARM ELF ABI imposes any such restriction.

Jan


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

Re: [PATCH v2 4/5] alternative/x86/arm32: Align altinstructions (and altinstr_replacement) sections.

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
>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%

This looks unexpected, and hence I'd like to ask for an explanation. If
anything I'd expect the image size to grow (slightly).

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

Can't this then be accompanied by dropping the (over-)alignment
done in xen.lds.S?

>ALTINSTR_ENTRY(feature, number)			\
>".section .discard,\"a\",@progbits\n"		\
>DISCARD_ENTRY(number)				\
>".section .altinstr_replacement, \"ax\"\n"	\
>+	".p2align 2\n"					\

This surely isn't needed on x86.

Jan


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

Re: [PATCH v2 5/5] livepatch: Declare live patching as a supported feature

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:48 PM >>>
>From: Ross Lagerwall <ross.lagerwall@citrix.com>
>
>See docs/features/livepatch.pandoc for the details.
>
>Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>Signed-off-by: Konrad Rzeszutek Wilk <konrad@kernel.org>

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


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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Konrad Rzeszutek Wilk]

On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote:
> >>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
> >--- a/docs/misc/livepatch.markdown
> >+++ b/docs/misc/livepatch.markdown
> >@@ -279,6 +279,10 @@ 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 SHOULD be four byte aligned. Otherwise
> >+we risk hitting Data Abort exception as un-aligned manipulation of data is
> >+prohibited on ARM 32.
> 
> This (and hence the rest of the patch) is not in line with the outcome of the
> earlier discussion we had. Nothing is wrong with a section having smaller
> alignment, as long as there are no 32-bit (or wider, but I don't think there
> are any such) relocations against such a section. And even if there were, I
> think it should rather be the code doing the relocations needing to cope, as
> I don't think the ARM ELF ABI imposes any such restriction.

The idea behind this patch is to give advance warnings. Akin to what
2ff229643b739e2fd0cd0536ee9fca506cfa92f8
"xen/livepatch: Don't crash on encountering STN_UNDEF relocations" did.

The other patches in this series fix the alignment issues.

The ARM ELF ABI (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf)

says:

4.3.5 Section Alignment
There is no minimum alignment required for a section. However, sections containing thumb code must be at least
16-bit aligned and sections containing ARM code must be at least 32-bit aligned.
Platform standards may set a limit on the maximum alignment that they can guarantee (normally the page size).



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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/31/17 6:04 PM >>>
>On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote:
>> >>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
>> >--- a/docs/misc/livepatch.markdown
>> >+++ b/docs/misc/livepatch.markdown
>> >@@ -279,6 +279,10 @@ 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 SHOULD be four byte aligned. Otherwise
>> >+we risk hitting Data Abort exception as un-aligned manipulation of data is
>> >+prohibited on ARM 32.
>> 
>> This (and hence the rest of the patch) is not in line with the outcome of the
>> earlier discussion we had. Nothing is wrong with a section having smaller
>> alignment, as long as there are no 32-bit (or wider, but I don't think there
>> are any such) relocations against such a section. And even if there were, I
>> think it should rather be the code doing the relocations needing to cope, as
>> I don't think the ARM ELF ABI imposes any such restriction.
>
>The idea behind this patch is to give advance warnings. Akin to what
>2ff229643b739e2fd0cd0536ee9fca506cfa92f8
>"xen/livepatch: Don't crash on encountering STN_UNDEF relocations" did.
>
>The other patches in this series fix the alignment issues.
>
>The ARM ELF ABI (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf)
>
>says:
>
>4.3.5 Section Alignment
>There is no minimum alignment required for a section. However, sections containing thumb code must be at least
>16-bit aligned and sections containing ARM code must be at least 32-bit aligned.
>Platform standards may set a limit on the maximum alignment that they can guarantee (normally the page size).

Note the "thumb code" and "ARM code" in here - iirc you're checking _all_
sections, not just ones containing code.

Jan





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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Konrad Rzeszutek Wilk]

On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote:
> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/31/17 6:04 PM >>>
> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote:
> >> >>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
> >> >--- a/docs/misc/livepatch.markdown
> >> >+++ b/docs/misc/livepatch.markdown
> >> >@@ -279,6 +279,10 @@ 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 SHOULD be four byte aligned. Otherwise
> >> >+we risk hitting Data Abort exception as un-aligned manipulation of data is
> >> >+prohibited on ARM 32.
> >> 
> >> This (and hence the rest of the patch) is not in line with the outcome of the
> >> earlier discussion we had. Nothing is wrong with a section having smaller
> >> alignment, as long as there are no 32-bit (or wider, but I don't think there
> >> are any such) relocations against such a section. And even if there were, I
> >> think it should rather be the code doing the relocations needing to cope, as
> >> I don't think the ARM ELF ABI imposes any such restriction.
> >
> >The idea behind this patch is to give advance warnings. Akin to what
> >2ff229643b739e2fd0cd0536ee9fca506cfa92f8
> >"xen/livepatch: Don't crash on encountering STN_UNDEF relocations" did.
> >
> >The other patches in this series fix the alignment issues.
> >
> >The ARM ELF ABI (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf)
> >
> >says:
> >
> >4.3.5 Section Alignment
> >There is no minimum alignment required for a section. However, sections containing thumb code must be at least
> >16-bit aligned and sections containing ARM code must be at least 32-bit aligned.
> >Platform standards may set a limit on the maximum alignment that they can guarantee (normally the page size).
> 
> Note the "thumb code" and "ARM code" in here - iirc you're checking _all_
> sections, not just ones containing code.

I can fix the code to only do the check for 'X' ones:

  [ 2] .text             PROGBITS         0000000000000000  00000070
       00000000000000ca  0000000000000000  AX       0     0     16
  [ 4] .altinstr_replace PROGBITS         0000000000000000  0000013c
       000000000000000b  0000000000000000  AX       0     0     4
  [ 5] .fixup            PROGBITS         0000000000000000  00000147
       000000000000000d  0000000000000000  AX       0     0     1


And also have the check in the relocation - which right now are
32-bit: R_ARM_ABS32, R_ARM_REL32, R_ARM_MOVW_ABS_NC, R_ARM_MOVT_ABS,
R_ARM_CALL, R_ARM_JUMP24 so will leave the code as in
arch_livepatch_perform.

But neither one of those is going to help in catching livepatches
that have the wrong alignment without relocations and not executable.
For example .livepatch.depends

Thoughts on how you would want to catch those?

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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Jan Beulich]

>>> On 07.09.17 at 19:36, <konrad@kernel.org> wrote:
> On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote:
>> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/31/17 6:04 PM >>>
>> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote:
>> >> >>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
>> >> >--- a/docs/misc/livepatch.markdown
>> >> >+++ b/docs/misc/livepatch.markdown
>> >> >@@ -279,6 +279,10 @@ 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 SHOULD be four byte aligned. Otherwise
>> >> >+we risk hitting Data Abort exception as un-aligned manipulation of data is
>> >> >+prohibited on ARM 32.
>> >> 
>> >> This (and hence the rest of the patch) is not in line with the outcome of 
> the
>> >> earlier discussion we had. Nothing is wrong with a section having smaller
>> >> alignment, as long as there are no 32-bit (or wider, but I don't think there
>> >> are any such) relocations against such a section. And even if there were, I
>> >> think it should rather be the code doing the relocations needing to cope, 
> as
>> >> I don't think the ARM ELF ABI imposes any such restriction.
>> >
>> >The idea behind this patch is to give advance warnings. Akin to what
>> >2ff229643b739e2fd0cd0536ee9fca506cfa92f8
>> >"xen/livepatch: Don't crash on encountering STN_UNDEF relocations" did.
>> >
>> >The other patches in this series fix the alignment issues.
>> >
>> >The ARM ELF ABI 
> (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf 
> )
>> >
>> >says:
>> >
>> >4.3.5 Section Alignment
>> >There is no minimum alignment required for a section. However, sections 
> containing thumb code must be at least
>> >16-bit aligned and sections containing ARM code must be at least 32-bit 
> aligned.
>> >Platform standards may set a limit on the maximum alignment that they can 
> guarantee (normally the page size).
>> 
>> Note the "thumb code" and "ARM code" in here - iirc you're checking _all_
>> sections, not just ones containing code.
> 
> I can fix the code to only do the check for 'X' ones:
> 
>   [ 2] .text             PROGBITS         0000000000000000  00000070
>        00000000000000ca  0000000000000000  AX       0     0     16
>   [ 4] .altinstr_replace PROGBITS         0000000000000000  0000013c
>        000000000000000b  0000000000000000  AX       0     0     4
>   [ 5] .fixup            PROGBITS         0000000000000000  00000147
>        000000000000000d  0000000000000000  AX       0     0     1
> 
> 
> And also have the check in the relocation - which right now are
> 32-bit: R_ARM_ABS32, R_ARM_REL32, R_ARM_MOVW_ABS_NC, R_ARM_MOVT_ABS,
> R_ARM_CALL, R_ARM_JUMP24 so will leave the code as in
> arch_livepatch_perform.

Relocations applicable to code only _may_ be acceptable to have
such an alignment check (but I could see cases where even that
might be too aggressive), but afaik R_ARM_ABS32 isn't a code
only one (out of the set listed above), so I doubt this should have
an alignment check.

> But neither one of those is going to help in catching livepatches
> that have the wrong alignment without relocations and not executable.
> For example .livepatch.depends

What does "wrong alignment" mean when there's no code involved?
I think what you want to detect simply can't be detected reliably,
without risking false positives.

Jan


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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Konrad Rzeszutek Wilk]

On Fri, Sep 08, 2017 at 03:30:07AM -0600, Jan Beulich wrote:
> >>> On 07.09.17 at 19:36, <konrad@kernel.org> wrote:
> > On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote:
> >> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/31/17 6:04 PM >>>
> >> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote:
> >> >> >>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
> >> >> >--- a/docs/misc/livepatch.markdown
> >> >> >+++ b/docs/misc/livepatch.markdown
> >> >> >@@ -279,6 +279,10 @@ 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 SHOULD be four byte aligned. Otherwise
> >> >> >+we risk hitting Data Abort exception as un-aligned manipulation of data is
> >> >> >+prohibited on ARM 32.
> >> >> 
> >> >> This (and hence the rest of the patch) is not in line with the outcome of 
> > the
> >> >> earlier discussion we had. Nothing is wrong with a section having smaller
> >> >> alignment, as long as there are no 32-bit (or wider, but I don't think there
> >> >> are any such) relocations against such a section. And even if there were, I
> >> >> think it should rather be the code doing the relocations needing to cope, 
> > as
> >> >> I don't think the ARM ELF ABI imposes any such restriction.
> >> >
> >> >The idea behind this patch is to give advance warnings. Akin to what
> >> >2ff229643b739e2fd0cd0536ee9fca506cfa92f8
> >> >"xen/livepatch: Don't crash on encountering STN_UNDEF relocations" did.
> >> >
> >> >The other patches in this series fix the alignment issues.
> >> >
> >> >The ARM ELF ABI 
> > (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf 
> > )
> >> >
> >> >says:
> >> >
> >> >4.3.5 Section Alignment
> >> >There is no minimum alignment required for a section. However, sections 
> > containing thumb code must be at least
> >> >16-bit aligned and sections containing ARM code must be at least 32-bit 
> > aligned.
> >> >Platform standards may set a limit on the maximum alignment that they can 
> > guarantee (normally the page size).
> >> 
> >> Note the "thumb code" and "ARM code" in here - iirc you're checking _all_
> >> sections, not just ones containing code.
> > 
> > I can fix the code to only do the check for 'X' ones:
> > 
> >   [ 2] .text             PROGBITS         0000000000000000  00000070
> >        00000000000000ca  0000000000000000  AX       0     0     16
> >   [ 4] .altinstr_replace PROGBITS         0000000000000000  0000013c
> >        000000000000000b  0000000000000000  AX       0     0     4
> >   [ 5] .fixup            PROGBITS         0000000000000000  00000147
> >        000000000000000d  0000000000000000  AX       0     0     1
> > 
> > 
> > And also have the check in the relocation - which right now are
> > 32-bit: R_ARM_ABS32, R_ARM_REL32, R_ARM_MOVW_ABS_NC, R_ARM_MOVT_ABS,
> > R_ARM_CALL, R_ARM_JUMP24 so will leave the code as in
> > arch_livepatch_perform.
> 
> Relocations applicable to code only _may_ be acceptable to have
> such an alignment check (but I could see cases where even that
> might be too aggressive), but afaik R_ARM_ABS32 isn't a code
> only one (out of the set listed above), so I doubt this should have
> an alignment check.
> 
> > But neither one of those is going to help in catching livepatches
> > that have the wrong alignment without relocations and not executable.
> > For example .livepatch.depends
> 
> What does "wrong alignment" mean when there's no code involved?

Anything which we try to access as a structure, or unsigned int,
that is not aligned to four bytes.

For example accessing .livepatch.depends from memory and blowing
up (hypervisor crashes) b/c it does not start at an four byte aligned
location.

> I think what you want to detect simply can't be detected reliably,
> without risking false positives.
> 
> Jan
> 

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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Jan Beulich]

>>> On 09.09.17 at 14:05, <konrad@kernel.org> wrote:
> On Fri, Sep 08, 2017 at 03:30:07AM -0600, Jan Beulich wrote:
>> >>> On 07.09.17 at 19:36, <konrad@kernel.org> wrote:
>> > On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote:
>> >> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/31/17 6:04 PM >>>
>> >> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote:
>> >> >> >>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
>> >> >> >--- a/docs/misc/livepatch.markdown
>> >> >> >+++ b/docs/misc/livepatch.markdown
>> >> >> >@@ -279,6 +279,10 @@ 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 SHOULD be four byte aligned. Otherwise
>> >> >> >+we risk hitting Data Abort exception as un-aligned manipulation of data is
>> >> >> >+prohibited on ARM 32.
>> >> >> 
>> >> >> This (and hence the rest of the patch) is not in line with the outcome of 
>> > the
>> >> >> earlier discussion we had. Nothing is wrong with a section having smaller
>> >> >> alignment, as long as there are no 32-bit (or wider, but I don't think 
> there
>> >> >> are any such) relocations against such a section. And even if there were, 
> I
>> >> >> think it should rather be the code doing the relocations needing to cope, 
>> > as
>> >> >> I don't think the ARM ELF ABI imposes any such restriction.
>> >> >
>> >> >The idea behind this patch is to give advance warnings. Akin to what
>> >> >2ff229643b739e2fd0cd0536ee9fca506cfa92f8
>> >> >"xen/livepatch: Don't crash on encountering STN_UNDEF relocations" did.
>> >> >
>> >> >The other patches in this series fix the alignment issues.
>> >> >
>> >> >The ARM ELF ABI 
>> > 
> (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf 
> 
>> > )
>> >> >
>> >> >says:
>> >> >
>> >> >4.3.5 Section Alignment
>> >> >There is no minimum alignment required for a section. However, sections 
>> > containing thumb code must be at least
>> >> >16-bit aligned and sections containing ARM code must be at least 32-bit 
>> > aligned.
>> >> >Platform standards may set a limit on the maximum alignment that they can 
>> > guarantee (normally the page size).
>> >> 
>> >> Note the "thumb code" and "ARM code" in here - iirc you're checking _all_
>> >> sections, not just ones containing code.
>> > 
>> > I can fix the code to only do the check for 'X' ones:
>> > 
>> >   [ 2] .text             PROGBITS         0000000000000000  00000070
>> >        00000000000000ca  0000000000000000  AX       0     0     16
>> >   [ 4] .altinstr_replace PROGBITS         0000000000000000  0000013c
>> >        000000000000000b  0000000000000000  AX       0     0     4
>> >   [ 5] .fixup            PROGBITS         0000000000000000  00000147
>> >        000000000000000d  0000000000000000  AX       0     0     1
>> > 
>> > 
>> > And also have the check in the relocation - which right now are
>> > 32-bit: R_ARM_ABS32, R_ARM_REL32, R_ARM_MOVW_ABS_NC, R_ARM_MOVT_ABS,
>> > R_ARM_CALL, R_ARM_JUMP24 so will leave the code as in
>> > arch_livepatch_perform.
>> 
>> Relocations applicable to code only _may_ be acceptable to have
>> such an alignment check (but I could see cases where even that
>> might be too aggressive), but afaik R_ARM_ABS32 isn't a code
>> only one (out of the set listed above), so I doubt this should have
>> an alignment check.
>> 
>> > But neither one of those is going to help in catching livepatches
>> > that have the wrong alignment without relocations and not executable.
>> > For example .livepatch.depends
>> 
>> What does "wrong alignment" mean when there's no code involved?
> 
> Anything which we try to access as a structure, or unsigned int,
> that is not aligned to four bytes.
> 
> For example accessing .livepatch.depends from memory and blowing
> up (hypervisor crashes) b/c it does not start at an four byte aligned
> location.

Hmm, as long as the relocation isn't required to be against aligned
fields only (mandated by the processor ABI) I think the code doing
the relocations would instead need to split the access, rather than
calling the section misaligned or increasing alignment beyond what
the ELF section headers say.

Jan

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

Re: [PATCH v2 4/5] alternative/x86/arm32: Align altinstructions (and altinstr_replacement) sections.

[Konrad Rzeszutek Wilk]

[-- Attachment #1: Type: text/plain, Size: 6387 bytes --]

On Mon, Jul 31, 2017 at 08:01:18AM -0600, Jan Beulich wrote:
> >>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
> >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%
> 
> This looks unexpected, and hence I'd like to ask for an explanation. If
> anything I'd expect the image size to grow (slightly).

With just:

diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index db4f08e0e7..43d1851f86 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)                           \

$ ~/linux/scripts/bloat-o-meter /tmp/xen-syms.baseline.debug xen-syms
add/remove: 0/0 grow/shrink: 1/2 up/down: 87/-237 (-150)
function                                     old     new   delta
sh_page_fault__guest_4                      8557    8644     +87
map_pages_to_xen                            4406    4322     -84
sh_x86_emulate_write__guest_4                444     291    -153
Total: Before=3315124, After=3314974, chg -0.00%
[konrad@localhost xen]$ 

I get the same amount if the '.p2align 2' is in .altinstr_replacement":

diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index db4f08e0e7..201683e29e 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -60,6 +60,7 @@ extern void alternative_instructions(void);
 	".section .discard,\"a\",@progbits\n"		\
 	DISCARD_ENTRY(number)				\
 	".section .altinstr_replacement, \"ax\"\n"	\
+    ".p2align 2\n" \
 	ALTINSTR_REPLACEMENT(newinstr, feature, number)	\
 	".popsection\n"
 
For fun I changed p2align to 1:

diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index db4f08e0e7..13a7204e6b 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -60,6 +60,7 @@ extern void alternative_instructions(void);
        ".section .discard,\"a\",@progbits\n"           \
        DISCARD_ENTRY(number)                           \
        ".section .altinstr_replacement, \"ax\"\n"      \
+    ".p2align 1\n" \
        ALTINSTR_REPLACEMENT(newinstr, feature, number) \
        ".popsection\n"
 
[konrad@localhost xen]$ ~/linux/scripts/bloat-o-meter /tmp/xen-syms.baseline.debug xen-syms
add/remove: 0/0 grow/shrink: 1/2 up/down: 87/-237 (-150)
function                                     old     new   delta
sh_page_fault__guest_4                      8557    8644     +87
map_pages_to_xen                            4406    4322     -84
sh_x86_emulate_write__guest_4                444     291    -153
Total: Before=3315124, After=3314974, chg -0.00%

And then to 0.

--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -60,6 +60,7 @@ extern void alternative_instructions(void);
        ".section .discard,\"a\",@progbits\n"           \
        DISCARD_ENTRY(number)                           \
        ".section .altinstr_replacement, \"ax\"\n"      \
+    ".p2align 0\n" \
        ALTINSTR_REPLACEMENT(newinstr, feature, number) \
        ".popsection\n"
 
[konrad@localhost xen]$ ~/linux/scripts/bloat-o-meter /tmp/xen-syms.baseline.debug xen-syms
add/remove: 0/0 grow/shrink: 1/2 up/down: 87/-237 (-150)
function                                     old     new   delta
sh_page_fault__guest_4                      8557    8644     +87
map_pages_to_xen                            4406    4322     -84
sh_x86_emulate_write__guest_4                444     291    -153
Total: Before=3315124, After=3314974, chg -0.00%


WTF?!

[konrad@localhost xen]$ git diff
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index db4f08e0e7..6c3d59a7df 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -60,6 +60,7 @@ extern void alternative_instructions(void);
        ".section .discard,\"a\",@progbits\n"           \
        DISCARD_ENTRY(number)                           \
        ".section .altinstr_replacement, \"ax\"\n"      \
+    "# HI!\n" \
        ALTINSTR_REPLACEMENT(newinstr, feature, number) \
        ".popsection\n"
 
[konrad@localhost xen]$ ~/linux/scripts/bloat-o-meter /tmp/xen-syms.baseline.debug xen-syms
add/remove: 0/0 grow/shrink: 1/2 up/down: 87/-237 (-150)
function                                     old     new   delta
sh_page_fault__guest_4                      8557    8644     +87
map_pages_to_xen                            4406    4322     -84
sh_x86_emulate_write__guest_4                444     291    -153
Total: Before=3315124, After=3314974, chg -0.00%


In other words, just adding an useless comment makes it smaller!?


Since this seems to be happening with arch/x86/mm.o, so I compiled the
.S for both the baseline and then one with the "HI!" comment.

Looking at the diff (attached) the difference seems to be that the
compiler optimized some of the operations.

In other words the bloat-o-meter for x86 is useless for this.
> 
> >--- a/xen/include/asm-x86/alternative.h
> >+++ b/xen/include/asm-x86/alternative.h
> >@@ -56,10 +56,12 @@ extern void alternative_instructions(void);
>  >
> >#define ALTERNATIVE_N(newinstr, feature, number)	\
> >".pushsection .altinstructions,\"a\"\n"		\
> >+	".p2align 2\n"					\
> 
> Can't this then be accompanied by dropping the (over-)alignment
> done in xen.lds.S?

Yes.

And also in the arm one.
> 
> >ALTINSTR_ENTRY(feature, number)			\
> >".section .discard,\"a\",@progbits\n"		\
> >DISCARD_ENTRY(number)				\
> >".section .altinstr_replacement, \"ax\"\n"	\
> >+	".p2align 2\n"					\
> 
> This surely isn't needed on x86.

Nope. But I added to be in sync with the other. But it can be removed.
> 
> Jan
> 

[-- Attachment #2: diff.gz --]
[-- Type: application/gzip, Size: 284675 bytes --]

[-- Attachment #3: mm.S.baseline.gz --]
[-- Type: application/gzip, Size: 298056 bytes --]

[-- Attachment #4: mm.S.new.gz --]
[-- Type: application/gzip, Size: 298052 bytes --]

[-- Attachment #5: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Konrad Rzeszutek Wilk]

On Mon, Sep 11, 2017 at 03:01:15AM -0600, Jan Beulich wrote:
> >>> On 09.09.17 at 14:05, <konrad@kernel.org> wrote:
> > On Fri, Sep 08, 2017 at 03:30:07AM -0600, Jan Beulich wrote:
> >> >>> On 07.09.17 at 19:36, <konrad@kernel.org> wrote:
> >> > On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote:
> >> >> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/31/17 6:04 PM >>>
> >> >> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote:
> >> >> >> >>> Konrad Rzeszutek Wilk <konrad@kernel.org> 07/26/17 9:50 PM >>>
> >> >> >> >--- a/docs/misc/livepatch.markdown
> >> >> >> >+++ b/docs/misc/livepatch.markdown
> >> >> >> >@@ -279,6 +279,10 @@ 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 SHOULD be four byte aligned. Otherwise
> >> >> >> >+we risk hitting Data Abort exception as un-aligned manipulation of data is
> >> >> >> >+prohibited on ARM 32.
> >> >> >> 
> >> >> >> This (and hence the rest of the patch) is not in line with the outcome of 
> >> > the
> >> >> >> earlier discussion we had. Nothing is wrong with a section having smaller
> >> >> >> alignment, as long as there are no 32-bit (or wider, but I don't think 
> > there
> >> >> >> are any such) relocations against such a section. And even if there were, 
> > I
> >> >> >> think it should rather be the code doing the relocations needing to cope, 
> >> > as
> >> >> >> I don't think the ARM ELF ABI imposes any such restriction.
> >> >> >
> >> >> >The idea behind this patch is to give advance warnings. Akin to what
> >> >> >2ff229643b739e2fd0cd0536ee9fca506cfa92f8
> >> >> >"xen/livepatch: Don't crash on encountering STN_UNDEF relocations" did.
> >> >> >
> >> >> >The other patches in this series fix the alignment issues.
> >> >> >
> >> >> >The ARM ELF ABI 
> >> > 
> > (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf 
> > 
> >> > )
> >> >> >
> >> >> >says:
> >> >> >
> >> >> >4.3.5 Section Alignment
> >> >> >There is no minimum alignment required for a section. However, sections 
> >> > containing thumb code must be at least
> >> >> >16-bit aligned and sections containing ARM code must be at least 32-bit 
> >> > aligned.
> >> >> >Platform standards may set a limit on the maximum alignment that they can 
> >> > guarantee (normally the page size).
> >> >> 
> >> >> Note the "thumb code" and "ARM code" in here - iirc you're checking _all_
> >> >> sections, not just ones containing code.
> >> > 
> >> > I can fix the code to only do the check for 'X' ones:
> >> > 
> >> >   [ 2] .text             PROGBITS         0000000000000000  00000070
> >> >        00000000000000ca  0000000000000000  AX       0     0     16
> >> >   [ 4] .altinstr_replace PROGBITS         0000000000000000  0000013c
> >> >        000000000000000b  0000000000000000  AX       0     0     4
> >> >   [ 5] .fixup            PROGBITS         0000000000000000  00000147
> >> >        000000000000000d  0000000000000000  AX       0     0     1
> >> > 
> >> > 
> >> > And also have the check in the relocation - which right now are
> >> > 32-bit: R_ARM_ABS32, R_ARM_REL32, R_ARM_MOVW_ABS_NC, R_ARM_MOVT_ABS,
> >> > R_ARM_CALL, R_ARM_JUMP24 so will leave the code as in
> >> > arch_livepatch_perform.
> >> 
> >> Relocations applicable to code only _may_ be acceptable to have
> >> such an alignment check (but I could see cases where even that
> >> might be too aggressive), but afaik R_ARM_ABS32 isn't a code
> >> only one (out of the set listed above), so I doubt this should have
> >> an alignment check.
> >> 
> >> > But neither one of those is going to help in catching livepatches
> >> > that have the wrong alignment without relocations and not executable.
> >> > For example .livepatch.depends
> >> 
> >> What does "wrong alignment" mean when there's no code involved?
> > 
> > Anything which we try to access as a structure, or unsigned int,
> > that is not aligned to four bytes.
> > 
> > For example accessing .livepatch.depends from memory and blowing
> > up (hypervisor crashes) b/c it does not start at an four byte aligned
> > location.
> 
> Hmm, as long as the relocation isn't required to be against aligned
> fields only (mandated by the processor ABI) I think the code doing
> the relocations would instead need to split the access, rather than
> calling the section misaligned or increasing alignment beyond what
> the ELF section headers say.

Maybe the serial log would explain this better:

xend_config_format     : 4
Executing: '(set -e;cd /root/test/livepatch;xen-livepatch load xen_bye_world.livepatch)' ..(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .note.gnu.build-id at 00a08000
(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .text at 00a06000
(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .rodata at 00a08024
(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .rodata.str1.4 at 00a08038
(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .livepatch.depends at 00a08043
(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .livepatch.funcs at 00a07000
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 0xa08000 (.note.gnu.build-id)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 0xa06000 (.text)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 0xa08024 (.rodata)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 0xa08038 (.rodata.str1.4)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 0xa08043 (.livepatch.depends)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 0xa07000 (.livepatch.funcs)
(XEN) livepatch_elf.c:319: livepatch: xen_bye_world: Absolute symbol: xen_bye_world_func.c => 00000000
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: $a => 0xa06000 (.text)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: .LC0 => 0xa08038 (.rodata.str1.4)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: $d => 0xa08038 (.rodata.str1.4)
(XEN) livepatch_elf.c:319: livepatch: xen_bye_world: Absolute symbol: xen_bye_world.c => 00000000
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: $d => 0xa08024 (.rodata)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: bye_world_patch_this_fnc => 0xa08024 (.rodata)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: $d => 0xa07000 (.livepatch.funcs)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: livepatch_xen_bye_world => 0xa07000 (.livepatch.funcs)
(XEN) livepatch_elf.c:314: livepatch: xen_bye_world: Undefined symbol resolved: xen_extra_version => 0x23ffe0
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: xen_bye_world => 0xa06000 (.text)
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.9Hello World  arm32  debug=y   Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     002400a0 xen_build_id_check+0x8/0xe8
(XEN) CPSR:   2007001a MODE:Hypervisor
(XEN)      R0: 00a08043 R1: 00000024 R2: 43fde940 R3: 43fde944
(XEN)      R4: 002960b0 R5: 00000000 R6: 00000000 R7: 43fde8b8
(XEN)      R8: 002960bc R9: 00000000 R10:1001c000 R11:43fd7dd4 R12:00000000
(XEN) HYP: SP: 43fd7cd4 LR: 0021a9c0
(XEN) 
(XEN)   VTCR_EL2: 80003558
(XEN)  VTTBR_EL2: 00010000bf9ce000
(XEN) 
(XEN)  SCTLR_EL2: 30cd187f
(XEN)    HCR_EL2: 000000000038663f
(XEN)  TTBR0_EL2: 00000000bfb12000
(XEN) 
(XEN)    ESR_EL2: 94000021
(XEN)  HPFAR_EL2: 000000000001c810
(XEN)      HDFAR: 00a0804b
(XEN)      HIFAR: 31dc8c18
(XEN) 
(XEN) Xen stack trace from sp=43fd7cd4:
(XEN)    94000021 1001d890 00a07000 0028b7f0 0028b410 00000008 0028b974 0028b410
(XEN)    00000001 00000002 00000007 43fd7d0c 00263bd4 43fd7d44 43fde958 00001d64
(XEN)    1001c000 43fde9e0 43fdeb98 0000001e 43fdeb60 43fdeb70 00000018 5f6e6578
(XEN)    5f657962 6c726f77 00000064 00000098 00009680 0028c293 400265e0 00000000
(XEN)    00989680 00000000 00989680 00000000 43fd7dc0 43fd7d7c 0025ffb0 02786800
(XEN)    0007c340 0007c200 00262f94 7c340b80 00000088 00000088 00000088 fc340004
(XEN)    b6fa7004 02786800 43fd7e40 43fd7dd4 0025bafc 00318580 43fd7ec8 43fd7e3c
(XEN)    00319614 43fd7f58 0029624c 00318580 002e1f80 00318580 00000000 43fd7eec
(XEN)    0023bbe4 1ca3fcf8 b6fa7004 00000000 00000000 00001cb5 00008c40 43fd7eac
(XEN)    00000000 43fd7e0c 0025ffb0 025ef920 0006f7c9 0006f600 00262f94 6f7c9b80
(XEN)    00000017 00000001 00000001 025ef920 ef7c9017 0029709c ef7c9017 43fd7e6c
(XEN)    0025b7b4 00000000 00318580 0000001b 0000000f 00000000 00000000 b6faa004
(XEN)    00000000 0000000e 00000000 00001d64 00000000 b6fa8004 00000000 b6e6b9cc
(XEN)    00000000 00023064 00000000 00010660 00000000 00023014 00000000 00010660
(XEN)    b6de12c0 b6de1780 00000001 00000000 b6f92fdf 00000000 00000001 00000001
(XEN)    00000000 b6de12c0 b6f575ac 00023118 00000002 43fd7f58 0023b21c 43fd7f58
(XEN)    00000000 00305000 beb7b8e0 edcf2000 00000000 43fd7f54 002673c4 00000000
(XEN)    2007009a 43fd7f24 43fd7f24 00000000 00000021 00000004 43fd7f58 00000000
(XEN)    00318580 ee2b8840 7c340f5f 00e00000 eff80800 00000d38 ffefed38 43fd7f44
(XEN)    00000000 c0a57000 00000000 00305000 beb7b8e0 edcf2000 00000000 43fd7f58
(XEN) Xen call trace:
(XEN)    [<002400a0>] xen_build_id_check+0x8/0xe8 (PC)
(XEN)    [<0021a9c0>] livepatch_op+0x768/0x1610 (LR)
(XEN)    [<0023bbe4>] do_sysctl+0x9c8/0xa9c
(XEN)    [<002673c4>] do_trap_guest_sync+0x11e0/0x177c
(XEN)    [<0026b6a0>] entry.o#return_from_trap+0/0x4
(XEN) 
(XEN) 
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) 
(XEN) ****************************************
(XEN) 
(XEN) Reboot in five seconds...


Keep in mind that this only happens if I cross-compile ARM32 under x86.

If I compile the test-case under ARM32 it works OK (as the
.livepatch.depends ends up being aligned to four bytes).

Now having said I am going to be posting a v3 patchset shortly
which has a fix for this ("xen/livepatch/x86/arm32: Force
.livepatch.depends section to be uint32_t aligned.").

But nonethless it may be better to have the extra belt
and suspends to catch these issues during run-time if somebody
does mess up with the alignment by mistake.

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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Jan Beulich]

>>> On 12.09.17 at 02:22, <konrad@kernel.org> wrote:
> On Mon, Sep 11, 2017 at 03:01:15AM -0600, Jan Beulich wrote:
>> Hmm, as long as the relocation isn't required to be against aligned
>> fields only (mandated by the processor ABI) I think the code doing
>> the relocations would instead need to split the access, rather than
>> calling the section misaligned or increasing alignment beyond what
>> the ELF section headers say.
> 
> Maybe the serial log would explain this better:
> 
> xend_config_format     : 4
> Executing: '(set -e;cd /root/test/livepatch;xen-livepatch load 
> xen_bye_world.livepatch)' ..(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .note.gnu.build-id at 00a08000
> (XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .text at 00a06000
> (XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .rodata at 00a08024
> (XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .rodata.str1.4 at 00a08038
> (XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .livepatch.depends at 00a08043
>[...]
> Keep in mind that this only happens if I cross-compile ARM32 under x86.

That would suggest a build environment / build tools issue then:
Cross builds aren't supposed to produce binaries different from
native builds.

> If I compile the test-case under ARM32 it works OK (as the
> .livepatch.depends ends up being aligned to four bytes).

So why is that? What entity is creating this section (or the
directive(s) to create it)?

Jan


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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Konrad Rzeszutek Wilk]

On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote:
> >>> On 12.09.17 at 02:22, <konrad@kernel.org> wrote:
> > On Mon, Sep 11, 2017 at 03:01:15AM -0600, Jan Beulich wrote:
> >> Hmm, as long as the relocation isn't required to be against aligned
> >> fields only (mandated by the processor ABI) I think the code doing
> >> the relocations would instead need to split the access, rather than
> >> calling the section misaligned or increasing alignment beyond what
> >> the ELF section headers say.
> > 
> > Maybe the serial log would explain this better:
> > 
> > xend_config_format     : 4
> > Executing: '(set -e;cd /root/test/livepatch;xen-livepatch load 
> > xen_bye_world.livepatch)' ..(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .note.gnu.build-id at 00a08000
> > (XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .text at 00a06000
> > (XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .rodata at 00a08024
> > (XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .rodata.str1.4 at 00a08038
> > (XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .livepatch.depends at 00a08043
> >[...]
> > Keep in mind that this only happens if I cross-compile ARM32 under x86.
> 
> That would suggest a build environment / build tools issue then:
> Cross builds aren't supposed to produce binaries different from
> native builds.

Hm, the gcc parameters on both native and cross compiler have same args:

konrad@osstest:/srv/cubietruck/source$ diff native.invocation /tmp/cross.invocation 
1c1
< gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /source/xen.orig.git/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_bye_world.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_bye_world.o.d -msoft-float -mcpu=cortex-a15 -I/source/xen.orig.git/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -c xen_bye_world.c -o xen_bye_world.o
---
> arm-linux-gnu-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /home/konrad/A20/xen.git/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_bye_world.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_bye_world.o.d -msoft-float -mcpu=cortex-a15 -I/home/konrad/A20/xen.git/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs   -c xen_bye_world.c -o xen_bye_world.o


> 
> > If I compile the test-case under ARM32 it works OK (as the
> > .livepatch.depends ends up being aligned to four bytes).
> 
> So why is that? What entity is creating this section (or the
> directive(s) to create it)?

gcc

Looking at the xen_bye_world.o produced by cross-compiler:

xen_bye_world.o:     file format elf32-littlearm

Contents of section .rodata:
 0000 78656e5f 65787472 615f7665 7273696f  xen_extra_versio
 0010 6e00                                 n. 

And native:

xen_bye_world.o:     file format elf32-littlearm

Contents of section .rodata:
 0000 78656e5f 65787472 615f7665 7273696f  xen_extra_versio
 0010 6e000000                             n...      

(The cross compiler is 7.0.1, while native is 4.9.2).


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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Jan Beulich]

>>> On 18.09.17 at 21:37, <konrad.wilk@oracle.com> wrote:
> On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote:
>> >>> On 12.09.17 at 02:22, <konrad@kernel.org> wrote:
>> > If I compile the test-case under ARM32 it works OK (as the
>> > .livepatch.depends ends up being aligned to four bytes).
>> 
>> So why is that? What entity is creating this section (or the
>> directive(s) to create it)?
> 
> gcc
> 
> Looking at the xen_bye_world.o produced by cross-compiler:
> 
> xen_bye_world.o:     file format elf32-littlearm
> 
> Contents of section .rodata:
>  0000 78656e5f 65787472 615f7665 7273696f  xen_extra_versio
>  0010 6e00                                 n. 
> 
> And native:
> 
> xen_bye_world.o:     file format elf32-littlearm
> 
> Contents of section .rodata:
>  0000 78656e5f 65787472 615f7665 7273696f  xen_extra_versio
>  0010 6e000000                             n...      

This may rather be a gas than a gcc behavioral difference. What's
the alignment of .rodata in both cases?

Jan


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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Konrad Rzeszutek Wilk]

On Tue, Sep 19, 2017 at 09:04:44AM -0600, Jan Beulich wrote:
> >>> On 18.09.17 at 21:37, <konrad.wilk@oracle.com> wrote:
> > On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote:
> >> >>> On 12.09.17 at 02:22, <konrad@kernel.org> wrote:
> >> > If I compile the test-case under ARM32 it works OK (as the
> >> > .livepatch.depends ends up being aligned to four bytes).
> >> 
> >> So why is that? What entity is creating this section (or the
> >> directive(s) to create it)?
> > 
> > gcc
> > 
> > Looking at the xen_bye_world.o produced by cross-compiler:
> > 
> > xen_bye_world.o:     file format elf32-littlearm
> > 
> > Contents of section .rodata:
> >  0000 78656e5f 65787472 615f7665 7273696f  xen_extra_versio
> >  0010 6e00                                 n. 
> > 
> > And native:
> > 
> > xen_bye_world.o:     file format elf32-littlearm
> > 
> > Contents of section .rodata:
> >  0000 78656e5f 65787472 615f7665 7273696f  xen_extra_versio
> >  0010 6e000000                             n...      
> 
> This may rather be a gas than a gcc behavioral difference. What's
> the alignment of .rodata in both cases?

Cross:

* on the livepatch:
..snip..
  [ 4] .rodata           PROGBITS        00000000 000074 000012 00   A  0   0  4
  [ 5] .rodata.str1.4    PROGBITS        00000000 000088 00000b 01 AMS  0   0  4
  [ 6] .livepatch.depend PROGBITS        00000000 000093 000024 00   A  0   0  1

* on the .o file:
Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
.. snip..
  [ 1] .text             PROGBITS        00000000 000034 000000 00  AX  0   0  1
  [ 2] .data             PROGBITS        00000000 000034 000000 00  WA  0   0  1
  [ 3] .bss              NOBITS          00000000 000034 000000 00  WA  0   0  1
  [ 4] .rodata           PROGBITS        00000000 000034 000014 00   A  0   0  4
  [ 5] .livepatch.funcs  PROGBITS        00000000 000048 000034 00  WA  0   0  4

Native:

 * on the livepatch:
..snip..
  [ 4] .rodata           PROGBITS        00000000 000074 000014 00   A  0   0  4
  [ 5] .rodata.str1.4    PROGBITS        00000000 000088 00000c 01 AMS  0   0  4
  [ 6] .livepatch.depend PROGBITS        00000000 000094 000024 00   A  0   0  1

* on the .o file:
..snip..
  [ 1] .text             PROGBITS        00000000 000034 000000 00  AX  0   0  1
  [ 2] .data             PROGBITS        00000000 000034 000000 00  WA  0   0  1
  [ 3] .bss              NOBITS          00000000 000034 000000 00  WA  0   0  1
  [ 4] .rodata           PROGBITS        00000000 000034 000012 00   A  0   0  4
  [ 5] .livepatch.funcs  PROGBITS        00000000 000048 000034 00  WA  0   0  4



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

Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.

[Jan Beulich]

>>> On 20.09.17 at 17:12, <konrad@kernel.org> wrote:
> On Tue, Sep 19, 2017 at 09:04:44AM -0600, Jan Beulich wrote:
>> >>> On 18.09.17 at 21:37, <konrad.wilk@oracle.com> wrote:
>> > On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote:
>> >> >>> On 12.09.17 at 02:22, <konrad@kernel.org> wrote:
>> >> > If I compile the test-case under ARM32 it works OK (as the
>> >> > .livepatch.depends ends up being aligned to four bytes).
>> >> 
>> >> So why is that? What entity is creating this section (or the
>> >> directive(s) to create it)?
>> > 
>> > gcc
>> > 
>> > Looking at the xen_bye_world.o produced by cross-compiler:
>> > 
>> > xen_bye_world.o:     file format elf32-littlearm
>> > 
>> > Contents of section .rodata:
>> >  0000 78656e5f 65787472 615f7665 7273696f  xen_extra_versio
>> >  0010 6e00                                 n. 
>> > 
>> > And native:
>> > 
>> > xen_bye_world.o:     file format elf32-littlearm
>> > 
>> > Contents of section .rodata:
>> >  0000 78656e5f 65787472 615f7665 7273696f  xen_extra_versio
>> >  0010 6e000000                             n...      
>> 
>> This may rather be a gas than a gcc behavioral difference. What's
>> the alignment of .rodata in both cases?
> 
> Cross:
> 
> * on the livepatch:
> ..snip..
>   [ 4] .rodata           PROGBITS        00000000 000074 000012 00   A  0   0  4
>   [ 5] .rodata.str1.4    PROGBITS        00000000 000088 00000b 01 AMS  0   0  4
>   [ 6] .livepatch.depend PROGBITS        00000000 000093 000024 00   A  0   0  1
> 
> * on the .o file:
> Section Headers:
>   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
> .. snip..
>   [ 1] .text             PROGBITS        00000000 000034 000000 00  AX  0   0  1
>   [ 2] .data             PROGBITS        00000000 000034 000000 00  WA  0   0  1
>   [ 3] .bss              NOBITS          00000000 000034 000000 00  WA  0   0  1
>   [ 4] .rodata           PROGBITS        00000000 000034 000014 00   A  0   0  4
>   [ 5] .livepatch.funcs  PROGBITS        00000000 000048 000034 00  WA  0   0  4

Hard to believe - a 0x14 bytes section gets shrunk to 0x12 bytes
by (presumably) ld -r?

> Native:
> 
>  * on the livepatch:
> ..snip..
>   [ 4] .rodata           PROGBITS        00000000 000074 000014 00   A  0   0  4
>   [ 5] .rodata.str1.4    PROGBITS        00000000 000088 00000c 01 AMS  0   0  4
>   [ 6] .livepatch.depend PROGBITS        00000000 000094 000024 00   A  0   0  1
> 
> * on the .o file:
> ..snip..
>   [ 1] .text             PROGBITS        00000000 000034 000000 00  AX  0   0  1
>   [ 2] .data             PROGBITS        00000000 000034 000000 00  WA  0   0  1
>   [ 3] .bss              NOBITS          00000000 000034 000000 00  WA  0   0  1
>   [ 4] .rodata           PROGBITS        00000000 000034 000012 00   A  0   0  4
>   [ 5] .livepatch.funcs  PROGBITS        00000000 000048 000034 00  WA  0   0  4

With things being the other way around here - did you perhaps mix
up files?

Jan



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