Re: [Xen-devel] [PATCH v4 05/16] livepatch: ARM/x86: Check displacement of old_addr and new_addr

2016-09-19 Thread Julien Grall 

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 

---


[...]


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 0xea00 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,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v4 05/16] livepatch: ARM/x86: Check displacement of old_addr and new_addr

2016-09-19 Thread Jan Beulich 
>>> On 16.09.16 at 18:38,  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  /* For struct livepatch_func. */
> -#include  /* For ARCH_PATCH_INSN_SIZE. */
> +#include  /* 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


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

[Xen-devel] [PATCH v4 05/16] livepatch: ARM/x86: Check displacement of old_addr and new_addr

2016-09-16 Thread Konrad Rzeszutek Wilk 
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 

---
Cc: Andrew Cooper 
Cc: Jan Beulich 
Cc: Stefano Stabellini 
Cc: Julien Grall 

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

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 0xea00 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  /* 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  /* For SZ_* macros. */
+
 #define ARCH_PATCH_INSN_SIZE 5
+#define ARCH_LIVEPATCH_RANGE