[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2 13/20] livepatch: Initial ARM64 support.
Hi Konrad,
On 25/08/16 14:37, Konrad Rzeszutek Wilk wrote:
As compared to x86 the va of the hypervisor .text
is locked down - we cannot modify the running pagetables
to have the .ro flag unset. We borrow the same idea that
alternative patching has - which is to vmap the entire
.text region and use the alternative virtual address
for patching.
Since we are doing vmap we may fail, hence the
arch_livepatch_quiesce was changed (see "x86,arm:
Change arch_livepatch_quiesce() decleration") to return
s/decleration/declaration/
an error value which will be bubbled in payload->rc and
provided to the user (along with messages in the ring buffer).
The livepatch virtual address space (where the new functions
are) needs to be close to the hypervisor virtual address
so that the trampoline can reach it. As such we re-use
the BOOT_RELOC_VIRT_START which is not used after bootup
(alternatively we can also use the space after the _end to
FIXMAP_ADDR(0), but that may be too small).
The ELF relocation engine at the start was coded from
the "ELF for the ARM 64-bit Architecture (AArch64)"
(http://infocenter.arm.com/help/topic/com.arm.doc.ihi0056b/IHI0056B_aaelf64.pdf)
but after a while of trying to write the correct bit shifting
and masking from scratch I ended up borrowing from Linux, the
'reloc_insn_imm' (Linux v4.7 arch/arm64/kernel/module.c function.
See 257cb251925f854da435cbf79b140984413871ac "arm64: Loadable modules")
And while at it - we also utilize code from Linux to construct
the right branch instruction (see "arm64/insn: introduce
aarch64_insn_gen_{nop|branch_imm}() helper functions").
In the livepatch payload loading code we tweak the #ifdef to
only exclude ARM_32. The exceptions are not part of ARM 32/64 hence
they are still behind the #ifdef.
We also expand the MAINTAINERS file to include the arm64 and arm32
platform specific livepatch file.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
[...]
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
new file mode 100644
index 0000000..0809a42
--- /dev/null
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -0,0 +1,323 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ */
+
+#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>
+#include <asm/livepatch.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);
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?
+ 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;
The computation here looks wrong. You are mixing (u32 *) and (void *).
+
+ /* 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;
+}
+
+enum aarch64_reloc_op {
+ RELOC_OP_NONE,
+ RELOC_OP_ABS,
+ RELOC_OP_PREL,
+ RELOC_OP_PAGE,
+};
+
+static u64 do_reloc(enum aarch64_reloc_op reloc_op, void *place, u64 val)
+{
+ switch (reloc_op) {
This seems to be a left-over of the Linux coding style.
+ case RELOC_OP_ABS:
+ return val;
+
+ case RELOC_OP_PREL:
+ return val - (u64)place;
+
+ case RELOC_OP_PAGE:
+ return (val & ~0xfff) - ((u64)place & ~0xfff);
+
+ case RELOC_OP_NONE:
+ return 0;
+
+ }
+
+ dprintk(XENLOG_DEBUG, LIVEPATCH "do_reloc: unknown relocation operation
%d\n", reloc_op);
NIT: Please add a blank line here.
+ return 0;
+}
+
+static int reloc_data(enum aarch64_reloc_op op, void *place, u64 val, int len)
+{
+ s64 sval = do_reloc(op, place, val);
+
+ switch (len) {
Same question about coding style here.
+ case 16:
+ *(s16 *)place = sval;
+ if (sval < S16_MIN || sval > U16_MAX)
Ditto
+ return -EOVERFLOW;
+ break;
+
+ case 32:
+ *(s32 *)place = sval;
+ if (sval < S32_MIN || sval > U32_MAX)
Ditto
+ return -EOVERFLOW;
+ break;
+
+ case 64:
+ *(s64 *)place = sval;
+ break;
+
+ default:
+ dprintk(XENLOG_DEBUG, LIVEPATCH "Invalid length (%d) for data
relocation\n", len);
+ return 0;
+ }
+
+ return 0;
+}
+
+enum aarch64_insn_movw_imm_type {
+ AARCH64_INSN_IMM_MOVNZ,
+ AARCH64_INSN_IMM_MOVKZ,
+};
+
+static int reloc_insn_imm(enum aarch64_reloc_op op, void *dest, u64 val,
+ int lsb, int len, enum aarch64_insn_imm_type
imm_type)
+{
+ u64 imm, imm_mask;
+ s64 sval;
+ u32 insn = *(u32 *)dest;
+
+ /* Calculate the relocation value. */
+ sval = do_reloc(op, dest, val);
+ sval >>= lsb;
+
+ /* Extract the value bits and shift them to bit 0. */
+ imm_mask = (BIT(lsb + len) - 1) >> lsb;
+ imm = sval & imm_mask;
+
+ /* Update the instruction's immediate field. */
+ insn = aarch64_insn_encode_immediate(imm_type, insn, imm);
+ *(u32 *)dest = insn;
+
+ /*
+ * Extract the upper value bits (including the sign bit) and
+ * shift them to bit 0.
+ */
+ sval = (s64)(sval & ~(imm_mask >> 1)) >> (len - 1);
+
+ /*
+ * Overflow has occurred if the upper bits are not all equal to
+ * the sign bit of the value.
+ */
+ if ((u64)(sval + 1) >= 2)
Ditto
+ return -EOVERFLOW;
+ return 0;
+}
+
+int arch_livepatch_perform_rela(struct livepatch_elf *elf,
+ const struct livepatch_elf_sec *base,
+ const struct livepatch_elf_sec *rela)
+{
+ const Elf_RelA *r;
+ unsigned int symndx, i;
+ uint64_t val;
+ void *dest;
+ bool_t overflow_check;
+
+ for ( i = 0; i < (rela->sec->sh_size / rela->sec->sh_entsize); i++ )
+ {
+ int ovf = 0;
+
+ r = rela->data + i * rela->sec->sh_entsize;
+
+ symndx = ELF64_R_SYM(r->r_info);
+
+ if ( symndx > elf->nsym )
+ {
+ dprintk(XENLOG_ERR, LIVEPATCH "%s: Relative relocation wants symbol@%u
which is past end!\n",
+ elf->name, symndx);
+ return -EINVAL;
+ }
+
+ dest = base->load_addr + r->r_offset; /* P */
+ val = elf->sym[symndx].sym->st_value + r->r_addend; /* S+A */
+
+ overflow_check = 1;
Please use true here.
+
+ /* ARM64 operations at minimum are always 32-bit. */
+ if ( r->r_offset >= base->sec->sh_size ||
+ (r->r_offset + sizeof(uint32_t)) > base->sec->sh_size )
+ goto bad_offset;
+
+ switch ( ELF64_R_TYPE(r->r_info) ) {
This seems to be a left-over of the Linux coding style.
+ /* Data */
+ case R_AARCH64_ABS64:
+ if ( r->r_offset + sizeof(uint64_t) > base->sec->sh_size )
+ goto bad_offset;
+ overflow_check = false;
+ ovf = reloc_data(RELOC_OP_ABS, dest, val, 64);
+ break;
+
+ 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?
[...]
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 755f596..f49e347 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -6,44 +6,80 @@
#include <xen/lib.h>
#include <xen/livepatch_elf.h>
#include <xen/livepatch.h>
+#include <xen/vmap.h>
+
+#include <asm/livepatch.h>
+#include <asm/mm.h>
+
+void *vmap_of_xen_text;
int arch_livepatch_quiesce(void)
{
- return -ENOSYS;
+ mfn_t text_mfn;
+ unsigned int text_order;
+
+ if ( vmap_of_xen_text )
+ return -EINVAL;
+
+ text_mfn = _mfn(virt_to_mfn(_start));
+ text_order = get_order_from_bytes(_end - _start);
+
+ /*
+ * 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,
NIT: Could you use 1U here?
+ VMAP_DEFAULT);
+
+ if ( !vmap_of_xen_text )
+ {
+ printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor!
(order=%u)\n",
+ text_order);
+ return -ENOMEM;
+ }
NIT: Missing blank line here.
+ return 0;
}
[...]
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 = 0;
+
+ ASSERT(va);
+ ASSERT(pages);
+
+ switch ( type ) {
NIT: The brace should be on a newline.
+ case LIVEPATCH_VA_RX:
+ flags = PTE_RO; /* R set, NX clear */
+ break;
+
+ case LIVEPATCH_VA_RW:
+ flags = PTE_NX; /* R clear, NX set */
+ break;
+
+ case LIVEPATCH_VA_RO:
+ flags = PTE_NX | PTE_RO; /* R set, NX set */
+ break;
+
+ default:
+ return -EINVAL;
+ }
+
+ return modify_xen_mappings(start, start + pages * PAGE_SIZE, flags);
}
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
