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

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

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

Ugh. This is a thorny one. I concur we need to check this - and better of
do it when we load the payload to make sure the distance is correct.

And that should also be on x86, so will spin of a seperate patch.

And in here add an ASSERT that the insn != AARCH64_BREAK_FAULT


.. snip..
> > -/* 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]?

Sure.
> 
> > +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".

OK.
> 
> >  {
> > +    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 :/.

Heheh.
> 
> 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 agree.
> 
> I forgot to mention it in the code, so I will send a patch to update it.

OK.
> 
> > +
> > +    /*
> > +     * 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.

OK.
> 
> >  }
> > 
> >  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.

LIVEPATCH_VMAP_START and LIVEPATCH_VMAP_END ?

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

That is not exposed on x86 thought.

I can expose HAVE_ALTERNATIVE on x86 and use that instead.

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

Would need to create an arch specific hook for this.

> define a new config option HAVE_ARCH_EX_TABLE to avoid architecture specific
> check in the common code.

That could do it too.
> 
> > @@ -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?

Of course.

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