Re: [PATCH v1 6/9] livepatch: Initial ARM64 support.

[Julien Grall <julien.grall@arm.com>]

Hi Konrad,

On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:

[...]

> diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
> new file mode 100644
> index 0000000..e348942
> --- /dev/null
> +++ b/xen/arch/arm/arm64/livepatch.c
> @@ -0,0 +1,247 @@
> +/*
> + *  Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + */
> +
> +#include "../livepatch.h"
> +#include <xen/bitops.h>
> +#include <xen/errno.h>
> +#include <xen/lib.h>
> +#include <xen/livepatch_elf.h>
> +#include <xen/livepatch.h>
> +#include <xen/mm.h>
> +#include <xen/vmap.h>
> +
> +#include <asm/bitops.h>
> +#include <asm/byteorder.h>
> +#include <asm/insn.h>
> +
> +void arch_livepatch_apply_jmp(struct livepatch_func *func)
> +{
> +    uint32_t insn;
> +    uint32_t *old_ptr;
> +    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);
> +
> +    if ( func->new_size )
> +        insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
> +                                           (unsigned long)func->new_addr,
> +                                           AARCH64_INSN_BRANCH_NOLINK);

The function aarch64_insn_gen_branch_imm will fail to create the branch 
if the difference between old_addr and new_addr is greater than 128MB.

In this case, the function will return AARCH64_BREAK_FAULT which will 
crash Xen when the instruction is executed.

I cannot find any sanity check in the hypervisor code. So are we 
trusting the payload?

> +    else
> +        insn = aarch64_insn_gen_nop();
> +
> +    new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text;
> +
> +    /* PATCH! */
> +    *(new_ptr) = cpu_to_le32(insn);
> +
> +    clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr));
> +}
> +
> +void arch_livepatch_revert_jmp(const struct livepatch_func *func)
> +{
> +    uint32_t *new_ptr;
> +    uint32_t insn;
> +
> +    memcpy(&insn, func->opaque, PATCH_INSN_SIZE);
> +
> +    new_ptr = (uint32_t *)func->old_addr - (u32 *)_start + vmap_of_xen_text;
> +
> +    /* PATCH! */
> +    *(new_ptr) = cpu_to_le32(insn);
> +
> +    clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr));
> +}
> +
> +int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
> +{
> +    const Elf_Ehdr *hdr = elf->hdr;
> +
> +    if ( hdr->e_machine != EM_AARCH64 ||
> +         hdr->e_ident[EI_CLASS] != ELFCLASS64 )
> +    {
> +        dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF Machine type!\n",
> +                elf->name);
> +        return -EOPNOTSUPP;
> +    }
> +
> +    return 0;
> +}
> +

[...]

> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 20bb2ba..607ee59 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -13,6 +13,7 @@
>  #include <xen/hypercall.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
> +#include <xen/livepatch.h>
>  #include <xen/sched.h>
>  #include <xen/softirq.h>
>  #include <xen/wait.h>
> @@ -55,6 +56,11 @@ void idle_loop(void)
>
>          do_tasklet();
>          do_softirq();
> +        /*
> +         * We MUST be last (or before dsb, wfi). Otherwise after we get the
> +         * softirq we would execute dsb,wfi (and sleep) and not patch.
> +         */
> +        check_for_livepatch_work();
>      }
>  }
>
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index 3093554..19f6033 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -1,56 +1,93 @@
>  /*
>   *  Copyright (C) 2016 Citrix Systems R&D Ltd.
>   */
> +

Spurious change?

> +#include "livepatch.h"
>  #include <xen/errno.h>
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/livepatch_elf.h>
>  #include <xen/livepatch.h>
> +#include <xen/vmap.h>
> +
> +#include <asm/mm.h>
>
> -/* On ARM32,64 instructions are always 4 bytes long. */
> -#define PATCH_INSN_SIZE 4

Rather than moving again PATCH_INSN_SIZE in this patch. Can you directly 
move it in patch [1]?

> +void *vmap_of_xen_text;
>
>  int arch_verify_insn_length(unsigned long len)
>  {
>      return len != PATCH_INSN_SIZE;
>  }
>
> -void arch_livepatch_quiesce(void)
> +int arch_livepatch_quiesce(void)

It would have been nice to move the prototype change out of this patch 
to keep it "straight forward".

>  {
> +    mfn_t text_mfn;
> +    unsigned int text_order;
> +
> +    if ( vmap_of_xen_text )
> +        return -EINVAL;
> +
> +    text_mfn = _mfn(virt_to_mfn(_stext));
> +    text_order = get_order_from_bytes(_end - _start);

It is a bit odd that you use _stext before and the _start. But I won't 
complain as I did the same in alternatives.c :/.

However, I think it should be enough to remap _stext -> _etext. I had to 
map all the Xen binary for the alternatives because we may patch the 
init text.

I forgot to mention it in the code, so I will send a patch to update it.

> +
> +    /*
> +     * The text section is read-only. So re-map Xen to be able to patch
> +     * the code.
> +     */
> +    vmap_of_xen_text = __vmap(&text_mfn, 1 << text_order, 1, 1, PAGE_HYPERVISOR,
> +                              VMAP_DEFAULT);
> +
> +    if ( !vmap_of_xen_text )
> +    {
> +        printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor! (order=%u)\n",
> +               text_order);
> +        return -ENOMEM;
> +    }
> +    return 0;
>  }
>
>  void arch_livepatch_revive(void)
>  {
> +    /*
> +     * Nuke the instruction cache. It has been cleaned before in

I guess you want to replace "It" by "Data cache" otherwise it does not 
make much sense.

> +     * arch_livepatch_apply_jmp.

What about the payload? It may contain instructions, so we need to 
ensure that all the data reached the memory.

> +     */
> +    invalidate_icache();
> +
> +    if ( vmap_of_xen_text )
> +        vunmap(vmap_of_xen_text);
> +
> +    vmap_of_xen_text = NULL;
> +
> +    /*
> +     * Need to flush the branch predicator for ARMv7 as it may be

s/predicator/predictor/

> +     * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b).
> +     */
> +    flush_xen_text_tlb_local();

I am a bit confused. In your comment you mention the branch but flush 
the TLBs. The two are not related.

However, I would prefer the branch predictor to be flushed directly in 
invalidate_icache by calling BPIALLIS. This is because flushing the 
cache means that you likely want to flush the branch predictor too.

>  }
>
>  int arch_livepatch_verify_func(const struct livepatch_func *func)
>  {
> -    return -ENOSYS;
> -}
> -
> -void arch_livepatch_apply_jmp(struct livepatch_func *func)
> -{
> -}
> +    if ( func->old_size < PATCH_INSN_SIZE )
> +        return -EINVAL;
>
> -void arch_livepatch_revert_jmp(const struct livepatch_func *func)
> -{
> +    return 0;
>  }
>
>  void arch_livepatch_post_action(void)
>  {
> +    /* arch_livepatch_revive has nuked the instruction cache. */
>  }
>
>  void arch_livepatch_mask(void)
>  {
> +    /* Mask System Error (SError) */
> +    local_abort_disable();
>  }
>
>  void arch_livepatch_unmask(void)
>  {
> -}
> -
> -int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
> -{
> -    return -ENOSYS;
> +    local_abort_enable();
>  }
>
>  int arch_livepatch_perform_rel(struct livepatch_elf *elf,
> @@ -60,20 +97,34 @@ int arch_livepatch_perform_rel(struct livepatch_elf *elf,
>      return -ENOSYS;
>  }
>
> -int arch_livepatch_perform_rela(struct livepatch_elf *elf,
> -                                const struct livepatch_elf_sec *base,
> -                                const struct livepatch_elf_sec *rela)
> -{
> -    return -ENOSYS;
> -}
> -
>  int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type)
>  {
> -    return -ENOSYS;
> +    unsigned long start = (unsigned long)va;
> +    unsigned int flags;
> +
> +    ASSERT(va);
> +    ASSERT(pages);
> +
> +    if ( type == LIVEPATCH_VA_RX )
> +        flags = PTE_RO; /* R set, NX clear */
> +    else if ( type == LIVEPATCH_VA_RW )
> +        flags = PTE_NX; /* R clear, NX set */
> +    else
> +        flags = PTE_NX | PTE_RO; /* R set, NX set */

va_type is an enum. So I would prefer to see a switch. So we can catch 
more easily any addition of a new member.

> +
> +    modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
> +
> +    return 0;
>  }
>
>  void __init arch_livepatch_init(void)
>  {
> +    void *start, *end;
> +
> +    start = (void *)LIVEPATCH_VMAP;
> +    end = start + MB(2);

Could you define the in config.h? So in the future we can rework more 
easily the memory layout.

> +
> +    vm_init_type(VMAP_XEN, start, end);
>  }
>
>  /*
> diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h
> new file mode 100644
> index 0000000..8c8d625
> --- /dev/null
> +++ b/xen/arch/arm/livepatch.h

I am not sure why this header is living in arch/arm/ and not 
include/asm-arm/

> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
> + *
> + */
> +
> +#ifndef __XEN_ARM_LIVEPATCH_H__
> +#define __XEN_ARM_LIVEPATCH_H__
> +
> +/* On ARM32,64 instructions are always 4 bytes long. */
> +#define PATCH_INSN_SIZE 4
> +
> +/*
> + * The va of the hypervisor .text region. We need this as the
> + * normal va are write protected.
> + */
> +extern void *vmap_of_xen_text;
> +
> +#endif /* __XEN_ARM_LIVEPATCH_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
> index 06c67bc..e3f3f37 100644
> --- a/xen/arch/x86/livepatch.c
> +++ b/xen/arch/x86/livepatch.c
> @@ -15,10 +15,12 @@
>
>  #define PATCH_INSN_SIZE 5
>
> -void arch_livepatch_quiesce(void)
> +int arch_livepatch_quiesce(void)
>  {
>      /* Disable WP to allow changes to read-only pages. */
>      write_cr0(read_cr0() & ~X86_CR0_WP);
> +
> +    return 0;
>  }
>
>  void arch_livepatch_revive(void)
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 51afa24..2fc76b6 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -222,7 +222,7 @@ endmenu
>  config LIVEPATCH
>  	bool "Live patching support (TECH PREVIEW)"
>  	default n
> -	depends on X86 && HAS_BUILD_ID = "y"
> +	depends on (X86 || ARM_64) && HAS_BUILD_ID = "y"
>  	---help---
>  	  Allows a running Xen hypervisor to be dynamically patched using
>  	  binary patches without rebooting. This is primarily used to binarily
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 9c45270..af9443d 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload,
>                                    sizeof(*region->frame[i].bugs);
>      }
>
> -#ifndef CONFIG_ARM
> +#ifndef CONFIG_ARM_32

I would prefer if you use CONFIG_ALTERNATIVE rather than CONFIG_ARM_32.

>      sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
>      if ( sec )
>      {
> @@ -711,9 +711,15 @@ static int prepare_payload(struct payload *payload,
>                  return -EINVAL;
>              }
>          }
> +#ifndef CONFIG_ARM
>          apply_alternatives_nocheck(start, end);
> +#else
> +        apply_alternatives(start, sec->sec->sh_size);
> +#endif
>      }
> +#endif
>
> +#ifndef CONFIG_ARM
>      sec = livepatch_elf_sec_by_name(elf, ".ex_table");
>      if ( sec )
>      {

Any reason to not move .ex_table in an x86 specific file? Or maybe we 
should define a new config option HAVE_ARCH_EX_TABLE to avoid 
architecture specific check in the common code.

> @@ -1083,12 +1089,17 @@ static int livepatch_list(xen_sysctl_livepatch_list_t *list)
>  static int apply_payload(struct payload *data)
>  {
>      unsigned int i;
> +    int rc;
>
>      printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
>              data->name, data->nfuncs);
>
> -    arch_livepatch_quiesce();
> -
> +    rc = arch_livepatch_quiesce();
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
> +        return rc;
> +    }
>      /*
>       * Since we are running with IRQs disabled and the hooks may call common
>       * code - which expects the spinlocks to run with IRQs enabled - we temporarly
> @@ -1119,10 +1130,16 @@ static int apply_payload(struct payload *data)
>  static int revert_payload(struct payload *data)
>  {
>      unsigned int i;
> +    int rc;
>
>      printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name);
>
> -    arch_livepatch_quiesce();
> +    rc = arch_livepatch_quiesce();
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
> +        return rc;
> +    }
>
>      for ( i = 0; i < data->nfuncs; i++ )
>          arch_livepatch_revert_jmp(&data->funcs[i]);
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index a96f845..8d876f6 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -80,9 +80,10 @@
>   *   4M -   6M   Fixmap: special-purpose 4K mapping slots
>   *   6M -   8M   Early boot mapping of FDT
>   *   8M -  10M   Early relocation address (used when relocating Xen)
> + *               and later for livepatch vmap (if compiled in)
>   *
>   * ARM32 layout:
> - *   0  -   8M   <COMMON>
> + *   0  -  10M   <COMMON>

May I ask to have this change and ...

>   *
>   *  32M - 128M   Frametable: 24 bytes per page for 16GB of RAM
>   * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
> @@ -93,7 +94,7 @@
>   *
>   * ARM64 layout:
>   * 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
> - *   0  -   8M   <COMMON>
> + *   0  -  10M   <COMMON>

this change in a separate patch to keep this patch livepatch only?

>   *
>   *   1G -   2G   VMAP: ioremap and early_ioremap
>   *
> @@ -113,6 +114,9 @@
>  #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
>  #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
>  #define BOOT_RELOC_VIRT_START  _AT(vaddr_t,0x00800000)
> +#ifdef CONFIG_LIVEPATCH
> +#define LIVEPATCH_VMAP         _AT(vaddr_t,0x00800000)
> +#endif
>
>  #define HYPERVISOR_VIRT_START  XEN_VIRT_START
>
> diff --git a/xen/include/asm-arm/current.h b/xen/include/asm-arm/current.h
> index 65c0cdf..f4fcfd6 100644
> --- a/xen/include/asm-arm/current.h
> +++ b/xen/include/asm-arm/current.h
> @@ -33,8 +33,15 @@ static inline struct cpu_info *get_cpu_info(void)
>
>  #define guest_cpu_user_regs() (&get_cpu_info()->guest_cpu_user_regs)
>
> +#ifdef CONFIG_LIVEPATCH
> +#define switch_stack_and_jump(stack, fn)                                \
> +    asm volatile ("mov sp,%0;"                                          \
> +                  "bl check_for_livepatch_work;"                        \
> +                  "b " STR(fn) : : "r" (stack) : "memory" )
> +#else
>  #define switch_stack_and_jump(stack, fn)                                \
>      asm volatile ("mov sp,%0; b " STR(fn) : : "r" (stack) : "memory" )
> +#endif

I have commented on the previous version about switch_stack_and_jump a 
while after you send this series. So I will spare you new comments here :).

>  #define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn)
>

[...]

> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
> index 2e64686..6f30c0d 100644
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -72,7 +72,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func);
>   * These functions are called around the critical region patching live code,
>   * for an architecture to take make appropratie global state adjustments.
>   */
> -void arch_livepatch_quiesce(void);
> +int arch_livepatch_quiesce(void);
>  void arch_livepatch_revive(void);
>
>  void arch_livepatch_apply_jmp(struct livepatch_func *func);
>

[1] 
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01832.html

-- 
Julien Grall

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