Message ID | 20160907003153.GA6688@char.us.oracle.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> ...snip.. > > > + case R_AARCH64_ABS32: > > > + ovf = reloc_data(RELOC_OP_ABS, dest, val, 32); > > > + break; > > > > I have noticed that not all the relocations are implemented (e.g > > R_AARCH64_ABS16, R_AARCH64_MOVW_*...). I don't think there is anything > > preventing the compiler to use them. So is there any particular reasons to > > not include them? > > I followed the same principle Ross did on x86 - only implement the ones that > the compiler has generated. And that is what I initially had in the v1, but expanded > it per request. > > I can include more, but at what point should I stop? xen/arch/arm/arm64/livepatch.c | 140 +++++++ xen/include/xen/elfstructs.h | 23 ++ lines later and I added them all in.
Hi Konrad, On 07/09/2016 01:31, Konrad Rzeszutek Wilk wrote: >>> +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); >> >> I don't see any value to use a temporary variable (old_ptr) to hold >> func->old_addr here. >> >>> + >>> + if ( func->new_addr ) >>> + insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr, >>> + (unsigned long)func->new_addr, >>> + AARCH64_INSN_BRANCH_NOLINK); >>> + else >>> + insn = aarch64_insn_gen_nop(); >>> + >>> + ASSERT(insn != AARCH64_BREAK_FAULT); >> >> Could you document in the code what prevents aarch64_insn_gen_branch_imm to >> not generate a break fault instruction? > > In the excitment of getting ARM32 implementation working - forgot > to write the code that would have done the check for all the platforms > (32MB for ARM, 128MB for ARM64, and 2GB on x86). > > It will be an followup patch, like so (compile tested): > > From 508157d81eaacab7ff621a84d7d885fb1bf689cc Mon Sep 17 00:00:00 2001 > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Date: Tue, 6 Sep 2016 20:14:18 -0400 > Subject: [PATCH] livepatch: ARM/x86: Check range of old_addr between new_addr > > If the distance is too great we are in trouble - as our relocation > distance can surely be clipped, or still have a valid width - but > cause an overflow of distance. > > On various architectures the maximum displacement for a branch/jump > varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86 for 32-bit > relocations is +/- 2G. It would be worth noting that the ARM64 and ARM32 displacement are only valid for unconditional branch (conditional one supports a smaller displacement). And livepatch is only using unconditional branch. > > [Note: On x86 we could use the 64-bit jmpq instruction which > would provide much bigger displacement to do a jump, but we would > still have issues with the new function not being able to reach > any of the old functions (as all the relocations would assume 32-bit > displacement)]. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Julien Grall <julien.grall@arm.com> > > v3: New submission. > --- > xen/arch/arm/arm64/livepatch.c | 1 + > xen/common/livepatch.c | 4 ++++ > xen/include/asm-arm/livepatch.h | 8 ++++++++ > xen/include/asm-x86/livepatch.h | 23 +++++++++++++++++++++++ > xen/include/xen/livepatch.h | 18 +++++++++++++++++- > 5 files changed, 53 insertions(+), 1 deletion(-) > create mode 100644 xen/include/asm-x86/livepatch.h > > diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c > index e4d2926..27a68ab 100644 > --- a/xen/arch/arm/arm64/livepatch.c > +++ b/xen/arch/arm/arm64/livepatch.c > @@ -35,6 +35,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func) > else > insn = aarch64_insn_gen_nop(); > > + /* Verified in arch_livepatch_verify_distance. */ > ASSERT(insn != AARCH64_BREAK_FAULT); > new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; > > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c > index 86232b8..1ba6ca9 100644 > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -587,6 +587,10 @@ static int prepare_payload(struct payload *payload, > rc = resolve_old_address(f, elf); > if ( rc ) > return rc; > + > + rc = arch_livepatch_verify_distance(f); > + if ( rc ) > + return rc; > } > > sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load"); > diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h > index 8c8d625..b1806d0 100644 > --- a/xen/include/asm-arm/livepatch.h > +++ b/xen/include/asm-arm/livepatch.h > @@ -6,6 +6,8 @@ > #ifndef __XEN_ARM_LIVEPATCH_H__ > #define __XEN_ARM_LIVEPATCH_H__ > > +#include <xen/sizes.h> /* For SZ_* macros. */ > + > /* On ARM32,64 instructions are always 4 bytes long. */ > #define PATCH_INSN_SIZE 4 > > @@ -15,6 +17,12 @@ > */ > extern void *vmap_of_xen_text; > > +#ifdef CONFIG_ARM_32 > +#define LIVEPATCH_ARCH_RANGE SZ_32M > +#else > +#define LIVEPATCH_ARCH_RANGE SZ_128M > +#endif Sorry for been picky, can you document where these values come from: - ARM32: A4.3 IN ARM DDI 0406C.j and we are using only ARM instruction in Xen. - ARM64: C1.3.2 in ARM DDI 0487A.j It might also be worth to mention that this is only valid for unconditional branch. The rest looks good to me. > + > #endif /* __XEN_ARM_LIVEPATCH_H__ */ Regards,
On 07/09/2016 04:33, Konrad Rzeszutek Wilk wrote: >> ...snip.. >>>> + case R_AARCH64_ABS32: >>>> + ovf = reloc_data(RELOC_OP_ABS, dest, val, 32); >>>> + break; >>> >>> I have noticed that not all the relocations are implemented (e.g >>> R_AARCH64_ABS16, R_AARCH64_MOVW_*...). I don't think there is anything >>> preventing the compiler to use them. So is there any particular reasons to >>> not include them? >> >> I followed the same principle Ross did on x86 - only implement the ones that >> the compiler has generated. And that is what I initially had in the v1, but expanded >> it per request. >> >> I can include more, but at what point should I stop? Good question. My question was more, would it ever be possible to be generated by the same compiler when applying a patch? > > > xen/arch/arm/arm64/livepatch.c | 140 +++++++ > xen/include/xen/elfstructs.h | 23 ++ > > lines later and I added them all in. >
On Wed, Sep 07, 2016 at 11:43:27AM +0100, Julien Grall wrote: > > > On 07/09/2016 04:33, Konrad Rzeszutek Wilk wrote: > > > ...snip.. > > > > > + case R_AARCH64_ABS32: > > > > > + ovf = reloc_data(RELOC_OP_ABS, dest, val, 32); > > > > > + break; > > > > > > > > I have noticed that not all the relocations are implemented (e.g > > > > R_AARCH64_ABS16, R_AARCH64_MOVW_*...). I don't think there is anything > > > > preventing the compiler to use them. So is there any particular reasons to > > > > not include them? > > > > > > I followed the same principle Ross did on x86 - only implement the ones that > > > the compiler has generated. And that is what I initially had in the v1, but expanded > > > it per request. > > > > > > I can include more, but at what point should I stop? > > Good question. My question was more, would it ever be possible to be > generated by the same compiler when applying a patch? If the hypervisor did not have them, then the payload will most likely not have them either. And vice-versa - if hypervisor does have them, then the payload will most likely have them. Either way I think we are debating academics as I already added all that code in :-) > > > > > > > xen/arch/arm/arm64/livepatch.c | 140 +++++++ > > xen/include/xen/elfstructs.h | 23 ++ > > > > lines later and I added them all in. > > > > -- > Julien Grall
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c index e4d2926..27a68ab 100644 --- a/xen/arch/arm/arm64/livepatch.c +++ b/xen/arch/arm/arm64/livepatch.c @@ -35,6 +35,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func) else insn = aarch64_insn_gen_nop(); + /* Verified in arch_livepatch_verify_distance. */ ASSERT(insn != AARCH64_BREAK_FAULT); new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 86232b8..1ba6ca9 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -587,6 +587,10 @@ static int prepare_payload(struct payload *payload, rc = resolve_old_address(f, elf); if ( rc ) return rc; + + rc = arch_livepatch_verify_distance(f); + if ( rc ) + return rc; } sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load"); diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h index 8c8d625..b1806d0 100644 --- a/xen/include/asm-arm/livepatch.h +++ b/xen/include/asm-arm/livepatch.h @@ -6,6 +6,8 @@ #ifndef __XEN_ARM_LIVEPATCH_H__ #define __XEN_ARM_LIVEPATCH_H__ +#include <xen/sizes.h> /* For SZ_* macros. */ + /* On ARM32,64 instructions are always 4 bytes long. */ #define PATCH_INSN_SIZE 4 @@ -15,6 +17,12 @@ */ extern void *vmap_of_xen_text; +#ifdef CONFIG_ARM_32 +#define LIVEPATCH_ARCH_RANGE SZ_32M +#else +#define LIVEPATCH_ARCH_RANGE SZ_128M +#endif + #endif /* __XEN_ARM_LIVEPATCH_H__ */ /* diff --git a/xen/include/asm-x86/livepatch.h b/xen/include/asm-x86/livepatch.h new file mode 100644 index 0000000..4b77601 --- /dev/null +++ b/xen/include/asm-x86/livepatch.h @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved. + * + */ + +#ifndef __XEN_X86_LIVEPATCH_H__ +#define __XEN_X86_LIVEPATCH_H__ + +#include <xen/sizes.h> /* For SZ_* macros. */ + +#define LIVEPATCH_ARCH_RANGE SZ_2G + +#endif /* __XEN_X86_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/include/xen/livepatch.h b/xen/include/xen/livepatch.h index 243e240..55553806 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -12,6 +12,7 @@ struct livepatch_elf_sym; struct xen_sysctl_livepatch_op; #include <xen/elfstructs.h> +#include <xen/errno.h> /* For -ENOSYS or -EOVERFLOW */ #ifdef CONFIG_LIVEPATCH /* @@ -66,7 +67,23 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type types void arch_livepatch_init(void); #include <public/sysctl.h> /* For struct livepatch_func. */ +#include <asm/livepatch.h> /* For LIVEPATCH_ARCH_RANGE. */ int arch_livepatch_verify_func(const struct livepatch_func *func); + +static inline int arch_livepatch_verify_distance(const struct livepatch_func *func) +{ + long offset; + + if ( !func->new_addr ) /* Ignore NOPs. */ + return 0; + + offset = ((long)func->old_addr - (long)func->new_addr); + if ( offset < -LIVEPATCH_ARCH_RANGE || offset >= LIVEPATCH_ARCH_RANGE ) + return -EOVERFLOW; + + return 0; +} + /* * These functions are called around the critical region patching live code, * for an architecture to take make appropratie global state adjustments. @@ -91,7 +108,6 @@ void arch_livepatch_unmask(void); #define init_or_livepatch_data __initdata #define init_or_livepatch __init -#include <xen/errno.h> /* For -ENOSYS */ static inline int livepatch_op(struct xen_sysctl_livepatch_op *op) { return -ENOSYS;