| Message ID | 1470716340-24474-4-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Konrad, On 09/08/2016 06:19, Konrad Rzeszutek Wilk wrote: > The initial support for ARM64 - and livepatching > works: As it is a very RFC I only gave a quick look. I have few comments on it (see below). > > (XEN) livepatch: xen_hello_world: Applying 1 functions > (XEN) hi_func: Hi! (called 1 times) > (XEN) Hook executing. > (XEN) livepatch: xen_hello_world finished APPLY with rc=0 > (XEN) livepatch.c:1687: livepatch: load_payload_fnc: rc=0 (p->rc=0) > (XEN) livepatch: Hello World > (XEN) 'x' pressed - Dumping all livepatch patches > (XEN) build-id: e144bafc4ee8635eee5bed8e3988b484765c46c8 > (XEN) name=xen_hello_world state=APPLIED(2) 0000000000318000 (.data=0000000000319000, .rodata=000000000031a000) using 3 pages. > (XEN) xen_extra_version patch 0000000000233fac(12) with 0000000000318000 (16) > (XEN) build-id=c4b842c276be43adbe4db788598b1e11bce04dc6 > (XEN) depend-on=9affa110481e8e13606c61b21e5f6a357a3c8ef9 > > ARM32 still has some issues. > > The are some TODOs left to be done: > > General: > - Bubble ALT_ORIG_PTR macro for both x86/ARM. > - Unify the ELF RELA checks - they are the same on x86/ARM[32,64]. > - Makefile generating .livepatch.depends needs to ingest the > -O argument based on architecture > - Test cases should move to common/ ? [Needs Jan's Ack] In general, I would be in favor of sharing as much as possible the code. > ARM32 issues: > - vm_init_type: Assertion 'is_xen_heap_mfn(ma >> PAGE_SHIFT)' failed at xen/include/asm/mm.h:23 xen/include/asm/mm.h:23 points to the beginning of struct page_info. Did you mean to write instead 233? This would point to maddr_virt. This would mean someone is trying to get the virtual address of MFN that is not in the xenheap (only the xenheap is mapped on ARM32). Do you have the full call stack? [...] > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> [...] > diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c > index aba1320..67749ed 100644 > --- a/xen/arch/arm/livepatch.c > +++ b/xen/arch/arm/livepatch.c > @@ -6,66 +6,100 @@ > #include <xen/lib.h> > #include <xen/livepatch_elf.h> > #include <xen/livepatch.h> > +#include <xen/vmap.h> > +#include "livepatch.h" > + > +#include <asm/mm.h> > + > +void *vmap_of_xen_text; > > void arch_livepatch_quiesce(void) > { > + mfn_t text_mfn; > + unsigned int text_order; > + > + if ( vmap_of_xen_text ) > + return; > + > + text_mfn = _mfn(virt_to_mfn(_stext)); > + text_order = get_order_from_bytes(_end - _start); > + > + /* > + * 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); Shouldn't we return an error if we fail to re-map the region? Otherwise the patch may silently be ignored (see arch_livepatch_apply_jmp). > } > > void arch_livepatch_revive(void) > { > + /* Nuke the instruction cache */ > + invalidate_icache(); I was about to say that this is not enough. But you called clean_and_invalidate_* every time you rewrite the jump. It might be worth to add a comment here, explain that the cache has been cleaned before hand. However, I can't find any data cache flush for the payload. Did I miss anything? Lastly, before I forgot, you will need to invalidate the branch predictor for ARMv7 as it may be architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b). > + > + if ( vmap_of_xen_text ) > + vunmap(vmap_of_xen_text); > + > + vmap_of_xen_text = NULL; > } > > int arch_livepatch_verify_func(const struct livepatch_func *func) > { > - return -ENOSYS; > -} > + /* No NOP patching yet. */ > + if ( !func->new_size ) > + return -EOPNOTSUPP; > > -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) > { > + /* TODO: No NMI on ARM? */ All interrupts can be masked on ARM so far. Although, local_irq_disable will only mask IRQ (i.e interrupt from the interrupt controller). We may want to mask SError (System Error) via local_abort_{disable,enable} to avoid receiving asynchronous abort whilst patching Xen. The interrupt will stay pending until it will be re-enabled in arch_livepatch_unmask. > } > > void arch_livepatch_unmask(void) > { > + /* TODO: No NMI on ARM? */ > } > > -int arch_livepatch_verify_elf(const struct livepatch_elf *elf) > +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; > > -int arch_livepatch_perform_rel(struct livepatch_elf *elf, > - const struct livepatch_elf_sec *base, > - const struct livepatch_elf_sec *rela) > -{ > - return -ENOSYS; > -} > + ASSERT(va); > + ASSERT(pages); > > -int arch_livepatch_perform_rela(struct livepatch_elf *elf, > - const struct livepatch_elf_sec *base, > - const struct livepatch_elf_sec *rela) > -{ > - return -ENOSYS; > -} > + if ( type == LIVEPATCH_VA_RX ) > + flags = 0x2; /* R set,NX clear */ > + else if ( type == LIVEPATCH_VA_RW ) > + flags = 0x1; /* R clear, NX set */ > + else > + flags = 0x3; /* R set,NX set */ > > -int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type) > -{ > - return -ENOSYS; > + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags); > + > + return 0; > } > > void __init arch_livepatch_init(void) > { > + void *start, *end; > + > + start = (void *)xen_virt_end; > + end = (void *)FIXMAP_ADDR(0); The space between xen_virt_end and FIXMAP_ADDR may be really small depending on the size of the Xen binary. I would introduce a specific region in the layout of few megabytes (not sure how much me need). Or re-use "early relocation address" (8M - 10M) as the region is only used during early boot. > + > + BUG_ON(start >= end); > + > + vm_init_type(VMAP_XEN, start, end); > } > > /* > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index eca7cdd..94b4911 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -143,6 +143,8 @@ vaddr_t xenheap_virt_end __read_mostly; > vaddr_t xenheap_virt_start __read_mostly; > #endif > > +vaddr_t xen_virt_end; > + > unsigned long frametable_base_pdx __read_mostly; > unsigned long frametable_virt_end __read_mostly; > > @@ -540,6 +542,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) > /* No flush required here as page table is not hooked in yet. */ > } > > + if ( i < LPAE_ENTRIES ) Why this check? > + xen_virt_end = XEN_VIRT_START + (i << PAGE_SHIFT); > + > pte = pte_of_xenaddr((vaddr_t)xen_xenmap); > pte.pt.table = 1; > write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte); > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c > index 88a79d8..6ffd2d0 100644 > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -618,7 +618,6 @@ static int prepare_payload(struct payload *payload, > sizeof(*region->frame[i].bugs); > } > > -#ifndef CONFIG_ARM > sec = livepatch_elf_sec_by_name(elf, ".altinstructions"); > if ( sec ) > { > @@ -636,8 +635,14 @@ static int prepare_payload(struct payload *payload, > > for ( a = start; a < end; a++ ) > { > +#ifndef CONFIG_ARM > + /* TODO: Bubble ALT_ORIG_PTR up. */ > const void *instr = &a->instr_offset + a->instr_offset; > const void *replacement = &a->repl_offset + a->repl_offset; > +#else > + const void *instr = &a->orig_offset + a->orig_offset; > + const void *replacement = &a->alt_offset + a->alt_offset; > +#endif > > if ( (instr < region->start && instr >= region->end) || > (replacement < region->start && replacement >= region->end) ) > @@ -647,9 +652,14 @@ 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 > } > > +#ifndef CONFIG_ARM > sec = livepatch_elf_sec_by_name(elf, ".ex_table"); > if ( sec ) > { > 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;" \ May I ask why check_for_livepatch_work is called in switch_stack_and_jump? > + "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 > > #define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn) Regards,
On Fri, Aug 12, 2016 at 05:00:47PM +0200, Julien Grall wrote: > Hi Konrad, > > On 09/08/2016 06:19, Konrad Rzeszutek Wilk wrote: > > The initial support for ARM64 - and livepatching > > works: > > As it is a very RFC I only gave a quick look. I have few comments on it (see > below). > > > > > (XEN) livepatch: xen_hello_world: Applying 1 functions > > (XEN) hi_func: Hi! (called 1 times) > > (XEN) Hook executing. > > (XEN) livepatch: xen_hello_world finished APPLY with rc=0 > > (XEN) livepatch.c:1687: livepatch: load_payload_fnc: rc=0 (p->rc=0) > > (XEN) livepatch: Hello World > > (XEN) 'x' pressed - Dumping all livepatch patches > > (XEN) build-id: e144bafc4ee8635eee5bed8e3988b484765c46c8 > > (XEN) name=xen_hello_world state=APPLIED(2) 0000000000318000 (.data=0000000000319000, .rodata=000000000031a000) using 3 pages. > > (XEN) xen_extra_version patch 0000000000233fac(12) with 0000000000318000 (16) > > (XEN) build-id=c4b842c276be43adbe4db788598b1e11bce04dc6 > > (XEN) depend-on=9affa110481e8e13606c61b21e5f6a357a3c8ef9 > > > > ARM32 still has some issues. > > > > The are some TODOs left to be done: > > > > General: > > - Bubble ALT_ORIG_PTR macro for both x86/ARM. > > - Unify the ELF RELA checks - they are the same on x86/ARM[32,64]. > > - Makefile generating .livepatch.depends needs to ingest the > > -O argument based on architecture > > - Test cases should move to common/ ? [Needs Jan's Ack] > > In general, I would be in favor of sharing as much as possible the code. > > > ARM32 issues: > > - vm_init_type: Assertion 'is_xen_heap_mfn(ma >> PAGE_SHIFT)' failed at xen/include/asm/mm.h:23 > > xen/include/asm/mm.h:23 points to the beginning of struct page_info. Did you > mean to write instead 233? Probably. > > This would point to maddr_virt. This would mean someone is trying to get the > virtual address of MFN that is not in the xenheap (only the xenheap is > mapped on ARM32). Do you have the full call stack? No :-( I need to rebuild it and put it on the board. Maybe in a week? > > [...] > > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > [...] > > > diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c > > index aba1320..67749ed 100644 > > --- a/xen/arch/arm/livepatch.c > > +++ b/xen/arch/arm/livepatch.c > > @@ -6,66 +6,100 @@ > > #include <xen/lib.h> > > #include <xen/livepatch_elf.h> > > #include <xen/livepatch.h> > > +#include <xen/vmap.h> > > +#include "livepatch.h" > > + > > +#include <asm/mm.h> > > + > > +void *vmap_of_xen_text; > > > > void arch_livepatch_quiesce(void) > > { > > + mfn_t text_mfn; > > + unsigned int text_order; > > + > > + if ( vmap_of_xen_text ) > > + return; > > + > > + text_mfn = _mfn(virt_to_mfn(_stext)); > > + text_order = get_order_from_bytes(_end - _start); > > + > > + /* > > + * 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); > > Shouldn't we return an error if we fail to re-map the region? > > Otherwise the patch may silently be ignored (see arch_livepatch_apply_jmp). We do actually bail out of patching if vmap_of_xen_text is NULL. But let me add a return (-ENOMEM) to this so that we don't attempt to do any patching and print a nice message. > > > } > > > > void arch_livepatch_revive(void) > > { > > + /* Nuke the instruction cache */ > > + invalidate_icache(); > > I was about to say that this is not enough. But you called > clean_and_invalidate_* every time you rewrite the jump. It might be worth to > add a comment here, explain that the cache has been cleaned before hand. > > However, I can't find any data cache flush for the payload. Did I miss > anything? No just didn't occur to me - as we patch the .text not the .data. I will take a look at the existing code and see if I can find out how to do the data cache flush. > > Lastly, before I forgot, you will need to invalidate the branch predictor > for ARMv7 as it may be architecturally visible to the software (see B2.2.4 > in ARM DDI 0406C.b). OK. > > > + > > + if ( vmap_of_xen_text ) > > + vunmap(vmap_of_xen_text); > > + > > + vmap_of_xen_text = NULL; > > } > > > > int arch_livepatch_verify_func(const struct livepatch_func *func) > > { > > - return -ENOSYS; > > -} > > + /* No NOP patching yet. */ > > + if ( !func->new_size ) > > + return -EOPNOTSUPP; > > > > -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) > > { > > + /* TODO: No NMI on ARM? */ > > All interrupts can be masked on ARM so far. Although, local_irq_disable will > only mask IRQ (i.e interrupt from the interrupt controller). > > We may want to mask SError (System Error) via local_abort_{disable,enable} > to avoid receiving asynchronous abort whilst patching Xen. The interrupt > will stay pending until it will be re-enabled in arch_livepatch_unmask. /me nods. Thanks! > > > } > > > > void arch_livepatch_unmask(void) > > { > > + /* TODO: No NMI on ARM? */ > > } > > > > -int arch_livepatch_verify_elf(const struct livepatch_elf *elf) > > +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; > > > > -int arch_livepatch_perform_rel(struct livepatch_elf *elf, > > - const struct livepatch_elf_sec *base, > > - const struct livepatch_elf_sec *rela) > > -{ > > - return -ENOSYS; > > -} > > + ASSERT(va); > > + ASSERT(pages); > > > > -int arch_livepatch_perform_rela(struct livepatch_elf *elf, > > - const struct livepatch_elf_sec *base, > > - const struct livepatch_elf_sec *rela) > > -{ > > - return -ENOSYS; > > -} > > + if ( type == LIVEPATCH_VA_RX ) > > + flags = 0x2; /* R set,NX clear */ > > + else if ( type == LIVEPATCH_VA_RW ) > > + flags = 0x1; /* R clear, NX set */ > > + else > > + flags = 0x3; /* R set,NX set */ > > > > -int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type) > > -{ > > - return -ENOSYS; > > + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags); > > + > > + return 0; > > } > > > > void __init arch_livepatch_init(void) > > { > > + void *start, *end; > > + > > + start = (void *)xen_virt_end; > > + end = (void *)FIXMAP_ADDR(0); > > The space between xen_virt_end and FIXMAP_ADDR may be really small depending > on the size of the Xen binary. > > I would introduce a specific region in the layout of few megabytes (not sure > how much me need). Or re-use "early relocation address" (8M - 10M) as the > region is only used during early boot. OK, will take a look. Thanks! > > > + > > + BUG_ON(start >= end); > > + > > + vm_init_type(VMAP_XEN, start, end); > > } > > > > /* > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > > index eca7cdd..94b4911 100644 > > --- a/xen/arch/arm/mm.c > > +++ b/xen/arch/arm/mm.c > > @@ -143,6 +143,8 @@ vaddr_t xenheap_virt_end __read_mostly; > > vaddr_t xenheap_virt_start __read_mostly; > > #endif > > > > +vaddr_t xen_virt_end; > > + > > unsigned long frametable_base_pdx __read_mostly; > > unsigned long frametable_virt_end __read_mostly; > > > > @@ -540,6 +542,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) > > /* No flush required here as page table is not hooked in yet. */ > > } > > > > + if ( i < LPAE_ENTRIES ) > > Why this check? /me scratches his head. That was for when I was also printing the entries. Let me remove it. > > > + xen_virt_end = XEN_VIRT_START + (i << PAGE_SHIFT); > > + > > pte = pte_of_xenaddr((vaddr_t)xen_xenmap); > > pte.pt.table = 1; > > write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte); > > > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c > > index 88a79d8..6ffd2d0 100644 > > --- a/xen/common/livepatch.c > > +++ b/xen/common/livepatch.c > > @@ -618,7 +618,6 @@ static int prepare_payload(struct payload *payload, > > sizeof(*region->frame[i].bugs); > > } > > > > -#ifndef CONFIG_ARM > > sec = livepatch_elf_sec_by_name(elf, ".altinstructions"); > > if ( sec ) > > { > > @@ -636,8 +635,14 @@ static int prepare_payload(struct payload *payload, > > > > for ( a = start; a < end; a++ ) > > { > > +#ifndef CONFIG_ARM > > + /* TODO: Bubble ALT_ORIG_PTR up. */ > > const void *instr = &a->instr_offset + a->instr_offset; > > const void *replacement = &a->repl_offset + a->repl_offset; > > +#else > > + const void *instr = &a->orig_offset + a->orig_offset; > > + const void *replacement = &a->alt_offset + a->alt_offset; > > +#endif > > > > if ( (instr < region->start && instr >= region->end) || > > (replacement < region->start && replacement >= region->end) ) > > @@ -647,9 +652,14 @@ 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 > > } > > > > +#ifndef CONFIG_ARM > > sec = livepatch_elf_sec_by_name(elf, ".ex_table"); > > if ( sec ) > > { > > 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;" \ > > May I ask why check_for_livepatch_work is called in switch_stack_and_jump? From 29f4ab0b0a4ff62589af35b7cbc2334e1d9acdcd: To perform and action on a payload, the hypercall sets up a data structure to schedule the work. A hook is added in the reset_stack_and_jump to check for work and execute it if needed (specifically we check an per-cpu flag to make this as quick as possible). In this way, patches can be applied with all CPUs idle and without stacks. The first CPU to run check_for_xsplice_work() becomes the master and triggers a reschedule softirq to trigger all the other CPUs to enter check_for_xsplice_work() with no stack. Once all CPUs have rendezvoused, all CPUs disable their IRQs and NMIs are ignored. The system is then quiscient and the master performs the action. After this, all CPUs enable IRQs and NMIs are re-enabled. > > > + "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 > > > > #define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn) > > Regards, > > -- > Julien Grall
Hi Konrad, On 12/08/2016 22:50, Konrad Rzeszutek Wilk wrote: > On Fri, Aug 12, 2016 at 05:00:47PM +0200, Julien Grall wrote: >>> 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;" \ >> >> May I ask why check_for_livepatch_work is called in switch_stack_and_jump? > > From 29f4ab0b0a4ff62589af35b7cbc2334e1d9acdcd: > To perform and action on a payload, the hypercall sets up a data > structure to schedule the work. A hook is added in the reset_stack_and_jump > to check for work and execute it if needed (specifically we check an > per-cpu flag to make this as quick as possible). > > In this way, patches can be applied with all CPUs idle and without > stacks. The first CPU to run check_for_xsplice_work() becomes the > master and triggers a reschedule softirq to trigger all the other CPUs > to enter check_for_xsplice_work() with no stack. Once all CPUs > have rendezvoused, all CPUs disable their IRQs and NMIs are ignored. > The system is then quiscient and the master performs the action. > After this, all CPUs enable IRQs and NMIs are re-enabled. I am a bit confused, switch_stack_and_jump will only be called on ARM when a vCPU is created. So why do you want to check livepatch at that time? After looking at the x86 version, I noticed that reset_stack_and_jump is called every time Xen returns to the guest, correct? If so, you may want to call check_for_livepatch_work in leave_hypervisor_tail instead. The latter will be call every time Xen returns to the guest. Regards,
On Mon, Aug 15, 2016 at 03:27:08PM +0200, Julien Grall wrote: > Hi Konrad, > > On 12/08/2016 22:50, Konrad Rzeszutek Wilk wrote: > > On Fri, Aug 12, 2016 at 05:00:47PM +0200, Julien Grall wrote: > > > > 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;" \ > > > > > > May I ask why check_for_livepatch_work is called in switch_stack_and_jump? > > > > From 29f4ab0b0a4ff62589af35b7cbc2334e1d9acdcd: > > To perform and action on a payload, the hypercall sets up a data > > structure to schedule the work. A hook is added in the reset_stack_and_jump > > to check for work and execute it if needed (specifically we check an > > per-cpu flag to make this as quick as possible). > > > > In this way, patches can be applied with all CPUs idle and without > > stacks. The first CPU to run check_for_xsplice_work() becomes the > > master and triggers a reschedule softirq to trigger all the other CPUs > > to enter check_for_xsplice_work() with no stack. Once all CPUs > > have rendezvoused, all CPUs disable their IRQs and NMIs are ignored. > > The system is then quiscient and the master performs the action. > > After this, all CPUs enable IRQs and NMIs are re-enabled. > > > I am a bit confused, switch_stack_and_jump will only be called on ARM when a > vCPU is created. So why do you want to check livepatch at that time? > > After looking at the x86 version, I noticed that reset_stack_and_jump is > called every time Xen returns to the guest, correct? Yes. I assumed ARM was the same. > > If so, you may want to call check_for_livepatch_work in > leave_hypervisor_tail instead. The latter will be call every time Xen > returns to the guest. Oooh. Thanks! > > Regards, > > -- > Julien Grall
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 0a96713..95cd8af 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -66,6 +66,16 @@ endif .PHONY: tests tests: + $(MAKE) -f $(BASEDIR)/Rules.mk -C test livepatch + +ifdef CONFIG_LIVEPATCH +all_symbols = --all-symbols +ifdef CONFIG_FAST_SYMBOL_LOOKUP +all_symbols = --all-symbols --sort-by-name +endif +else +all_symbols = +endif $(TARGET).axf: $(TARGET)-syms # XXX: VE model loads by VMA so instead of @@ -93,12 +103,12 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \ $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0 $(NM) -pa --format=sysv $(@D)/.$(@F).0 \ - | $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).0.S + | $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).0.S $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0.o $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \ $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1 $(NM) -pa --format=sysv $(@D)/.$(@F).1 \ - | $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1.S + | $(BASEDIR)/tools/symbols $(all_symbols) --sysv --sort >$(@D)/.$(@F).1.S $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o $(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \ $(@D)/.$(@F).1.o -o $@ diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile index b20db64..5966de0 100644 --- a/xen/arch/arm/arm32/Makefile +++ b/xen/arch/arm/arm32/Makefile @@ -4,8 +4,8 @@ obj-$(EARLY_PRINTK) += debug.o obj-y += domctl.o obj-y += domain.o obj-y += entry.o +obj-$(CONFIG_LIVEPATCH) += livepatch.o obj-y += proc-v7.o proc-caxx.o obj-y += smpboot.o obj-y += traps.o obj-y += vfp.o - diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c new file mode 100644 index 0000000..14a4e12 --- /dev/null +++ b/xen/arch/arm/arm32/livepatch.c @@ -0,0 +1,150 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + */ + +#include <xen/lib.h> +#include <xen/errno.h> +#include <xen/livepatch_elf.h> +#include <xen/livepatch.h> + +void arch_livepatch_apply_jmp(struct livepatch_func *func) +{ +} + +void arch_livepatch_revert_jmp(const struct livepatch_func *func) +{ +} + +int arch_livepatch_verify_elf(const struct livepatch_elf *elf) +{ + const Elf_Ehdr *hdr = elf->hdr; + + if ( hdr->e_machine != EM_ARM || + hdr->e_ident[EI_CLASS] != ELFCLASS32 ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF Machine type!\n", + elf->name); + return -EOPNOTSUPP; + } + + if ( (hdr->e_flags & EF_ARM_EABI_MASK) != EF_ARM_EABI_VER5 ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Unsupported ELF EABI(%x)!\n", + elf->name, hdr->e_flags); + return -EOPNOTSUPP; + } + + return 0; +} + +int arch_livepatch_perform_rel(struct livepatch_elf *elf, + const struct livepatch_elf_sec *base, + const struct livepatch_elf_sec *rela) +{ + return -ENOSYS; +} + +int arch_livepatch_perform_rela(struct livepatch_elf *elf, + const struct livepatch_elf_sec *base, + const struct livepatch_elf_sec *rela) +{ + const Elf_RelA *r; + unsigned int symndx, i; + uint32_t val; + void *dest; + + + if ( !rela->sec->sh_size ) + return 0; + + if ( rela->sec->sh_entsize < sizeof(Elf_RelA) || + rela->sec->sh_size % rela->sec->sh_entsize ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Section relative header is corrupted!\n", + elf->name); + return -EINVAL; + } + + for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ ) + { + s32 offset; + + symndx = ELF32_R_SYM(r->r_info); + if ( symndx > elf->nsym ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative symbol wants symbol@%u which is past end!\n", + elf->name, symndx); + return -EINVAL; + } + + dest = base->load_addr + r->r_offset; /* P */ + val = elf->sym[symndx].sym->st_value; /* S */ + + /* r->r_addend is computed below. */ + switch ( ELF32_R_TYPE(r->r_info) ) { + case R_ARM_NONE: + /* ignore */ + break; + + case R_ARM_MOVW_ABS_NC: + /* MOVW loads 16 bits into the bottom half of a register */ + /* ResultMask(X) = X & 0xFFFF */ + case R_ARM_MOVT_ABS: + /* MOVT loads 16 bits into the top half of a register.*/ + /* ResultMask(X)= X & 0xFFFF0000 */ + if ( ELF32_R_TYPE(r->r_info) == R_ARM_MOVT_ABS ) + val &= 0xFFFF0000; + else + val &= 0xFFFF; + /* + * insn[19:16] = Result_Mask(X) >> 12 + * insn[11:0] = Result_Mask(X) & 0xFFF + */ + *(u32 *)dest |= val & 0xFFF; + *(u32 *)dest |= (val >> 12) << 16; + break; + + case R_ARM_ABS32: /* (S + A) | T */ + *(u32 *)dest = val + r->r_addend; + break; + + case R_ARM_CALL: + case R_ARM_JUMP24: + offset = *(u32 *)dest; + /* addend = sign_extend (insn[23:0] << 2) */ + offset = (offset & 0x00ffffff) << 2; + /* (S + A) - P */ + offset += val - (unsigned long)dest; + /* X & 0x03FFFFFE */ + offset &= 0x03FFFFFE; + *(u32 *)dest = offset; + /* TODO: Check overflow. */ + if ( 0 ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n", + elf->name, i, rela->name, base->name); + return -EOVERFLOW; + } + break; + case R_ARM_REL32: /* ((S + A) | T) – P */ + *(u32 *)dest = *(u32 *)dest + val - (unsigned long)dest; + break; + + default: + dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation #%x\n", + elf->name, ELF32_R_TYPE(r->r_info)); + return -EOPNOTSUPP; + } + } + return 0; +} + +/* + * 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/arm/arm64/Makefile b/xen/arch/arm/arm64/Makefile index c1fa43f..149b6b3 100644 --- a/xen/arch/arm/arm64/Makefile +++ b/xen/arch/arm/arm64/Makefile @@ -6,6 +6,7 @@ obj-y += domctl.o obj-y += domain.o obj-y += entry.o obj-y += insn.o +obj-$(CONFIG_LIVEPATCH) += livepatch.o obj-y += smpboot.o obj-y += traps.o obj-y += vfp.o diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c new file mode 100644 index 0000000..cc2e0a1 --- /dev/null +++ b/xen/arch/arm/arm64/livepatch.c @@ -0,0 +1,268 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + */ + +#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 "../livepatch.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)); + + if ( !vmap_of_xen_text ) + return; + + /* Save old one. */ + old_ptr = func->old_addr; + memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE); + + /* Branch, no link. */ + insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr, + (unsigned long)func->new_addr); + + 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; +} + +int arch_livepatch_perform_rel(struct livepatch_elf *elf, + const struct livepatch_elf_sec *base, + const struct livepatch_elf_sec *rela) +{ + return -EOPNOTSUPP; +} + + +static int reloc_insn_imm(void *dest, u64 val, int lsb, int len, + enum aarch64_insn_imm_type imm_type) +{ + u64 imm, imm_mask; + s64 sval = val; + u32 insn = *(u32 *)dest; + + /* Calculate the relocation value. */ + sval >>= lsb; + + /* Extract the value bits and shift them to bit 0. */ + imm_mask = (BIT(lsb + len) - 1) >> lsb; + imm = sval & imm_mask; + + /* Update the instruction's immediate field. */ + insn = aarch64_insn_encode_immediate(imm_type, insn, imm); + *(u32 *)dest = insn; + + /* + * Extract the upper value bits (including the sign bit) and + * shift them to bit 0. + */ + sval = (s64)(sval & ~(imm_mask >> 1)) >> (len - 1); + + /* + * Overflow has occurred if the upper bits are not all equal to + * the sign bit of the value. + */ + if ((u64)(sval + 1) >= 2) + return -EOVERFLOW; + + return 0; +} + +int arch_livepatch_perform_rela(struct livepatch_elf *elf, + const struct livepatch_elf_sec *base, + const struct livepatch_elf_sec *rela) +{ + const Elf_RelA *r; + unsigned int symndx, i; + uint64_t val; + void *dest; + + if ( !rela->sec->sh_size ) + return 0; + + if ( rela->sec->sh_entsize < sizeof(Elf_RelA) || + rela->sec->sh_size % rela->sec->sh_entsize ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Section relative header is corrupted!\n", + elf->name); + return -EINVAL; + } + + for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ ) + { + int err = 0; + + r = rela->data + i * rela->sec->sh_entsize; + + symndx = ELF64_R_SYM(r->r_info); + + if ( symndx > elf->nsym ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u which is past end!\n", + elf->name, symndx); + return -EINVAL; + } + + dest = base->load_addr + r->r_offset; /* P */ + val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */ + + /* ARM64 operations at minimum are always 32-bit. */ + if ( r->r_offset >= base->sec->sh_size || + (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size ) + goto bad_offset; + + dprintk(XENLOG_DEBUG, LIVEPATCH "%s: %s @%p val=%#lx, type=%ld\n", + elf->name, elf->sym[symndx].name, dest, val, ELF64_R_TYPE(r->r_info)); + + switch ( ELF64_R_TYPE(r->r_info) ) { + /* Data */ + case R_AARCH64_ABS64: + if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size ) + goto bad_offset; + *(int64_t *)dest = val; + break; + + case R_AARCH64_ABS32: + *(int32_t *)dest = val; + if ( (int64_t)val != *(int32_t *)dest ) + err = -EOVERFLOW; + break; + + case R_AARCH64_PREL64: + if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size ) + goto bad_offset; + + val -= (uint64_t)dest; + *(int64_t *)dest = val; + break; + + case R_AARCH64_PREL32: + val -= (uint64_t)dest; + *(int32_t *)dest = val; + if ( (int64_t)val != *(int32_t *)dest ) + err = -EOVERFLOW; + break; + + /* Instructions. */ + case R_AARCH64_ADR_PREL_LO21: + val -= (uint64_t)dest; + err = reloc_insn_imm(dest, val, 0, 21, AARCH64_INSN_IMM_ADR); + break; + + case R_AARCH64_ADR_PREL_PG_HI21: + val = (val & ~0xfff) - ((u64)dest & ~0xfff); + err = reloc_insn_imm(dest, val, 12, 21, AARCH64_INSN_IMM_ADR); + break; + + case R_AARCH64_LDST8_ABS_LO12_NC: + case R_AARCH64_ADD_ABS_LO12_NC: + err = reloc_insn_imm(dest, val, 0, 12, AARCH64_INSN_IMM_12); + if ( err == -EOVERFLOW ) + err = 0; + break; + + case R_AARCH64_LDST16_ABS_LO12_NC: + err = reloc_insn_imm(dest, val, 1, 11, AARCH64_INSN_IMM_12); + if ( err == -EOVERFLOW ) + err = 0; + break; + + case R_AARCH64_LDST32_ABS_LO12_NC: + err = reloc_insn_imm(dest, val, 2, 10, AARCH64_INSN_IMM_12); + if ( err == -EOVERFLOW ) + err = 0; + break; + + case R_AARCH64_LDST64_ABS_LO12_NC: + err = reloc_insn_imm(dest, val, 3, 9, AARCH64_INSN_IMM_12); + if ( err == -EOVERFLOW ) + err = 0; + break; + + case R_AARCH64_CONDBR19: + err = reloc_insn_imm(dest, val, 2, 19, AARCH64_INSN_IMM_19); + break; + + case R_AARCH64_JUMP26: + case R_AARCH64_CALL26: + val -= (uint64_t)dest; + err = reloc_insn_imm(dest, val, 2, 26, AARCH64_INSN_IMM_26); + break; + + default: + dprintk(XENLOG_ERR, LIVEPATCH "%s: Unhandled relocation %lu\n", + elf->name, ELF64_R_TYPE(r->r_info)); + return -EOPNOTSUPP; + } + + if ( err ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Overflow in relocation %u in %s for %s!\n", + elf->name, i, rela->name, base->name); + return err; + } + } + return 0; + + bad_offset: + dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation offset is past %s section!\n", + elf->name, base->name); + return -EINVAL; +} + +/* + * 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/arm/livepatch.c b/xen/arch/arm/livepatch.c index aba1320..67749ed 100644 --- a/xen/arch/arm/livepatch.c +++ b/xen/arch/arm/livepatch.c @@ -6,66 +6,100 @@ #include <xen/lib.h> #include <xen/livepatch_elf.h> #include <xen/livepatch.h> +#include <xen/vmap.h> +#include "livepatch.h" + +#include <asm/mm.h> + +void *vmap_of_xen_text; void arch_livepatch_quiesce(void) { + mfn_t text_mfn; + unsigned int text_order; + + if ( vmap_of_xen_text ) + return; + + text_mfn = _mfn(virt_to_mfn(_stext)); + text_order = get_order_from_bytes(_end - _start); + + /* + * 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); } void arch_livepatch_revive(void) { + /* Nuke the instruction cache */ + invalidate_icache(); + + if ( vmap_of_xen_text ) + vunmap(vmap_of_xen_text); + + vmap_of_xen_text = NULL; } int arch_livepatch_verify_func(const struct livepatch_func *func) { - return -ENOSYS; -} + /* No NOP patching yet. */ + if ( !func->new_size ) + return -EOPNOTSUPP; -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) { + /* TODO: No NMI on ARM? */ } void arch_livepatch_unmask(void) { + /* TODO: No NMI on ARM? */ } -int arch_livepatch_verify_elf(const struct livepatch_elf *elf) +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; -int arch_livepatch_perform_rel(struct livepatch_elf *elf, - const struct livepatch_elf_sec *base, - const struct livepatch_elf_sec *rela) -{ - return -ENOSYS; -} + ASSERT(va); + ASSERT(pages); -int arch_livepatch_perform_rela(struct livepatch_elf *elf, - const struct livepatch_elf_sec *base, - const struct livepatch_elf_sec *rela) -{ - return -ENOSYS; -} + if ( type == LIVEPATCH_VA_RX ) + flags = 0x2; /* R set,NX clear */ + else if ( type == LIVEPATCH_VA_RW ) + flags = 0x1; /* R clear, NX set */ + else + flags = 0x3; /* R set,NX set */ -int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type) -{ - return -ENOSYS; + modify_xen_mappings(start, start + pages * PAGE_SIZE, flags); + + return 0; } void __init arch_livepatch_init(void) { + void *start, *end; + + start = (void *)xen_virt_end; + end = (void *)FIXMAP_ADDR(0); + + BUG_ON(start >= end); + + vm_init_type(VMAP_XEN, start, end); } /* diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index eca7cdd..94b4911 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -143,6 +143,8 @@ vaddr_t xenheap_virt_end __read_mostly; vaddr_t xenheap_virt_start __read_mostly; #endif +vaddr_t xen_virt_end; + unsigned long frametable_base_pdx __read_mostly; unsigned long frametable_virt_end __read_mostly; @@ -540,6 +542,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr) /* No flush required here as page table is not hooked in yet. */ } + if ( i < LPAE_ENTRIES ) + xen_virt_end = XEN_VIRT_START + (i << PAGE_SHIFT); + pte = pte_of_xenaddr((vaddr_t)xen_xenmap); pte.pt.table = 1; write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte); diff --git a/xen/arch/arm/test/Makefile b/xen/arch/arm/test/Makefile new file mode 100644 index 0000000..c302f6a --- /dev/null +++ b/xen/arch/arm/test/Makefile @@ -0,0 +1,85 @@ +include $(XEN_ROOT)/Config.mk + +CODE_ADDR=$(shell nm --defined $(1) | grep $(2) | awk '{print "0x"$$1}') +CODE_SZ=$(shell nm --defined -S $(1) | grep $(2) | awk '{ print "0x"$$2}') + +.PHONY: default + +LIVEPATCH := xen_hello_world.livepatch +LIVEPATCH_BYE := xen_bye_world.livepatch +LIVEPATCH_REPLACE := xen_replace_world.livepatch + +default: livepatch + +install: livepatch + $(INSTALL_DATA) $(LIVEPATCH) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH) + $(INSTALL_DATA) $(LIVEPATCH_BYE) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_BYE) + $(INSTALL_DATA) $(LIVEPATCH_REPLACE) $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_REPLACE) +uninstall: + rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH) + rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_BYE) + rm -f $(DESTDIR)$(DEBUG_DIR)/$(LIVEPATCH_REPLACE) + +.PHONY: clean +clean:: + rm -f *.o .*.o.d *.livepatch config.h + +# +# To compute these values we need the binary files: xen-syms +# and xen_hello_world_func.o to be already compiled. +# +.PHONY: config.h +config.h: OLD_CODE_SZ=$(call CODE_SZ,$(BASEDIR)/xen-syms,xen_extra_version) +config.h: NEW_CODE_SZ=$(call CODE_SZ,$<,xen_hello_world) +config.h: xen_hello_world_func.o + (set -e; \ + echo "#define NEW_CODE_SZ $(NEW_CODE_SZ)"; \ + echo "#define OLD_CODE_SZ $(OLD_CODE_SZ)") > $@ + +xen_hello_world.o: config.h + +.PHONY: $(LIVEPATCH) +$(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o + $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH) $^ + +# +# This target is only accessible if CONFIG_LIVEPATCH is defined, which +# depends on $(build_id_linker) being available. Hence we do not +# need any checks. +# +# N.B. The reason we don't use arch/x86/note.o is that it may +# not be built (it is for EFI builds), and that we do not have +# the note.o.bin to muck with (as it gets deleted) +# +.PHONY: note.o +note.o: + $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin + $(OBJCOPY) -I binary -O elf64-littleaarch64 -B aarch64 \ + --rename-section=.data=.livepatch.depends -S $@.bin $@ + rm -f $@.bin + +# +# Extract the build-id of the xen_hello_world.livepatch +# (which xen_bye_world will depend on). +# +.PHONY: hello_world_note.o +hello_world_note.o: $(LIVEPATCH) + $(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) $@.bin + $(OBJCOPY) -I binary -O elf64-littleaarch64 -B aarch64 \ + --rename-section=.data=.livepatch.depends -S $@.bin $@ + rm -f $@.bin + +xen_bye_world.o: config.h + +.PHONY: $(LIVEPATCH_BYE) +$(LIVEPATCH_BYE): xen_bye_world_func.o xen_bye_world.o hello_world_note.o + $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_BYE) $^ + +xen_replace_world.o: config.h + +.PHONY: $(LIVEPATCH_REPLACE) +$(LIVEPATCH_REPLACE): xen_replace_world_func.o xen_replace_world.o note.o + $(LD) $(LDFLAGS) $(build_id_linker) -r -o $(LIVEPATCH_REPLACE) $^ + +.PHONY: livepatch +livepatch: $(LIVEPATCH) $(LIVEPATCH_BYE) $(LIVEPATCH_REPLACE) diff --git a/xen/arch/arm/test/xen_bye_world.c b/xen/arch/arm/test/xen_bye_world.c new file mode 100644 index 0000000..b75e0b1 --- /dev/null +++ b/xen/arch/arm/test/xen_bye_world.c @@ -0,0 +1,34 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#include "config.h" +#include <xen/lib.h> +#include <xen/types.h> +#include <xen/version.h> +#include <xen/livepatch.h> + +#include <public/sysctl.h> + +static char bye_world_patch_this_fnc[] = "xen_extra_version"; +extern const char *xen_bye_world(void); + +struct livepatch_func __section(".livepatch.funcs") livepatch_xen_bye_world = { + .version = LIVEPATCH_PAYLOAD_VERSION, + .name = bye_world_patch_this_fnc, + .new_addr = xen_bye_world, + .old_addr = xen_extra_version, + .new_size = NEW_CODE_SZ, + .old_size = OLD_CODE_SZ, +}; + +/* + * 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/arm/test/xen_bye_world_func.c b/xen/arch/arm/test/xen_bye_world_func.c new file mode 100644 index 0000000..32ef341 --- /dev/null +++ b/xen/arch/arm/test/xen_bye_world_func.c @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#include <xen/types.h> + +/* Our replacement function for xen_hello_world. */ +const char *xen_bye_world(void) +{ + return "Bye World!"; +} + +/* + * 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/arm/test/xen_hello_world.c b/xen/arch/arm/test/xen_hello_world.c new file mode 100644 index 0000000..e6a095b --- /dev/null +++ b/xen/arch/arm/test/xen_hello_world.c @@ -0,0 +1,69 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#include "config.h" +#include <xen/lib.h> +#include <xen/types.h> +#include <xen/version.h> +#include <xen/livepatch.h> +#include <xen/livepatch_payload.h> + +#include <public/sysctl.h> + +static char hello_world_patch_this_fnc[] = "xen_extra_version"; +extern const char *xen_hello_world(void); +static unsigned int cnt; + +static void apply_hook(void) +{ + printk(KERN_DEBUG "Hook executing.\n"); +} + +static void revert_hook(void) +{ + printk(KERN_DEBUG "Hook unloaded.\n"); +} + +static void hi_func(void) +{ + printk(KERN_DEBUG "%s: Hi! (called %u times)\n", __func__, ++cnt); +}; + +/* If we are sorted we _MUST_ be the last .livepatch.hook section. */ +static void Z_check_fnc(void) +{ + printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt); + BUG_ON(cnt == 0 || cnt > 2); + cnt = 0; /* Otherwise if you revert, apply, revert the value will be 4! */ +} + +LIVEPATCH_LOAD_HOOK(apply_hook); +LIVEPATCH_UNLOAD_HOOK(revert_hook); + +/* Imbalance here. Two load and three unload. */ + +LIVEPATCH_LOAD_HOOK(hi_func); +LIVEPATCH_UNLOAD_HOOK(hi_func); + +LIVEPATCH_UNLOAD_HOOK(Z_check_fnc); + +struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = { + .version = LIVEPATCH_PAYLOAD_VERSION, + .name = hello_world_patch_this_fnc, + .new_addr = xen_hello_world, + .old_addr = xen_extra_version, + .new_size = NEW_CODE_SZ, + .old_size = OLD_CODE_SZ, +}; + +/* + * 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/arm/test/xen_hello_world_func.c b/xen/arch/arm/test/xen_hello_world_func.c new file mode 100644 index 0000000..6a0c440 --- /dev/null +++ b/xen/arch/arm/test/xen_hello_world_func.c @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#include <xen/types.h> +#include <asm/alternative.h> + + +/* Our replacement function for xen_extra_version. */ +const char *xen_hello_world(void) +{ + asm(ALTERNATIVE("nop", "nop", 1)); + + return "Hello World"; +} + +/* + * 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/arm/test/xen_replace_world.c b/xen/arch/arm/test/xen_replace_world.c new file mode 100644 index 0000000..a2a221a --- /dev/null +++ b/xen/arch/arm/test/xen_replace_world.c @@ -0,0 +1,33 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#include "config.h" +#include <xen/lib.h> +#include <xen/types.h> +#include <xen/livepatch.h> + +#include <public/sysctl.h> + +static char xen_replace_world_name[] = "xen_extra_version"; +extern const char *xen_replace_world(void); + +struct livepatch_func __section(".livepatch.funcs") livepatch_xen_replace_world = { + .version = LIVEPATCH_PAYLOAD_VERSION, + .name = xen_replace_world_name, + .old_addr = 0, /* Forces the hypervisor to lookup .name */ + .new_addr = xen_replace_world, + .new_size = NEW_CODE_SZ, + .old_size = OLD_CODE_SZ, +}; + +/* + * 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/arm/test/xen_replace_world_func.c b/xen/arch/arm/test/xen_replace_world_func.c new file mode 100644 index 0000000..afb5cda --- /dev/null +++ b/xen/arch/arm/test/xen_replace_world_func.c @@ -0,0 +1,22 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#include <xen/types.h> + +/* Our replacement function for xen_hello_world. */ +const char *xen_replace_world(void) +{ + return "Hello Again World!"; +} + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * tab-width: 4 + * indent-tabs-mode: nil + * End: + */ diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 51afa24..8b4dfbc 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 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 88a79d8..6ffd2d0 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -618,7 +618,6 @@ static int prepare_payload(struct payload *payload, sizeof(*region->frame[i].bugs); } -#ifndef CONFIG_ARM sec = livepatch_elf_sec_by_name(elf, ".altinstructions"); if ( sec ) { @@ -636,8 +635,14 @@ static int prepare_payload(struct payload *payload, for ( a = start; a < end; a++ ) { +#ifndef CONFIG_ARM + /* TODO: Bubble ALT_ORIG_PTR up. */ const void *instr = &a->instr_offset + a->instr_offset; const void *replacement = &a->repl_offset + a->repl_offset; +#else + const void *instr = &a->orig_offset + a->orig_offset; + const void *replacement = &a->alt_offset + a->alt_offset; +#endif if ( (instr < region->start && instr >= region->end) || (replacement < region->start && replacement >= region->end) ) @@ -647,9 +652,14 @@ 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 } +#ifndef CONFIG_ARM sec = livepatch_elf_sec_by_name(elf, ".ex_table"); if ( sec ) { 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 #define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn) diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h index 19eadd2..f3e8f7e 100644 --- a/xen/include/asm-arm/mm.h +++ b/xen/include/asm-arm/mm.h @@ -120,6 +120,7 @@ extern vaddr_t xenheap_virt_end; extern vaddr_t xenheap_virt_start; #endif +extern vaddr_t xen_virt_end; #ifdef CONFIG_ARM_32 #define is_xen_heap_page(page) is_xen_heap_mfn(page_to_mfn(page)) #define is_xen_heap_mfn(mfn) ({ \ diff --git a/xen/include/xen/elfstructs.h b/xen/include/xen/elfstructs.h index 5f2082e..43a7060 100644 --- a/xen/include/xen/elfstructs.h +++ b/xen/include/xen/elfstructs.h @@ -103,6 +103,15 @@ typedef uint64_t Elf64_Xword; (ehdr).e_ident[EI_MAG2] == ELFMAG2 && \ (ehdr).e_ident[EI_MAG3] == ELFMAG3) +/* e_flags */ +#define EF_ARM_EABI_MASK 0xff000000 +#define EF_ARM_EABI_UNKNOWN 0x00000000 +#define EF_ARM_EABI_VER1 0x01000000 +#define EF_ARM_EABI_VER2 0x02000000 +#define EF_ARM_EABI_VER3 0x03000000 +#define EF_ARM_EABI_VER4 0x04000000 +#define EF_ARM_EABI_VER5 0x05000000 + /* ELF Header */ typedef struct elfhdr { unsigned char e_ident[EI_NIDENT]; /* ELF Identification */ @@ -171,6 +180,7 @@ typedef struct { #define EM_PPC 20 /* PowerPC */ #define EM_PPC64 21 /* PowerPC 64-bit */ #define EM_ARM 40 /* Advanced RISC Machines ARM */ +#define EM_AARCH64 183 #define EM_ALPHA 41 /* DEC ALPHA */ #define EM_SPARCV9 43 /* SPARC version 9 */ #define EM_ALPHA_EXP 0x9026 /* DEC ALPHA */ @@ -359,6 +369,31 @@ typedef struct { #define R_X86_64_PC32 2 /* PC relative 32 bit signed */ #define R_X86_64_PLT32 4 /* 32 bit PLT address */ +/* + * S - address of symbol. + * A - addend for relocation (r_addend) + * P - address of the dest being relocated (derieved from r_offset) + * NC - No check for overflow. + * + * The defines also use _PREL for PC-relative address, and _NC is No Check. + */ +#define R_AARCH64_ABS64 257 /* Direct 64 bit. S+A, NC*/ +#define R_AARCH64_ABS32 258 /* Direct 32 bit. S+A */ +#define R_AARCH64_PREL64 260 /* S+A-P, NC */ +#define R_AARCH64_PREL32 261 /* S+A-P */ + +#define R_AARCH64_ADR_PREL_LO21 274 /* ADR imm, [20:0]. S+A-P */ +#define R_AARCH64_ADR_PREL_PG_HI21 275 /* ADRP imm, [32:12]. Page(S+A) - Page(P).*/ +#define R_AARCH64_ADD_ABS_LO12_NC 277 /* ADD imm. from bits 11:0. S+A, NC */ + +#define R_AARCH64_CONDBR19 280 /* Bits 20:2, S+A-P */ +#define R_AARCH64_JUMP26 282 /* Bits 27:2, S+A-P */ +#define R_AARCH64_CALL26 283 /* Bits 27:2, S+A-P */ +#define R_AARCH64_LDST16_ABS_LO12_NC 284 /* LD/ST to bits 11:1, S+A, NC */ +#define R_AARCH64_LDST32_ABS_LO12_NC 285 /* LD/ST to bits 11:2, S+A, NC */ +#define R_AARCH64_LDST64_ABS_LO12_NC 286 /* LD/ST to bits 11:3, S+A, NC */ +#define R_AARCH64_LDST8_ABS_LO12_NC 278 /* LD/ST to bits 11:0, S+A, NC */ + /* Program Header */ typedef struct { Elf32_Word p_type; /* segment type */
The initial support for ARM64 - and livepatching works: (XEN) livepatch: xen_hello_world: Applying 1 functions (XEN) hi_func: Hi! (called 1 times) (XEN) Hook executing. (XEN) livepatch: xen_hello_world finished APPLY with rc=0 (XEN) livepatch.c:1687: livepatch: load_payload_fnc: rc=0 (p->rc=0) (XEN) livepatch: Hello World (XEN) 'x' pressed - Dumping all livepatch patches (XEN) build-id: e144bafc4ee8635eee5bed8e3988b484765c46c8 (XEN) name=xen_hello_world state=APPLIED(2) 0000000000318000 (.data=0000000000319000, .rodata=000000000031a000) using 3 pages. (XEN) xen_extra_version patch 0000000000233fac(12) with 0000000000318000 (16) (XEN) build-id=c4b842c276be43adbe4db788598b1e11bce04dc6 (XEN) depend-on=9affa110481e8e13606c61b21e5f6a357a3c8ef9 ARM32 still has some issues. The are some TODOs left to be done: General: - Bubble ALT_ORIG_PTR macro for both x86/ARM. - Unify the ELF RELA checks - they are the same on x86/ARM[32,64]. - Makefile generating .livepatch.depends needs to ingest the -O argument based on architecture - Test cases should move to common/ ? [Needs Jan's Ack] ARM32 issues: - vm_init_type: Assertion 'is_xen_heap_mfn(ma >> PAGE_SHIFT)' failed at xen/include/asm/mm.h:23 - Need to check R_ARM_CALL, R_ARM_JUMP24: Overflow check. - Need to implement the rest of ELF RELA Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- RFC: Wholy cow! It works! Cc: Ross Lagerwall <ross.lagerwall@citrix.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Julien Grall <julien.grall@arm.com> Cc Jan Beulich <jbeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> --- xen/arch/arm/Makefile | 14 +- xen/arch/arm/arm32/Makefile | 2 +- xen/arch/arm/arm32/livepatch.c | 150 ++++++++++++++++ xen/arch/arm/arm64/Makefile | 1 + xen/arch/arm/arm64/livepatch.c | 268 +++++++++++++++++++++++++++++ xen/arch/arm/livepatch.c | 84 ++++++--- xen/arch/arm/mm.c | 5 + xen/arch/arm/test/Makefile | 85 +++++++++ xen/arch/arm/test/xen_bye_world.c | 34 ++++ xen/arch/arm/test/xen_bye_world_func.c | 22 +++ xen/arch/arm/test/xen_hello_world.c | 69 ++++++++ xen/arch/arm/test/xen_hello_world_func.c | 26 +++ xen/arch/arm/test/xen_replace_world.c | 33 ++++ xen/arch/arm/test/xen_replace_world_func.c | 22 +++ xen/common/Kconfig | 2 +- xen/common/livepatch.c | 12 +- xen/include/asm-arm/current.h | 7 + xen/include/asm-arm/mm.h | 1 + xen/include/xen/elfstructs.h | 35 ++++ 19 files changed, 842 insertions(+), 30 deletions(-) create mode 100644 xen/arch/arm/arm32/livepatch.c create mode 100644 xen/arch/arm/arm64/livepatch.c create mode 100644 xen/arch/arm/test/Makefile create mode 100644 xen/arch/arm/test/xen_bye_world.c create mode 100644 xen/arch/arm/test/xen_bye_world_func.c create mode 100644 xen/arch/arm/test/xen_hello_world.c create mode 100644 xen/arch/arm/test/xen_hello_world_func.c create mode 100644 xen/arch/arm/test/xen_replace_world.c create mode 100644 xen/arch/arm/test/xen_replace_world_func.c