[PATCH] Livepatch ARM32 fixes thanks to cross-compiler.

[PATCH] Livepatch ARM32 fixes thanks to cross-compiler.

[Konrad Rzeszutek Wilk]

Hey,

A long time ago, in a far away galaxy where ARM CubieTrucks
ruled the world a cross compiled livepatch was attempted
to be loaded.

And behold.

It crashed the hypervisor with an alignment error.

This set of three patches tightens the checks around alignment
to make sure that we catch such errand issues.

Please review at your own leisure.

 xen/arch/arm/arm32/livepatch.c | 18 ++++++++++++--
 xen/arch/arm/arm64/livepatch.c |  6 +++++
 xen/arch/x86/livepatch.c       |  6 +++++
 xen/common/livepatch.c         | 55 ++++++++++++++++++++++++++++++++++++++----
 xen/common/livepatch_elf.c     |  7 ++++++
 xen/include/xen/elfstructs.h   |  2 ++
 xen/include/xen/livepatch.h    |  1 +
 7 files changed, 88 insertions(+), 7 deletions(-)


Konrad Rzeszutek Wilk (3):
      xen/livepatch: Tighten alignment checks.
      livepatch: Include sizes when an mismatch occurs
      xen/livepatch/ARM32: Don't crash on livepatches loaded with wrong alignment.


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

[PATCH v1 1/3] xen/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
two checks: One on the ELF file itself as it is being parsed, and
then when we copy the payload contents in memory - and check the
aligmnent needs there.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/livepatch.c       | 9 +++++++++
 xen/common/livepatch_elf.c   | 7 +++++++
 xen/include/xen/elfstructs.h | 2 ++
 3 files changed, 18 insertions(+)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index ca36161..5d53096 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -406,6 +406,15 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
             ASSERT(offset[i] != UINT_MAX);
 
             elf->sec[i].load_addr = buf + offset[i];
+            if ( elf->sec[i].sec->sh_addralign > 1 &&
+                 ((Elf_Addr)elf->sec[i].load_addr % elf->sec[i].sec->sh_addralign) )
+             {
+                dprintk(XENLOG_ERR, LIVEPATCH "%s: %s @ %p is not aligned (%"PRIuElfWord")\n",
+                        elf->name, elf->sec[i].name, elf->sec[i].load_addr,
+                        elf->sec[i].sec->sh_addralign);
+                rc = -EINVAL;
+                goto out;
+            }
 
             /* Don't copy NOBITS - such as BSS. */
             if ( elf->sec[i].sec->sh_type != SHT_NOBITS )
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index b69e271..852e9c4 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -86,6 +86,13 @@ 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] size (%"PRIuElfWord") is not aligned properly (%"PRIuElfWord")\n",
+                    elf->name, i, sec[i].sec->sh_size, sec[i].sec->sh_addralign);
+            return -EINVAL;
+        }
         else if ( (sec[i].sec->sh_flags & (SHF_WRITE | SHF_ALLOC)) &&
                   sec[i].sec->sh_type == SHT_NOBITS &&
                   sec[i].sec->sh_size > LIVEPATCH_MAX_SIZE )
diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h
index 950e149..edc8862 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	"08u"
 
 #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.9.3


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

[PATCH v1 2/3] 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.

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

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 5d53096..c0eb609 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -520,8 +520,8 @@ static int prepare_payload(struct payload *payload,
     ASSERT(sec);
     if ( sec->sec->sh_size % sizeof(*payload->funcs) )
     {
-        dprintk(XENLOG_ERR, LIVEPATCH "%s: Wrong size of "ELF_LIVEPATCH_FUNC"!\n",
-                elf->name);
+        dprintk(XENLOG_ERR, LIVEPATCH "%s: Wrong size of "ELF_LIVEPATCH_FUNC"! (exp: %zu vs %"PRIuElfWord")\n",
+                elf->name, sizeof(*payload->funcs), sec->sec->sh_size);
         return -EINVAL;
     }
 
@@ -648,8 +648,9 @@ static int prepare_payload(struct payload *payload,
 
         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);
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Wrong size of .bug_frames.%u! (exp: %zu vs %"PRIuElfWord")\n",
+                    elf->name, i, sizeof(*region->frame[i].bugs),
+                    sec->sec->sh_size);
             return -EINVAL;
         }
 
-- 
2.9.3


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

[PATCH v1 3/3] xen/livepatch/ARM32: Don't 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
aligned 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:

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.

However 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 erorr 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.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/arm/arm32/livepatch.c | 18 ++++++++++++++++--
 xen/arch/arm/arm64/livepatch.c |  6 ++++++
 xen/arch/x86/livepatch.c       |  6 ++++++
 xen/common/livepatch.c         | 37 ++++++++++++++++++++++++++++++++++++-
 xen/include/xen/livepatch.h    |  1 +
 5 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index 41378a5..ccb9bf8 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;
@@ -233,7 +239,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 +257,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 +277,15 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
                     elf->name, symndx);
             return -EINVAL;
         }
+        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;
+        }
+
+        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 2247b92..7b36210 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 406eb91..b3cbdac 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 c0eb609..7c52eed 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -495,7 +495,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",
@@ -568,6 +573,12 @@ static int prepare_payload(struct payload *payload,
         if ( sec->sec->sh_size % sizeof(*payload->load_funcs) )
             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;
+        }
         payload->load_funcs = sec->load_addr;
         payload->n_load_funcs = sec->sec->sh_size / sizeof(*payload->load_funcs);
     }
@@ -578,6 +589,12 @@ static int prepare_payload(struct payload *payload,
         if ( sec->sec->sh_size % sizeof(*payload->unload_funcs) )
             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;
+        }
         payload->unload_funcs = sec->load_addr;
         payload->n_unload_funcs = sec->sec->sh_size / sizeof(*payload->unload_funcs);
     }
@@ -653,6 +670,12 @@ static int prepare_payload(struct payload *payload,
                     sec->sec->sh_size);
             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;
+        }
 
         region->frame[i].bugs = sec->load_addr;
         region->frame[i].n_bugs = sec->sec->sh_size /
@@ -671,6 +694,12 @@ static int prepare_payload(struct payload *payload,
                     elf->name, sizeof(*a));
             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;
+        }
 
         start = sec->load_addr;
         end = sec->load_addr + sec->sec->sh_size;
@@ -710,6 +739,12 @@ static int prepare_payload(struct payload *payload,
                     sec->sec->sh_size);
             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;
+        }
 
         s = sec->load_addr;
         e = sec->load_addr + sec->sec->sh_size;
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index fccff94..290bfd5 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -77,6 +77,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.9.3


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

Re: [PATCH v1 1/3] xen/livepatch: Tighten alignment checks.

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 6:53 PM >>>
>--- a/xen/common/livepatch.c
>+++ b/xen/common/livepatch.c
>@@ -406,6 +406,15 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
             >ASSERT(offset[i] != UINT_MAX);
 >
             >elf->sec[i].load_addr = buf + offset[i];
>+            if ( elf->sec[i].sec->sh_addralign > 1 &&

I think ruling out just zero here would be sufficient and look more "natural".

>+                 ((Elf_Addr)elf->sec[i].load_addr % elf->sec[i].sec->sh_addralign) )

The cast here mkes me wonder what it is that you're checking, or
whether the check isn't being done later than it should be done: I'd
expect all such to happen on the original section header, where no
pointer types exist (and hence such a cast shouldn't be needed).

And then - what about the alignment not being a power of 2? Is that
well defined?

>--- a/xen/common/livepatch_elf.c
>+++ b/xen/common/livepatch_elf.c
>@@ -86,6 +86,13 @@ 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 )

Ah, here it is - why the second check further up then?

>+        {
>+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section [%u] size (%"PRIuElfWord") is not aligned properly (%"PRIuElfWord")\n",
>+                    elf->name, i, sec[i].sec->sh_size, sec[i].sec->sh_addralign);

What does section size (being logged here) have to do with the check that
failed? I also question the use of decimal values here - generally I find sizes,
offsets, and alignments easier to deal with when they are being presented
as hex numbers.

>--- 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	"08u"
 
And leading zeros would cause even more confusion, as they would suggest
(at least to C programmers) the number to be octal.

Jan


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

Re: [PATCH v1 2/3] livepatch: Include sizes when an mismatch occurs

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 6:53 PM >>>
>--- a/xen/common/livepatch.c
>+++ b/xen/common/livepatch.c
>@@ -520,8 +520,8 @@ static int prepare_payload(struct payload *payload,
     >ASSERT(sec);
     >if ( sec->sec->sh_size % sizeof(*payload->funcs) )
     >{
>-        dprintk(XENLOG_ERR, LIVEPATCH "%s: Wrong size of "ELF_LIVEPATCH_FUNC"!\n",
>-                elf->name);
>+        dprintk(XENLOG_ERR, LIVEPATCH "%s: Wrong size of "ELF_LIVEPATCH_FUNC"! (exp: %zu vs %"PRIuElfWord")\n",
>+                elf->name, sizeof(*payload->funcs), sec->sec->sh_size);

What you print as expected value isn't really the only permitted one - the
expectation is the value to be a multiple of it. I wonder if the message
text therefore isn't confusing now. Also, how about embedding the actual
size right in the base message, i.e. something like ''Wrong size NNN of ...
(must be multiple of MMM)"?

Jan


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

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

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 6:53 PM >>>
>This issue was observed on ARM32 with a cross compiler for the
>livepatches. Mainly the livepatches .data section size was not
>aligned 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:
>
>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.
>
>However 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.

Which section is it that would not be aligned properly in memory?
.altinstructions, with an alignment of 1, can be placed anywhere. You
shouldn't enforce extra alignment. If higher alignment is needed, the
code producing this section emission needs to be fixed.

>This allows us on ARM32 to erorr out with:
>
>livepatch: xen_hello_world: dest=00a0404a (.altinstructions) is not aligned properly!

I therefore doubt this is a correct diagnostic.

What you may want to consider is silently padding sections to a multiple
of their specified alignment.

Jan


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

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

[Konrad Rzeszutek Wilk]

On Tue, Jul 11, 2017 at 02:06:09PM -0600, Jan Beulich wrote:
> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 6:53 PM >>>
> >This issue was observed on ARM32 with a cross compiler for the
> >livepatches. Mainly the livepatches .data section size was not
> >aligned 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:
> >
> >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.
> >
> >However 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.
> 
> Which section is it that would not be aligned properly in memory?

.altinstructions, thanks to .rodata not being padded properly.

> .altinstructions, with an alignment of 1, can be placed anywhere. You
> shouldn't enforce extra alignment. If higher alignment is needed, the
> code producing this section emission needs to be fixed.

And there is the path to madness :-) We would need to provide an
linker map to make sure that they are with the correct alignment.

Or use the xen.lds on, but we would need to tweak it as the ASSERTS
in it will complain.

> 
> >This allows us on ARM32 to erorr out with:
> >
> >livepatch: xen_hello_world: dest=00a0404a (.altinstructions) is not aligned properly!
> 
> I therefore doubt this is a correct diagnostic.

It is - if we try to muck around with the .altinstructions we will crash
on ARM32.

> 
> What you may want to consider is silently padding sections to a multiple
> of their specified alignment.

Hm. That hadn't occured to me. Let me try that.

> 
> Jan
> 

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

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

[Jan Beulich]

>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 10:34 PM >>>
>On Tue, Jul 11, 2017 at 02:06:09PM -0600, Jan Beulich wrote:
>> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 6:53 PM >>>
>> >This issue was observed on ARM32 with a cross compiler for the
>> >livepatches. Mainly the livepatches .data section size was not
>> >aligned 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:
>> >
>> >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.
>> >
>> >However 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.
>> 
>> Which section is it that would not be aligned properly in memory?
>
>.altinstructions, thanks to .rodata not being padded properly.
>
>> .altinstructions, with an alignment of 1, can be placed anywhere. You
>> shouldn't enforce extra alignment. If higher alignment is needed, the
>> code producing this section emission needs to be fixed.
>
>And there is the path to madness :-) We would need to provide an
>linker map to make sure that they are with the correct alignment.

Why? I'd expect it to be the assembler directives creating contributions to
that section to simply lack a .align or alike. And indeed, there's nothing
like that in ARM's alternative.h. Please see commit 01fe4da624 ("x86: force
suitable alignment in sources rather than in linker script") for further
context.

Jan


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

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

[Jan Beulich]

>>> "Jan Beulich" <jbeulich@suse.com> 07/11/17 10:07 PM >>>
>What you may want to consider is silently padding sections to a multiple
>of their specified alignment.

I actually think now that it was a bad idea to suggest this, in particular in this
context. If you followed that road, you'd end up with a random alignment
dependency of on section on whatever the prior section's alignment happens
to be.

Jan


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

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

[Julien Grall]

Hi,

On 12/07/17 07:07, Jan Beulich wrote:
>>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 10:34 PM >>>
>> On Tue, Jul 11, 2017 at 02:06:09PM -0600, Jan Beulich wrote:
>>>>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 6:53 PM >>>
>>>> This issue was observed on ARM32 with a cross compiler for the
>>>> livepatches. Mainly the livepatches .data section size was not
>>>> aligned 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:
>>>>
>>>> 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.
>>>>
>>>> However 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.
>>>
>>> Which section is it that would not be aligned properly in memory?
>>
>> .altinstructions, thanks to .rodata not being padded properly.
>>
>>> .altinstructions, with an alignment of 1, can be placed anywhere. You
>>> shouldn't enforce extra alignment. If higher alignment is needed, the
>>> code producing this section emission needs to be fixed.
>>
>> And there is the path to madness :-) We would need to provide an
>> linker map to make sure that they are with the correct alignment.
>
> Why? I'd expect it to be the assembler directives creating contributions to
> that section to simply lack a .align or alike. And indeed, there's nothing
> like that in ARM's alternative.h. Please see commit 01fe4da624 ("x86: force
> suitable alignment in sources rather than in linker script") for further
> context.

FWIW, I agree with Jan here. .altinstructions section should contain the 
right alignment in the source code.

Cheers,

-- 
Julien Grall

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

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

[Konrad Rzeszutek Wilk]

On Mon, Jul 24, 2017 at 11:34:26AM +0100, Julien Grall wrote:
> Hi,
> 
> On 12/07/17 07:07, Jan Beulich wrote:
> > > > > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 10:34 PM >>>
> > > On Tue, Jul 11, 2017 at 02:06:09PM -0600, Jan Beulich wrote:
> > > > > > > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 6:53 PM >>>
> > > > > This issue was observed on ARM32 with a cross compiler for the
> > > > > livepatches. Mainly the livepatches .data section size was not
> > > > > aligned 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:
> > > > > 
> > > > > 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.
> > > > > 
> > > > > However 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.
> > > > 
> > > > Which section is it that would not be aligned properly in memory?
> > > 
> > > .altinstructions, thanks to .rodata not being padded properly.
> > > 
> > > > .altinstructions, with an alignment of 1, can be placed anywhere. You
> > > > shouldn't enforce extra alignment. If higher alignment is needed, the
> > > > code producing this section emission needs to be fixed.
> > > 
> > > And there is the path to madness :-) We would need to provide an
> > > linker map to make sure that they are with the correct alignment.
> > 
> > Why? I'd expect it to be the assembler directives creating contributions to
> > that section to simply lack a .align or alike. And indeed, there's nothing
> > like that in ARM's alternative.h. Please see commit 01fe4da624 ("x86: force
> > suitable alignment in sources rather than in linker script") for further
> > context.
> 
> FWIW, I agree with Jan here. .altinstructions section should contain the
> right alignment in the source code.

Sure.

Regardless of that  - should this code which catches errant alignments
of livepatches (say they are generated by other tools) still be in the code base?

It is extra belt and suspenders.

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

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

[Julien Grall]

On 24/07/17 14:14, Konrad Rzeszutek Wilk wrote:
> On Mon, Jul 24, 2017 at 11:34:26AM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 12/07/17 07:07, Jan Beulich wrote:
>>>>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 10:34 PM >>>
>>>> On Tue, Jul 11, 2017 at 02:06:09PM -0600, Jan Beulich wrote:
>>>>>>>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/11/17 6:53 PM >>>
>>>>>> This issue was observed on ARM32 with a cross compiler for the
>>>>>> livepatches. Mainly the livepatches .data section size was not
>>>>>> aligned 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:
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> However 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.
>>>>>
>>>>> Which section is it that would not be aligned properly in memory?
>>>>
>>>> .altinstructions, thanks to .rodata not being padded properly.
>>>>
>>>>> .altinstructions, with an alignment of 1, can be placed anywhere. You
>>>>> shouldn't enforce extra alignment. If higher alignment is needed, the
>>>>> code producing this section emission needs to be fixed.
>>>>
>>>> And there is the path to madness :-) We would need to provide an
>>>> linker map to make sure that they are with the correct alignment.
>>>
>>> Why? I'd expect it to be the assembler directives creating contributions to
>>> that section to simply lack a .align or alike. And indeed, there's nothing
>>> like that in ARM's alternative.h. Please see commit 01fe4da624 ("x86: force
>>> suitable alignment in sources rather than in linker script") for further
>>> context.
>>
>> FWIW, I agree with Jan here. .altinstructions section should contain the
>> right alignment in the source code.
>
> Sure.
>
> Regardless of that  - should this code which catches errant alignments
> of livepatches (say they are generated by other tools) still be in the code base?
>
> It is extra belt and suspenders.
>

I think they should go. We want to limit the number of potential crash 
and give useful feedback to the user :).

Cheers,

-- 
Julien Grall

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