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

2016-09-28 Thread Ross Lagerwall 

On 09/23/2016 04:59 PM, Konrad Rzeszutek Wilk wrote:

On Fri, Sep 23, 2016 at 11:37:27AM -0400, Konrad Rzeszutek Wilk wrote:

On Fri, Sep 23, 2016 at 03:36:27PM +0100, Ross Lagerwall wrote:

On 09/21/2016 06:32 PM, Konrad Rzeszutek Wilk wrote:

If the distance is too big 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 fortunately we are not using that.


Wouldn't this be simpler as a BUILD_BUG_ON() in arch_livepatch_init()?

Something like BUILD_BUG_ON(((XEN_VIRT_END - NR_CPUS * PAGE_SIZE) -
XEN_VIRT_START) > ARCH_LIVEPATCH_RANGE)?

And BUILD_BUG_ON(((XEN_VIRT_END - NR_CPUS * PAGE_SIZE) - XEN_VIRT_START) >
ARCH_LIVEPATCH_RANGE)

And something similar for ARM...


What does that have to with the payload? The displacement calculations(checks)
are done when we load the payload.


Ooh, you are thinking of just making sure that the displacement in virtual
addresses _should_ be OK.

And that BUILD_BUG_ON would certainly do it.

But my thinking was more of the payload either having some wacky relocations 
(say
having an initial addendum that along with the XEN_VIRT_START -> XEN_VIRT_END)
causes us to be way past the right place (especially with ARM32 where we only
have 32MB distance). And having this extra check makes me sleep better at night.


Assuming that you had build checked the maximum possible displacement in 
virtual address space, then the only way a payload would fail the check 
is if either old_addr or new_addr doesn't point to hypervisor code or 
payload code (otherwise it would be in the range), but instead points to 
some other bit of memory. In that case, the payload is so completely 
broken you've got bigger problems...


There are many ways that payloads that can be broken. This checks only a 
special case of the payload being broken -- and only if the incorrect 
bit exceeds a certain range...



I don't really object to the check but it seems like an unnecessarily 
special case.


Cheers,
--
Ross Lagerwall

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

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

2016-09-23 Thread Konrad Rzeszutek Wilk 
On Fri, Sep 23, 2016 at 11:37:27AM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Sep 23, 2016 at 03:36:27PM +0100, Ross Lagerwall wrote:
> > On 09/21/2016 06:32 PM, Konrad Rzeszutek Wilk wrote:
> > > If the distance is too big 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 fortunately we are not using that.
> > 
> > Wouldn't this be simpler as a BUILD_BUG_ON() in arch_livepatch_init()?
> > 
> > Something like BUILD_BUG_ON(((XEN_VIRT_END - NR_CPUS * PAGE_SIZE) -
> > XEN_VIRT_START) > ARCH_LIVEPATCH_RANGE)?
> > 
> > And BUILD_BUG_ON(((XEN_VIRT_END - NR_CPUS * PAGE_SIZE) - XEN_VIRT_START) >
> > ARCH_LIVEPATCH_RANGE)
> > 
> > And something similar for ARM...
> 
> What does that have to with the payload? The displacement calculations(checks)
> are done when we load the payload.

Ooh, you are thinking of just making sure that the displacement in virtual
addresses _should_ be OK.

And that BUILD_BUG_ON would certainly do it.

But my thinking was more of the payload either having some wacky relocations 
(say
having an initial addendum that along with the XEN_VIRT_START -> XEN_VIRT_END)
causes us to be way past the right place (especially with ARM32 where we only
have 32MB distance). And having this extra check makes me sleep better at night.

Adding the BUILD_BUG_ON as a futher check is fine, but as a different patch.

> > 
> > -- 
> > Ross Lagerwall

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

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

2016-09-23 Thread Konrad Rzeszutek Wilk 
On Fri, Sep 23, 2016 at 03:36:27PM +0100, Ross Lagerwall wrote:
> On 09/21/2016 06:32 PM, Konrad Rzeszutek Wilk wrote:
> > If the distance is too big 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 fortunately we are not using that.
> 
> Wouldn't this be simpler as a BUILD_BUG_ON() in arch_livepatch_init()?
> 
> Something like BUILD_BUG_ON(((XEN_VIRT_END - NR_CPUS * PAGE_SIZE) -
> XEN_VIRT_START) > ARCH_LIVEPATCH_RANGE)?
> 
> And BUILD_BUG_ON(((XEN_VIRT_END - NR_CPUS * PAGE_SIZE) - XEN_VIRT_START) >
> ARCH_LIVEPATCH_RANGE)
> 
> And something similar for ARM...

What does that have to with the payload? The displacement calculations(checks)
are done when we load the payload.
> 
> -- 
> Ross Lagerwall

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

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

2016-09-23 Thread Ross Lagerwall 

On 09/21/2016 06:32 PM, Konrad Rzeszutek Wilk wrote:

If the distance is too big 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 fortunately we are not using that.


Wouldn't this be simpler as a BUILD_BUG_ON() in arch_livepatch_init()?

Something like BUILD_BUG_ON(((XEN_VIRT_END - NR_CPUS * PAGE_SIZE) - 
XEN_VIRT_START) > ARCH_LIVEPATCH_RANGE)?


And BUILD_BUG_ON(((XEN_VIRT_END - NR_CPUS * PAGE_SIZE) - XEN_VIRT_START) 
> ARCH_LIVEPATCH_RANGE)


And something similar for ARM...

--
Ross Lagerwall

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

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

2016-09-22 Thread Julien Grall 

Hi Konrad,

On 21/09/16 18:32, Konrad Rzeszutek Wilk wrote:

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


NIT: There is no j revision of the ARMv7 manual. The latest on is 0406C.c.

With that:

Reviewed-by: Julien Grall 

Regards,

--
Julien Grall

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

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

2016-09-21 Thread Konrad Rzeszutek Wilk 
If the distance is too big 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 fortunately 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.
Move most of the docs in "livepatch: Initial ARM64 support."
Drop the cast on assigning ARCH_LIVEPATCH_RANGE to an variable.
---
 docs/misc/livepatch.markdown|  3 +++
 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 | 17 -
 6 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index ff2cfb8..16f45f3 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -1151,6 +1151,9 @@ with proper offset is used for an unconditional branch to 
the new code.
 This means that that `old_size` **MUST** be at least four bytes if patching
 in trampoline.
 
+The instruction offset is limited on ARM32 to +/- 32MB to 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.
 
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index 7cb1812..d3fc9ac 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -40,6 +40,7 @@ void arch_livepatch_apply(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 66f23e0..f2f866c 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -545,6 +545,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 SZ_2G
 
 #endif /* __XEN_X86_LIVEPATCH_H__ */
 
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index b7f66d4..b7b84e7 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 
+#include  /* For -ENOSYS or -EOVERFLOW */
 #ifdef CONFIG_LIVEPATCH
 
 /*
@@ -79,6 +80,21 @@ unsigned int livepatch_insn_len(const struct livepatch_func 
*func)
 
 return ARCH_PATCH_INSN_SIZE;
 }
+
+static inline int