Re: [PATCH v2 20/20] livepatch: ARM32 support.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Thu, Aug 25, 2016 at 09:37:35AM -0400, Konrad Rzeszutek Wilk wrote:
> The patch piggybacks on: livepatch: Initial ARM64 support, which
> brings up all of the neccessary livepatch infrastructure pieces in.
> 
> This patch adds three major pieces:
> 
>  1) ELF relocations. ARM32 uses SHT_REL instead of SHT_RELA which
>     means the adddendum had to be extracted from within the
>     instruction. Which required parsing BL/BLX, B/BL<cond>,
>     MOVT, and MOVW instructions.
> 
>     The code was written from scratch using the ARM ELF manual
>     (and the ARM Architecture Reference Manual)
> 
>  2) Inserting an trampoline. We use the B (branch to address)
>     which uses an offset that is based on the PC value: PC + imm32.
>     Because we insert the branch at the start of the old function
>     we have to account for the instruction already being fetched
>     and subtract -4 from the delta (new_addr - old_addr).
> 
>  3) Allows the test-cases to be built under ARM 32.
>     The "livepatch: tests: Make them compile under ARM64"
>     put in the right infrastructure for it and we piggyback on it.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> 
> v2: First submission.
> ---
>  xen/arch/arm/arm32/livepatch.c | 252 ++++++++++++++++++++++++++++++++++++++++-
>  xen/arch/arm/arm64/livepatch.c |   7 ++
>  xen/arch/arm/livepatch.c       |   7 --
>  xen/common/Kconfig             |   2 +-
>  xen/include/xen/elfstructs.h   |  24 +++-
>  xen/test/Makefile              |   2 -
>  xen/test/livepatch/Makefile    |   3 +
>  7 files changed, 284 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
> index c33b68d..63e450b 100644
> --- a/xen/arch/arm/arm32/livepatch.c
> +++ b/xen/arch/arm/arm32/livepatch.c
> @@ -3,28 +3,276 @@
>   */
>  
>  #include <xen/errno.h>
> +#include <xen/kernel.h>
>  #include <xen/lib.h>
>  #include <xen/livepatch_elf.h>
>  #include <xen/livepatch.h>
>  
> +#include <asm/page.h>
> +#include <asm/livepatch.h>
> +
>  void arch_livepatch_apply_jmp(struct livepatch_func *func)
>  {
> +    uint32_t insn;
> +    uint32_t *old_ptr;

This is now removed.
> +    uint32_t *new_ptr;
> +
> +    BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> +    BUILD_BUG_ON(PATCH_INSN_SIZE != sizeof(insn));
> +
> +    ASSERT(vmap_of_xen_text);
> +
> +    /* Save old one. */
> +    old_ptr = func->old_addr;
> +    memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);

Which makes this smaller.
> +
> +    if ( func->new_addr )
> +    {
> +        s32 delta;
> +
> +        /*
> +         * The -4 is to account for the b <offset> instruction placed at
> +         * the start of the func->old_addr.
> +         */
> +        delta = (s32)(func->new_addr - func->old_addr - 4);

And I made this a bit simpler:

	delta = (s32)func->new_addr - (s32)func->old_addr - PATCH_INSN_SIZE;

Along with a comment refering to the ARM DDI 0406C.c  A8.8.18

Anyhow, when I posted this patch I was excited that everything "worked". 

But a more dilligient test showed that in fact the SP is being corrupted.

That is if I call 'xl info' before patching (with this inline patch):

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index d0edb13..793e219 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -240,6 +240,7 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         xen_extraversion_t extraversion;
 
         memset(extraversion, 0, sizeof(extraversion));
+        printk("%s: %p %p\n", &extraversion, xen_extra_version());
         safe_strcpy(extraversion, deny ? xen_deny() : xen_extra_version());
         if ( copy_to_guest(arg, extraversion, ARRAY_SIZE(extraversion)) )
             return -EFAULT;

I get:
(XEN) do_xen_version: dst=43fd7ad8 src=0028b020

OK, good.

With the hypervisor being patched I get:

(XEN) do_xen_version: dst=ffffffe8 src=00805038

The src is corrected - it points to the payload .rodata section.

But the SP is all messed up! And that ends in tears with the hypervisor:
 Assertion 'diff < STACK_SIZE' failed at traps.c:864


Decoding the instructions that are being called (the new xen_extra_version())
yields:

   0:   e52db004        push	{fp}
   4:   e28db000        add     fp, sp, #0
   8:   e3050038        movw    r0, #20536      ; 0x5038
   c:   e3400080        movt    r0, #128        ; 0x80
  10:   e24bd000        sub     sp, fp, #0
  14:   e12fff1e        bx      lr

(This is after the relocation has been done).
And the unconditional branch that is put in the old xen_extra_version
is: 

 0:   ea1710d0        b       0x5c4348

	Which is correct too - to check for correctness I added two brkp.
	One (71 00 20 e1) at the start of the new 'xen_extra_version'.
	And then another four bytes in front of it (72 02 20 e1). The signature
	of them is different and the exception we hit was the first Prefetch Abort
        (71 00 20 e1).

Anyhow something is amiss here. I am not sure if there are some hidden
semantics in regard to a B condition.

I am going to try to patch in the old xen_extra_version an BL instruction
followed by BX LR and see if that makes a difference.

But in the meantime please ignore this patch. It needs some more
work.

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