| Message ID | 1474043908-12101-6-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 16.09.16 at 18:38, <konrad.wilk@oracle.com> wrote: > If the distance is too great we are in trouble - as our relocation s/great/big/ (or large), as mentioned before? > @@ -68,7 +69,7 @@ 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 ARCH_PATCH_INSN_SIZE. */ > +#include <asm/livepatch.h> /* For ARCH_[PATCH_INSN_SIZE, LIVEPATCH_RANGE]. */ Perhaps better to drop the comment - it'll get unwieldy to extend it the same way for the next addition, and it's kind of expected to include the per-arch header here. > @@ -78,6 +79,21 @@ static inline size_t livepatch_insn_len(const struct livepatch_func *func) > > return ARCH_PATCH_INSN_SIZE; > } > + > +static inline int livepatch_verify_distance(const struct livepatch_func *func) > +{ > + long offset; > + long range = (long)ARCH_LIVEPATCH_RANGE; So you've dropped a few casts, but left this one around. What good does it do? Jan
Hello Konrad, On 16/09/2016 18:38, Konrad Rzeszutek Wilk wrote: > 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 unconditional > branch/jump varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86 > for 32-bit relocations is +/- 2G. > > 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). And "furthermore would require an register or > memory location to load/store the address to." (From Jan). > > On ARM the conditional branch supports even a smaller displacement > but fortunatly we are not using that. s/fortunatly/fortunately/ > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- [...] > diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown > index 9e72897..5baaa0a 100644 > --- a/docs/misc/livepatch.markdown > +++ b/docs/misc/livepatch.markdown > @@ -1100,7 +1100,7 @@ and no .data or .bss sections. > The hypervisor should verify that the in-place patching would fit within > the code or data. > > -### Trampoline (e9 opcode) > +### Trampoline (e9 opcode), x86 > > The e9 opcode used for jmpq uses a 32-bit signed displacement. That means > we are limited to up to 2GB of virtual address to place the new code > @@ -1134,3 +1134,15 @@ that in the hypervisor is advised. > The tool for generating payloads currently does perform a compile-time > check to ensure that the function to be replaced is large enough. > > +The hypervisor also checks the displacement during loading of the payload. > + > +#### Trampoline (ea opcode), ARM > + > +The 0xea000000 instruction (with proper offset) is used for an unconditional > +branch to the new code. The opcode/encoding mentioned is wrong for AArch64. Anyway, I am not sure why you want to mention the opcode in the documentation. I think it would be enough to specify: "unconditional branch instruction (for the encoding see the ARM ARM).". > This means we are limited on ARM32 to +/- 32MB > +displacement and on ARM64 to +/- 128MB displacement. > + > +The new code is placed in the 8M - 10M virtual address space while the > +Xen code is in 2M - 4M. That gives us enough space. > + > +The hypervisor also checks the displacement during loading of the payload. Regards,
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown index 9e72897..5baaa0a 100644 --- a/docs/misc/livepatch.markdown +++ b/docs/misc/livepatch.markdown @@ -1100,7 +1100,7 @@ and no .data or .bss sections. The hypervisor should verify that the in-place patching would fit within the code or data. -### Trampoline (e9 opcode) +### Trampoline (e9 opcode), x86 The e9 opcode used for jmpq uses a 32-bit signed displacement. That means we are limited to up to 2GB of virtual address to place the new code @@ -1134,3 +1134,15 @@ that in the hypervisor is advised. The tool for generating payloads currently does perform a compile-time check to ensure that the function to be replaced is large enough. +The hypervisor also checks the displacement during loading of the payload. + +#### Trampoline (ea opcode), ARM + +The 0xea000000 instruction (with proper offset) is used for an unconditional +branch to the new code. This means we are limited on ARM32 to +/- 32MB +displacement and on ARM64 to +/- 128MB displacement. + +The new code is placed in the 8M - 10M virtual address space while the +Xen code is in 2M - 4M. That gives us enough space. + +The hypervisor also checks the displacement during loading of the payload. diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c index 49eb69b..7d593b2 100644 --- a/xen/arch/arm/arm64/livepatch.c +++ b/xen/arch/arm/arm64/livepatch.c @@ -40,6 +40,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func) else insn = aarch64_insn_gen_nop(); + /* Verified in 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 a4ce8c7..38260b9 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -540,6 +540,10 @@ static int prepare_payload(struct payload *payload, rc = resolve_old_address(f, elf); if ( rc ) return rc; + + rc = 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 929c7d9..482d74f 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 ARCH_PATCH_INSN_SIZE 4 @@ -15,6 +17,15 @@ */ extern void *vmap_of_xen_text; +/* These ranges are only for unconditional branches. */ +#ifdef CONFIG_ARM_32 +/* ARM32: A4.3 IN ARM DDI 0406C.j - we are using only ARM instructions in Xen.*/ +#define ARCH_LIVEPATCH_RANGE SZ_32M +#else +/* ARM64: C1.3.2 in ARM DDI 0487A.j */ +#define ARCH_LIVEPATCH_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 index 5e04aa1..7dfc2e7 100644 --- a/xen/include/asm-x86/livepatch.h +++ b/xen/include/asm-x86/livepatch.h @@ -6,7 +6,10 @@ #ifndef __XEN_X86_LIVEPATCH_H__ #define __XEN_X86_LIVEPATCH_H__ +#include <xen/sizes.h> /* For SZ_* macros. */ + #define ARCH_PATCH_INSN_SIZE 5 +#define ARCH_LIVEPATCH_RANGE SZ_2G #endif /* __XEN_X86_LIVEPATCH_H__ */ diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index b714fbc..6ea92b5 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 /* @@ -68,7 +69,7 @@ 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 ARCH_PATCH_INSN_SIZE. */ +#include <asm/livepatch.h> /* For ARCH_[PATCH_INSN_SIZE, LIVEPATCH_RANGE]. */ int arch_livepatch_verify_func(const struct livepatch_func *func); static inline size_t livepatch_insn_len(const struct livepatch_func *func) @@ -78,6 +79,21 @@ static inline size_t livepatch_insn_len(const struct livepatch_func *func) return ARCH_PATCH_INSN_SIZE; } + +static inline int livepatch_verify_distance(const struct livepatch_func *func) +{ + long offset; + long range = (long)ARCH_LIVEPATCH_RANGE; + + if ( !func->new_addr ) /* Ignore NOPs. */ + return 0; + + offset = func->old_addr - func->new_addr; + if ( offset < -range || offset >= 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. @@ -102,7 +118,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;
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 unconditional branch/jump varies. ARM32 is +/- 32MB, ARM64 is +/- 128MB while x86 for 32-bit relocations is +/- 2G. 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). And "furthermore would require an register or memory location to load/store the address to." (From Jan). On ARM the conditional branch supports even a smaller displacement but fortunatly we are not using that. 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. v4: s/arch_livepatch_verify_distance/livepatch_verify_distance/ s/LIVEPATCH_ARCH_RANGE/ARCH_LIVEPATCH_RANGE/ v5: Updated commit description with Jan's comment Ditch the casting of long on calculating offset. --- docs/misc/livepatch.markdown | 14 +++++++++++++- xen/arch/arm/arm64/livepatch.c | 1 + xen/common/livepatch.c | 4 ++++ xen/include/asm-arm/livepatch.h | 11 +++++++++++ xen/include/asm-x86/livepatch.h | 3 +++ xen/include/xen/livepatch.h | 19 +++++++++++++++++-- 6 files changed, 49 insertions(+), 3 deletions(-)