Hi Konrad,
On 15/08/16 00:07, Konrad Rzeszutek Wilk wrote:
[...]
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
new file mode 100644
index 0000000..e348942
--- /dev/null
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -0,0 +1,247 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ */
+
+#include "../livepatch.h"
+#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 <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));
+
+ ASSERT(vmap_of_xen_text);
+
+ /* Save old one. */
+ old_ptr = func->old_addr;
+ memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
+
+ if ( func->new_size )
+ insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr,
+ (unsigned long)func->new_addr,
+ AARCH64_INSN_BRANCH_NOLINK);
The function aarch64_insn_gen_branch_imm will fail to create the branch
if the difference between old_addr and new_addr is greater than 128MB.
In this case, the function will return AARCH64_BREAK_FAULT which will
crash Xen when the instruction is executed.
I cannot find any sanity check in the hypervisor code. So are we
trusting the payload?
+ else
+ insn = aarch64_insn_gen_nop();
+
+ 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;
+}
+
[...]
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 20bb2ba..607ee59 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -13,6 +13,7 @@
#include <xen/hypercall.h>
#include <xen/init.h>
#include <xen/lib.h>
+#include <xen/livepatch.h>
#include <xen/sched.h>
#include <xen/softirq.h>
#include <xen/wait.h>
@@ -55,6 +56,11 @@ void idle_loop(void)
do_tasklet();
do_softirq();
+ /*
+ * We MUST be last (or before dsb, wfi). Otherwise after we get the
+ * softirq we would execute dsb,wfi (and sleep) and not patch.
+ */
+ check_for_livepatch_work();
}
}
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 3093554..19f6033 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -1,56 +1,93 @@
/*
* Copyright (C) 2016 Citrix Systems R&D Ltd.
*/
+
Spurious change?
+#include "livepatch.h"
#include <xen/errno.h>
#include <xen/init.h>
#include <xen/lib.h>
#include <xen/livepatch_elf.h>
#include <xen/livepatch.h>
+#include <xen/vmap.h>
+
+#include <asm/mm.h>
-/* On ARM32,64 instructions are always 4 bytes long. */
-#define PATCH_INSN_SIZE 4
Rather than moving again PATCH_INSN_SIZE in this patch. Can you directly
move it in patch [1]?
+void *vmap_of_xen_text;
int arch_verify_insn_length(unsigned long len)
{
return len != PATCH_INSN_SIZE;
}
-void arch_livepatch_quiesce(void)
+int arch_livepatch_quiesce(void)
It would have been nice to move the prototype change out of this patch
to keep it "straight forward".
{
+ mfn_t text_mfn;
+ unsigned int text_order;
+
+ if ( vmap_of_xen_text )
+ return -EINVAL;
+
+ text_mfn = _mfn(virt_to_mfn(_stext));
+ text_order = get_order_from_bytes(_end - _start);
It is a bit odd that you use _stext before and the _start. But I won't
complain as I did the same in alternatives.c :/.
However, I think it should be enough to remap _stext -> _etext. I had to
map all the Xen binary for the alternatives because we may patch the
init text.
I forgot to mention it in the code, so I will send a patch to update it.
+
+ /*
+ * 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);
+
+ if ( !vmap_of_xen_text )
+ {
+ printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor!
(order=%u)\n",
+ text_order);
+ return -ENOMEM;
+ }
+ return 0;
}
void arch_livepatch_revive(void)
{
+ /*
+ * Nuke the instruction cache. It has been cleaned before in
I guess you want to replace "It" by "Data cache" otherwise it does not
make much sense.
+ * arch_livepatch_apply_jmp.
What about the payload? It may contain instructions, so we need to
ensure that all the data reached the memory.
+ */
+ invalidate_icache();
+
+ if ( vmap_of_xen_text )
+ vunmap(vmap_of_xen_text);
+
+ vmap_of_xen_text = NULL;
+
+ /*
+ * Need to flush the branch predicator for ARMv7 as it may be
s/predicator/predictor/
+ * architecturally visible to the software (see B2.2.4 in ARM DDI 0406C.b).
+ */
+ flush_xen_text_tlb_local();
I am a bit confused. In your comment you mention the branch but flush
the TLBs. The two are not related.
However, I would prefer the branch predictor to be flushed directly in
invalidate_icache by calling BPIALLIS. This is because flushing the
cache means that you likely want to flush the branch predictor too.
}
int arch_livepatch_verify_func(const struct livepatch_func *func)
{
- return -ENOSYS;
-}
-
-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)
{
+ /* Mask System Error (SError) */
+ local_abort_disable();
}
void arch_livepatch_unmask(void)
{
-}
-
-int arch_livepatch_verify_elf(const struct livepatch_elf *elf)
-{
- return -ENOSYS;
+ local_abort_enable();
}
int arch_livepatch_perform_rel(struct livepatch_elf *elf,
@@ -60,20 +97,34 @@ int arch_livepatch_perform_rel(struct livepatch_elf *elf,
return -ENOSYS;
}
-int arch_livepatch_perform_rela(struct livepatch_elf *elf,
- const struct livepatch_elf_sec *base,
- const struct livepatch_elf_sec *rela)
-{
- return -ENOSYS;
-}
-
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;
+
+ ASSERT(va);
+ ASSERT(pages);
+
+ if ( type == LIVEPATCH_VA_RX )
+ flags = PTE_RO; /* R set, NX clear */
+ else if ( type == LIVEPATCH_VA_RW )
+ flags = PTE_NX; /* R clear, NX set */
+ else
+ flags = PTE_NX | PTE_RO; /* R set, NX set */
va_type is an enum. So I would prefer to see a switch. So we can catch
more easily any addition of a new member.
+
+ modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
+
+ return 0;
}
void __init arch_livepatch_init(void)
{
+ void *start, *end;
+
+ start = (void *)LIVEPATCH_VMAP;
+ end = start + MB(2);
Could you define the in config.h? So in the future we can rework more
easily the memory layout.
+
+ vm_init_type(VMAP_XEN, start, end);
}
/*
diff --git a/xen/arch/arm/livepatch.h b/xen/arch/arm/livepatch.h
new file mode 100644
index 0000000..8c8d625
--- /dev/null
+++ b/xen/arch/arm/livepatch.h
I am not sure why this header is living in arch/arm/ and not
include/asm-arm/
@@ -0,0 +1,28 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#ifndef __XEN_ARM_LIVEPATCH_H__
+#define __XEN_ARM_LIVEPATCH_H__
+
+/* On ARM32,64 instructions are always 4 bytes long. */
+#define PATCH_INSN_SIZE 4
+
+/*
+ * The va of the hypervisor .text region. We need this as the
+ * normal va are write protected.
+ */
+extern void *vmap_of_xen_text;
+
+#endif /* __XEN_ARM_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/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 06c67bc..e3f3f37 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -15,10 +15,12 @@
#define PATCH_INSN_SIZE 5
-void arch_livepatch_quiesce(void)
+int arch_livepatch_quiesce(void)
{
/* Disable WP to allow changes to read-only pages. */
write_cr0(read_cr0() & ~X86_CR0_WP);
+
+ return 0;
}
void arch_livepatch_revive(void)
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 51afa24..2fc76b6 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 (X86 || ARM_64) && 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 9c45270..af9443d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -682,7 +682,7 @@ static int prepare_payload(struct payload *payload,
sizeof(*region->frame[i].bugs);
}
-#ifndef CONFIG_ARM
+#ifndef CONFIG_ARM_32
I would prefer if you use CONFIG_ALTERNATIVE rather than CONFIG_ARM_32.
sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
if ( sec )
{
@@ -711,9 +711,15 @@ 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
}
+#endif
+#ifndef CONFIG_ARM
sec = livepatch_elf_sec_by_name(elf, ".ex_table");
if ( sec )
{
Any reason to not move .ex_table in an x86 specific file? Or maybe we
should define a new config option HAVE_ARCH_EX_TABLE to avoid
architecture specific check in the common code.
@@ -1083,12 +1089,17 @@ static int livepatch_list(xen_sysctl_livepatch_list_t
*list)
static int apply_payload(struct payload *data)
{
unsigned int i;
+ int rc;
printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
data->name, data->nfuncs);
- arch_livepatch_quiesce();
-
+ rc = arch_livepatch_quiesce();
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
+ return rc;
+ }
/*
* Since we are running with IRQs disabled and the hooks may call common
* code - which expects the spinlocks to run with IRQs enabled - we
temporarly
@@ -1119,10 +1130,16 @@ static int apply_payload(struct payload *data)
static int revert_payload(struct payload *data)
{
unsigned int i;
+ int rc;
printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name);
- arch_livepatch_quiesce();
+ rc = arch_livepatch_quiesce();
+ if ( rc )
+ {
+ printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name);
+ return rc;
+ }
for ( i = 0; i < data->nfuncs; i++ )
arch_livepatch_revert_jmp(&data->funcs[i]);
diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
index a96f845..8d876f6 100644
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -80,9 +80,10 @@
* 4M - 6M Fixmap: special-purpose 4K mapping slots
* 6M - 8M Early boot mapping of FDT
* 8M - 10M Early relocation address (used when relocating Xen)
+ * and later for livepatch vmap (if compiled in)
*
* ARM32 layout:
- * 0 - 8M <COMMON>
+ * 0 - 10M <COMMON>
May I ask to have this change and ...
*
* 32M - 128M Frametable: 24 bytes per page for 16GB of RAM
* 256M - 1G VMAP: ioremap and early_ioremap use this virtual address
@@ -93,7 +94,7 @@
*
* ARM64 layout:
* 0x0000000000000000 - 0x0000007fffffffff (512GB, L0 slot [0])
- * 0 - 8M <COMMON>
+ * 0 - 10M <COMMON>
this change in a separate patch to keep this patch livepatch only?
*
* 1G - 2G VMAP: ioremap and early_ioremap
*
@@ -113,6 +114,9 @@
#define FIXMAP_ADDR(n) (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
#define BOOT_FDT_VIRT_START _AT(vaddr_t,0x00600000)
#define BOOT_RELOC_VIRT_START _AT(vaddr_t,0x00800000)
+#ifdef CONFIG_LIVEPATCH
+#define LIVEPATCH_VMAP _AT(vaddr_t,0x00800000)
+#endif
#define HYPERVISOR_VIRT_START XEN_VIRT_START
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
I have commented on the previous version about switch_stack_and_jump a
while after you send this series. So I will spare you new comments here :).
#define reset_stack_and_jump(fn) switch_stack_and_jump(get_cpu_info(), fn)
[...]
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 2e64686..6f30c0d 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -72,7 +72,7 @@ int arch_livepatch_verify_func(const struct livepatch_func
*func);
* These functions are called around the critical region patching live code,
* for an architecture to take make appropratie global state adjustments.
*/
-void arch_livepatch_quiesce(void);
+int arch_livepatch_quiesce(void);
void arch_livepatch_revive(void);
void arch_livepatch_apply_jmp(struct livepatch_func *func);
[1]
https://lists.xenproject.org/archives/html/xen-devel/2016-08/msg01832.html
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
