| Message ID | 20170912003726.368-10-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Konrad, On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote: > This was found when porting livepatch-build-tools to ARM64/32. > > When livepatch-build-tools are built (and test-case thanks to: > livepatch/tests: Make sure all .livepatch.funcs sections are read-only) > the .livepatch.funcs are in read-only section. > > However the hypervisor uses the 'opaque' for its own purpose, that > is stashing the original code. But the .livepatch_funcs section is > in the RO vmap area so on ARM[32,64] we get a fault. This is because the payload is secure at loading and therefore before it get applied, right? I was wondering if we could either defer the call to secure_payload or make the region temporarily writeable? > > On x86 the same protection is in place. In 'arch_livepatch_quiesce' > we disable WP to allow changes to read-only pages (and in arch_live_resume I can't find any function call arch_live_resume in Xen code. Do you mean arch_livepatch_revive? > we enable the WP protection). > > On ARM[32,64] we do not have the luxury of a global register that can > be changed after boot. In lieu of that we use the vmap to create > a temporary virtual address in which we can use instead. > > To do this we need to stash during livepatch: vmap of the hypervisor > code, vmap of the .livepatch_funcs (vmap comes in page aligned virtual > addresses), offset in the vmap (in case it is not nicely aligned), and > the original first livepatch_funcs to figure out the index. > > Equipped with that we can patch livepatch functions which have > .livepatch_funcs in rodata section. > > An alternative is to add the 'W' flag during loading of the > .livepatch_funcs which would result the section being in writeable > region from the gecko. > > Note that this vmap solution could be extended to x86 as well. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > --- > xen/arch/arm/arm32/livepatch.c | 11 ++++++--- > xen/arch/arm/arm64/livepatch.c | 11 ++++++--- > xen/arch/arm/livepatch.c | 52 ++++++++++++++++++++++++++++++++--------- > xen/arch/x86/livepatch.c | 2 +- > xen/common/livepatch.c | 5 ++-- > xen/include/asm-arm/livepatch.h | 13 ++++++++--- > xen/include/xen/livepatch.h | 2 +- > 7 files changed, 71 insertions(+), 25 deletions(-) > > diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c > index 10887ace81..d793ebcaad 100644 > --- a/xen/arch/arm/arm32/livepatch.c > +++ b/xen/arch/arm/arm32/livepatch.c > @@ -16,18 +16,23 @@ void arch_livepatch_apply(struct livepatch_func *func) > uint32_t insn; > uint32_t *new_ptr; > unsigned int i, len; > + struct livepatch_func *f; > > BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque)); > BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn)); > > - ASSERT(vmap_of_xen_text); > + ASSERT(livepatch_vmap.text); > > len = livepatch_insn_len(func); > if ( !len ) > return; > > + /* Index in the vmap region. */ > + i = livepatch_vmap.va - func; > + f = (struct livepatch_func *)(livepatch_vmap.funcs + livepatch_vmap.offset) + i; > + > /* Save old ones. */ > - memcpy(func->opaque, func->old_addr, len); > + memcpy(f->opaque, func->old_addr, len); > > if ( func->new_addr ) > { > @@ -56,7 +61,7 @@ void arch_livepatch_apply(struct livepatch_func *func) > else > insn = 0xe1a00000; /* mov r0, r0 */ > > - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; > + new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text; > len = len / sizeof(uint32_t); > > /* PATCH! */ > diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c > index 2728e2a125..662bedabc3 100644 > --- a/xen/arch/arm/arm64/livepatch.c > +++ b/xen/arch/arm/arm64/livepatch.c > @@ -20,18 +20,23 @@ void arch_livepatch_apply(struct livepatch_func *func) > uint32_t insn; > uint32_t *new_ptr; > unsigned int i, len; > + struct livepatch_func *f; > > BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque)); > BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn)); > > - ASSERT(vmap_of_xen_text); > + ASSERT(livepatch_vmap.text); > > len = livepatch_insn_len(func); > if ( !len ) > return; > > + /* Index in the vmap region. */ > + i = livepatch_vmap.va - func; > + f = (struct livepatch_func *)(livepatch_vmap.funcs + livepatch_vmap.offset) + i; > + > /* Save old ones. */ > - memcpy(func->opaque, func->old_addr, len); > + memcpy(f->opaque, func->old_addr, len); > > if ( func->new_addr ) > insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr, > @@ -43,7 +48,7 @@ void arch_livepatch_apply(struct livepatch_func *func) > /* Verified in livepatch_verify_distance. */ > ASSERT(insn != AARCH64_BREAK_FAULT); > > - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; > + new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text; > len = len / sizeof(uint32_t); > > /* PATCH! */ > diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c > index 3e53524365..2f9ae8e61e 100644 > --- a/xen/arch/arm/livepatch.c > +++ b/xen/arch/arm/livepatch.c > @@ -6,6 +6,7 @@ > #include <xen/lib.h> > #include <xen/livepatch_elf.h> > #include <xen/livepatch.h> > +#include <xen/pfn.h> > #include <xen/vmap.h> > > #include <asm/cpufeature.h> > @@ -16,14 +17,18 @@ > #undef virt_to_mfn > #define virt_to_mfn(va) _mfn(__virt_to_mfn(va)) > > -void *vmap_of_xen_text; > +struct livepatch_vmap_stash livepatch_vmap; > > -int arch_livepatch_quiesce(void) > +int arch_livepatch_quiesce(struct livepatch_func *funcs, unsigned int nfuncs) > { > - mfn_t text_mfn; > + mfn_t text_mfn, rodata_mfn; > + void *vmap_addr; > unsigned int text_order; > + unsigned long va = (unsigned long)(funcs); > + unsigned int offs = va & (PAGE_SIZE - 1); > + unsigned int size = PFN_UP(offs + nfuncs * sizeof(*funcs)); > > - if ( vmap_of_xen_text ) > + if ( livepatch_vmap.text || livepatch_vmap.funcs ) > return -EINVAL; > > text_mfn = virt_to_mfn(_start); > @@ -33,16 +38,33 @@ int arch_livepatch_quiesce(void) > * The text section is read-only. So re-map Xen to be able to patch > * the code. > */ > - vmap_of_xen_text = __vmap(&text_mfn, 1U << text_order, 1, 1, PAGE_HYPERVISOR, > - VMAP_DEFAULT); > + vmap_addr = __vmap(&text_mfn, 1U << text_order, 1, 1, PAGE_HYPERVISOR, > + VMAP_DEFAULT); > > - if ( !vmap_of_xen_text ) > + if ( !vmap_addr ) > { > printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor! (order=%u)\n", > text_order); > return -ENOMEM; > } > > + livepatch_vmap.text = vmap_addr; > + livepatch_vmap.offset = offs; > + > + rodata_mfn = virt_to_mfn(va & PAGE_MASK); > + vmap_addr = __vmap(&rodata_mfn, size, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT); > + if ( !vmap_addr ) > + { > + printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of livepatch_funcs! (mfn=%"PRI_mfn", size=%u)\n", > + mfn_x(rodata_mfn), size); > + vunmap(livepatch_vmap.text); > + livepatch_vmap.text = NULL; > + return -ENOMEM; > + } > + > + livepatch_vmap.funcs = vmap_addr; > + livepatch_vmap.va = funcs; > + > return 0; > } > > @@ -54,10 +76,18 @@ void arch_livepatch_revive(void) > */ > invalidate_icache(); > > - if ( vmap_of_xen_text ) > - vunmap(vmap_of_xen_text); > + if ( livepatch_vmap.text ) > + vunmap(livepatch_vmap.text); > + > + livepatch_vmap.text = NULL; > + > + if ( livepatch_vmap.funcs ) > + vunmap(livepatch_vmap.funcs); > + > + livepatch_vmap.funcs = NULL; > > - vmap_of_xen_text = NULL; > + livepatch_vmap.va = NULL; > + livepatch_vmap.offset = 0; > } > > int arch_livepatch_verify_func(const struct livepatch_func *func) > @@ -78,7 +108,7 @@ void arch_livepatch_revert(const struct livepatch_func *func) > uint32_t *new_ptr; > unsigned int len; > > - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; > + new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text; > > len = livepatch_insn_len(func); > memcpy(new_ptr, func->opaque, len); > diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c > index 48d20fdacd..8522fcbd36 100644 > --- a/xen/arch/x86/livepatch.c > +++ b/xen/arch/x86/livepatch.c > @@ -14,7 +14,7 @@ > #include <asm/nmi.h> > #include <asm/livepatch.h> > > -int arch_livepatch_quiesce(void) > +int arch_livepatch_quiesce(struct livepatch_func *func, unsigned int nfuncs) > { > /* Disable WP to allow changes to read-only pages. */ > write_cr0(read_cr0() & ~X86_CR0_WP); > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c > index dbab8a3f6f..e707802279 100644 > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -571,7 +571,6 @@ static int prepare_payload(struct payload *payload, > if ( rc ) > return rc; > } > - > sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load"); > if ( sec ) > { > @@ -1070,7 +1069,7 @@ static int apply_payload(struct payload *data) > printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n", > data->name, data->nfuncs); > > - rc = arch_livepatch_quiesce(); > + rc = arch_livepatch_quiesce(data->funcs, data->nfuncs); > if ( rc ) > { > printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name); > @@ -1111,7 +1110,7 @@ static int revert_payload(struct payload *data) > > printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name); > > - rc = arch_livepatch_quiesce(); > + rc = arch_livepatch_quiesce(data->funcs, data->nfuncs); > if ( rc ) > { > printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name); > diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h > index 6bca79deb9..e030aedced 100644 > --- a/xen/include/asm-arm/livepatch.h > +++ b/xen/include/asm-arm/livepatch.h > @@ -12,10 +12,17 @@ > #define ARCH_PATCH_INSN_SIZE 4 > > /* > - * The va of the hypervisor .text region. We need this as the > - * normal va are write protected. > + * The va of the hypervisor .text region and the livepatch_funcs. > + * We need this as the normal va are write protected. > */ > -extern void *vmap_of_xen_text; > +struct livepatch_vmap_stash { > + void *text; /* vmap of hypervisor code. */ > + void *funcs; /* vmap of the .livepatch.funcs. */ > + unsigned int offset; /* Offset in 'funcs'. */ > + struct livepatch_func *va; /* The original va. */ > +}; > + > +extern struct livepatch_vmap_stash livepatch_vmap; > > /* These ranges are only for unconditional branches. */ > #ifdef CONFIG_ARM_32 > diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h > index e9bab87f28..a97afb92f9 100644 > --- a/xen/include/xen/livepatch.h > +++ b/xen/include/xen/livepatch.h > @@ -104,7 +104,7 @@ static inline int livepatch_verify_distance(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. > */ > -int arch_livepatch_quiesce(void); > +int arch_livepatch_quiesce(struct livepatch_func *func, unsigned int nfuncs); > void arch_livepatch_revive(void); > > void arch_livepatch_apply(struct livepatch_func *func); >
On Thu, Sep 14, 2017 at 02:20:42PM +0100, Julien Grall wrote: > Hi Konrad, > > On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote: > > This was found when porting livepatch-build-tools to ARM64/32. > > > > When livepatch-build-tools are built (and test-case thanks to: > > livepatch/tests: Make sure all .livepatch.funcs sections are read-only) > > the .livepatch.funcs are in read-only section. > > > > However the hypervisor uses the 'opaque' for its own purpose, that > > is stashing the original code. But the .livepatch_funcs section is > > in the RO vmap area so on ARM[32,64] we get a fault. > > This is because the payload is secure at loading and therefore before it get > applied, right? Yes. > > I was wondering if we could either defer the call to secure_payload or make > the region temporarily writeable? This patch creates a temporary writeable virtual address space. But the idea of making the region temporarily writeable is also possible. Is there a specific register I can use for this? > > > > > On x86 the same protection is in place. In 'arch_livepatch_quiesce' > > we disable WP to allow changes to read-only pages (and in arch_live_resume > > I can't find any function call arch_live_resume in Xen code. Do you mean > arch_livepatch_revive? Yes, let me update that. > > > we enable the WP protection). > > > > On ARM[32,64] we do not have the luxury of a global register that can > > be changed after boot. In lieu of that we use the vmap to create > > a temporary virtual address in which we can use instead. > > > > To do this we need to stash during livepatch: vmap of the hypervisor > > code, vmap of the .livepatch_funcs (vmap comes in page aligned virtual > > addresses), offset in the vmap (in case it is not nicely aligned), and > > the original first livepatch_funcs to figure out the index. > > > > Equipped with that we can patch livepatch functions which have > > .livepatch_funcs in rodata section. > > > > An alternative is to add the 'W' flag during loading of the > > .livepatch_funcs which would result the section being in writeable > > region from the gecko. > > > Note that this vmap solution could be extended to x86 as well. And also this, as there is more to it (As Andrew pointed out). > > > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > --- > > xen/arch/arm/arm32/livepatch.c | 11 ++++++--- > > xen/arch/arm/arm64/livepatch.c | 11 ++++++--- > > xen/arch/arm/livepatch.c | 52 ++++++++++++++++++++++++++++++++--------- > > xen/arch/x86/livepatch.c | 2 +- > > xen/common/livepatch.c | 5 ++-- > > xen/include/asm-arm/livepatch.h | 13 ++++++++--- > > xen/include/xen/livepatch.h | 2 +- > > 7 files changed, 71 insertions(+), 25 deletions(-) > > > > diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c > > index 10887ace81..d793ebcaad 100644 > > --- a/xen/arch/arm/arm32/livepatch.c > > +++ b/xen/arch/arm/arm32/livepatch.c > > @@ -16,18 +16,23 @@ void arch_livepatch_apply(struct livepatch_func *func) > > uint32_t insn; > > uint32_t *new_ptr; > > unsigned int i, len; > > + struct livepatch_func *f; > > BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque)); > > BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn)); > > - ASSERT(vmap_of_xen_text); > > + ASSERT(livepatch_vmap.text); > > len = livepatch_insn_len(func); > > if ( !len ) > > return; > > + /* Index in the vmap region. */ > > + i = livepatch_vmap.va - func; > > + f = (struct livepatch_func *)(livepatch_vmap.funcs + livepatch_vmap.offset) + i; > > + > > /* Save old ones. */ > > - memcpy(func->opaque, func->old_addr, len); > > + memcpy(f->opaque, func->old_addr, len); > > if ( func->new_addr ) > > { > > @@ -56,7 +61,7 @@ void arch_livepatch_apply(struct livepatch_func *func) > > else > > insn = 0xe1a00000; /* mov r0, r0 */ > > - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; > > + new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text; > > len = len / sizeof(uint32_t); > > /* PATCH! */ > > diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c > > index 2728e2a125..662bedabc3 100644 > > --- a/xen/arch/arm/arm64/livepatch.c > > +++ b/xen/arch/arm/arm64/livepatch.c > > @@ -20,18 +20,23 @@ void arch_livepatch_apply(struct livepatch_func *func) > > uint32_t insn; > > uint32_t *new_ptr; > > unsigned int i, len; > > + struct livepatch_func *f; > > BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque)); > > BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn)); > > - ASSERT(vmap_of_xen_text); > > + ASSERT(livepatch_vmap.text); > > len = livepatch_insn_len(func); > > if ( !len ) > > return; > > + /* Index in the vmap region. */ > > + i = livepatch_vmap.va - func; > > + f = (struct livepatch_func *)(livepatch_vmap.funcs + livepatch_vmap.offset) + i; > > + > > /* Save old ones. */ > > - memcpy(func->opaque, func->old_addr, len); > > + memcpy(f->opaque, func->old_addr, len); > > if ( func->new_addr ) > > insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr, > > @@ -43,7 +48,7 @@ void arch_livepatch_apply(struct livepatch_func *func) > > /* Verified in livepatch_verify_distance. */ > > ASSERT(insn != AARCH64_BREAK_FAULT); > > - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; > > + new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text; > > len = len / sizeof(uint32_t); > > /* PATCH! */ > > diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c > > index 3e53524365..2f9ae8e61e 100644 > > --- a/xen/arch/arm/livepatch.c > > +++ b/xen/arch/arm/livepatch.c > > @@ -6,6 +6,7 @@ > > #include <xen/lib.h> > > #include <xen/livepatch_elf.h> > > #include <xen/livepatch.h> > > +#include <xen/pfn.h> > > #include <xen/vmap.h> > > #include <asm/cpufeature.h> > > @@ -16,14 +17,18 @@ > > #undef virt_to_mfn > > #define virt_to_mfn(va) _mfn(__virt_to_mfn(va)) > > -void *vmap_of_xen_text; > > +struct livepatch_vmap_stash livepatch_vmap; > > -int arch_livepatch_quiesce(void) > > +int arch_livepatch_quiesce(struct livepatch_func *funcs, unsigned int nfuncs) > > { > > - mfn_t text_mfn; > > + mfn_t text_mfn, rodata_mfn; > > + void *vmap_addr; > > unsigned int text_order; > > + unsigned long va = (unsigned long)(funcs); > > + unsigned int offs = va & (PAGE_SIZE - 1); > > + unsigned int size = PFN_UP(offs + nfuncs * sizeof(*funcs)); > > - if ( vmap_of_xen_text ) > > + if ( livepatch_vmap.text || livepatch_vmap.funcs ) > > return -EINVAL; > > text_mfn = virt_to_mfn(_start); > > @@ -33,16 +38,33 @@ int arch_livepatch_quiesce(void) > > * The text section is read-only. So re-map Xen to be able to patch > > * the code. > > */ > > - vmap_of_xen_text = __vmap(&text_mfn, 1U << text_order, 1, 1, PAGE_HYPERVISOR, > > - VMAP_DEFAULT); > > + vmap_addr = __vmap(&text_mfn, 1U << text_order, 1, 1, PAGE_HYPERVISOR, > > + VMAP_DEFAULT); > > - if ( !vmap_of_xen_text ) > > + if ( !vmap_addr ) > > { > > printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor! (order=%u)\n", > > text_order); > > return -ENOMEM; > > } > > + livepatch_vmap.text = vmap_addr; > > + livepatch_vmap.offset = offs; > > + > > + rodata_mfn = virt_to_mfn(va & PAGE_MASK); > > + vmap_addr = __vmap(&rodata_mfn, size, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT); > > + if ( !vmap_addr ) > > + { > > + printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of livepatch_funcs! (mfn=%"PRI_mfn", size=%u)\n", > > + mfn_x(rodata_mfn), size); > > + vunmap(livepatch_vmap.text); > > + livepatch_vmap.text = NULL; > > + return -ENOMEM; > > + } > > + > > + livepatch_vmap.funcs = vmap_addr; > > + livepatch_vmap.va = funcs; > > + > > return 0; > > } > > @@ -54,10 +76,18 @@ void arch_livepatch_revive(void) > > */ > > invalidate_icache(); > > - if ( vmap_of_xen_text ) > > - vunmap(vmap_of_xen_text); > > + if ( livepatch_vmap.text ) > > + vunmap(livepatch_vmap.text); > > + > > + livepatch_vmap.text = NULL; > > + > > + if ( livepatch_vmap.funcs ) > > + vunmap(livepatch_vmap.funcs); > > + > > + livepatch_vmap.funcs = NULL; > > - vmap_of_xen_text = NULL; > > + livepatch_vmap.va = NULL; > > + livepatch_vmap.offset = 0; > > } > > int arch_livepatch_verify_func(const struct livepatch_func *func) > > @@ -78,7 +108,7 @@ void arch_livepatch_revert(const struct livepatch_func *func) > > uint32_t *new_ptr; > > unsigned int len; > > - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; > > + new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text; > > len = livepatch_insn_len(func); > > memcpy(new_ptr, func->opaque, len); > > diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c > > index 48d20fdacd..8522fcbd36 100644 > > --- a/xen/arch/x86/livepatch.c > > +++ b/xen/arch/x86/livepatch.c > > @@ -14,7 +14,7 @@ > > #include <asm/nmi.h> > > #include <asm/livepatch.h> > > -int arch_livepatch_quiesce(void) > > +int arch_livepatch_quiesce(struct livepatch_func *func, unsigned int nfuncs) > > { > > /* Disable WP to allow changes to read-only pages. */ > > write_cr0(read_cr0() & ~X86_CR0_WP); > > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c > > index dbab8a3f6f..e707802279 100644 > > --- a/xen/common/livepatch.c > > +++ b/xen/common/livepatch.c > > @@ -571,7 +571,6 @@ static int prepare_payload(struct payload *payload, > > if ( rc ) > > return rc; > > } > > - > > sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load"); > > if ( sec ) > > { > > @@ -1070,7 +1069,7 @@ static int apply_payload(struct payload *data) > > printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n", > > data->name, data->nfuncs); > > - rc = arch_livepatch_quiesce(); > > + rc = arch_livepatch_quiesce(data->funcs, data->nfuncs); > > if ( rc ) > > { > > printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name); > > @@ -1111,7 +1110,7 @@ static int revert_payload(struct payload *data) > > printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name); > > - rc = arch_livepatch_quiesce(); > > + rc = arch_livepatch_quiesce(data->funcs, data->nfuncs); > > if ( rc ) > > { > > printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name); > > diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h > > index 6bca79deb9..e030aedced 100644 > > --- a/xen/include/asm-arm/livepatch.h > > +++ b/xen/include/asm-arm/livepatch.h > > @@ -12,10 +12,17 @@ > > #define ARCH_PATCH_INSN_SIZE 4 > > /* > > - * The va of the hypervisor .text region. We need this as the > > - * normal va are write protected. > > + * The va of the hypervisor .text region and the livepatch_funcs. > > + * We need this as the normal va are write protected. > > */ > > -extern void *vmap_of_xen_text; > > +struct livepatch_vmap_stash { > > + void *text; /* vmap of hypervisor code. */ > > + void *funcs; /* vmap of the .livepatch.funcs. */ > > + unsigned int offset; /* Offset in 'funcs'. */ > > + struct livepatch_func *va; /* The original va. */ > > +}; > > + > > +extern struct livepatch_vmap_stash livepatch_vmap; > > /* These ranges are only for unconditional branches. */ > > #ifdef CONFIG_ARM_32 > > diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h > > index e9bab87f28..a97afb92f9 100644 > > --- a/xen/include/xen/livepatch.h > > +++ b/xen/include/xen/livepatch.h > > @@ -104,7 +104,7 @@ static inline int livepatch_verify_distance(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. > > */ > > -int arch_livepatch_quiesce(void); > > +int arch_livepatch_quiesce(struct livepatch_func *func, unsigned int nfuncs); > > void arch_livepatch_revive(void); > > void arch_livepatch_apply(struct livepatch_func *func); > > > > -- > Julien Grall
Hi Konrad, On 19/09/17 01:35, Konrad Rzeszutek Wilk wrote: > On Thu, Sep 14, 2017 at 02:20:42PM +0100, Julien Grall wrote: >> Hi Konrad, >> >> On 12/09/17 01:37, Konrad Rzeszutek Wilk wrote: >>> This was found when porting livepatch-build-tools to ARM64/32. >>> >>> When livepatch-build-tools are built (and test-case thanks to: >>> livepatch/tests: Make sure all .livepatch.funcs sections are read-only) >>> the .livepatch.funcs are in read-only section. >>> >>> However the hypervisor uses the 'opaque' for its own purpose, that >>> is stashing the original code. But the .livepatch_funcs section is >>> in the RO vmap area so on ARM[32,64] we get a fault. >> >> This is because the payload is secure at loading and therefore before it get >> applied, right? > > Yes. >> >> I was wondering if we could either defer the call to secure_payload or make >> the region temporarily writeable? > > This patch creates a temporary writeable virtual address space. > > But the idea of making the region temporarily writeable is also possible. > Is there a specific register I can use for this? There is no specific register. I was suggest to call modify_xen_mappings on the region with (RW) and then you are done switch back to RO/RX. I can't see any implication on Arm to temporary switch a mapping from read-only to read-write. I am not sure for x86. Cheers,
diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c index 10887ace81..d793ebcaad 100644 --- a/xen/arch/arm/arm32/livepatch.c +++ b/xen/arch/arm/arm32/livepatch.c @@ -16,18 +16,23 @@ void arch_livepatch_apply(struct livepatch_func *func) uint32_t insn; uint32_t *new_ptr; unsigned int i, len; + struct livepatch_func *f; BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque)); BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn)); - ASSERT(vmap_of_xen_text); + ASSERT(livepatch_vmap.text); len = livepatch_insn_len(func); if ( !len ) return; + /* Index in the vmap region. */ + i = livepatch_vmap.va - func; + f = (struct livepatch_func *)(livepatch_vmap.funcs + livepatch_vmap.offset) + i; + /* Save old ones. */ - memcpy(func->opaque, func->old_addr, len); + memcpy(f->opaque, func->old_addr, len); if ( func->new_addr ) { @@ -56,7 +61,7 @@ void arch_livepatch_apply(struct livepatch_func *func) else insn = 0xe1a00000; /* mov r0, r0 */ - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; + new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text; len = len / sizeof(uint32_t); /* PATCH! */ diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c index 2728e2a125..662bedabc3 100644 --- a/xen/arch/arm/arm64/livepatch.c +++ b/xen/arch/arm/arm64/livepatch.c @@ -20,18 +20,23 @@ void arch_livepatch_apply(struct livepatch_func *func) uint32_t insn; uint32_t *new_ptr; unsigned int i, len; + struct livepatch_func *f; BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > sizeof(func->opaque)); BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != sizeof(insn)); - ASSERT(vmap_of_xen_text); + ASSERT(livepatch_vmap.text); len = livepatch_insn_len(func); if ( !len ) return; + /* Index in the vmap region. */ + i = livepatch_vmap.va - func; + f = (struct livepatch_func *)(livepatch_vmap.funcs + livepatch_vmap.offset) + i; + /* Save old ones. */ - memcpy(func->opaque, func->old_addr, len); + memcpy(f->opaque, func->old_addr, len); if ( func->new_addr ) insn = aarch64_insn_gen_branch_imm((unsigned long)func->old_addr, @@ -43,7 +48,7 @@ void arch_livepatch_apply(struct livepatch_func *func) /* Verified in livepatch_verify_distance. */ ASSERT(insn != AARCH64_BREAK_FAULT); - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; + new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text; len = len / sizeof(uint32_t); /* PATCH! */ diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c index 3e53524365..2f9ae8e61e 100644 --- a/xen/arch/arm/livepatch.c +++ b/xen/arch/arm/livepatch.c @@ -6,6 +6,7 @@ #include <xen/lib.h> #include <xen/livepatch_elf.h> #include <xen/livepatch.h> +#include <xen/pfn.h> #include <xen/vmap.h> #include <asm/cpufeature.h> @@ -16,14 +17,18 @@ #undef virt_to_mfn #define virt_to_mfn(va) _mfn(__virt_to_mfn(va)) -void *vmap_of_xen_text; +struct livepatch_vmap_stash livepatch_vmap; -int arch_livepatch_quiesce(void) +int arch_livepatch_quiesce(struct livepatch_func *funcs, unsigned int nfuncs) { - mfn_t text_mfn; + mfn_t text_mfn, rodata_mfn; + void *vmap_addr; unsigned int text_order; + unsigned long va = (unsigned long)(funcs); + unsigned int offs = va & (PAGE_SIZE - 1); + unsigned int size = PFN_UP(offs + nfuncs * sizeof(*funcs)); - if ( vmap_of_xen_text ) + if ( livepatch_vmap.text || livepatch_vmap.funcs ) return -EINVAL; text_mfn = virt_to_mfn(_start); @@ -33,16 +38,33 @@ int arch_livepatch_quiesce(void) * The text section is read-only. So re-map Xen to be able to patch * the code. */ - vmap_of_xen_text = __vmap(&text_mfn, 1U << text_order, 1, 1, PAGE_HYPERVISOR, - VMAP_DEFAULT); + vmap_addr = __vmap(&text_mfn, 1U << text_order, 1, 1, PAGE_HYPERVISOR, + VMAP_DEFAULT); - if ( !vmap_of_xen_text ) + if ( !vmap_addr ) { printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of hypervisor! (order=%u)\n", text_order); return -ENOMEM; } + livepatch_vmap.text = vmap_addr; + livepatch_vmap.offset = offs; + + rodata_mfn = virt_to_mfn(va & PAGE_MASK); + vmap_addr = __vmap(&rodata_mfn, size, 1, 1, PAGE_HYPERVISOR, VMAP_DEFAULT); + if ( !vmap_addr ) + { + printk(XENLOG_ERR LIVEPATCH "Failed to setup vmap of livepatch_funcs! (mfn=%"PRI_mfn", size=%u)\n", + mfn_x(rodata_mfn), size); + vunmap(livepatch_vmap.text); + livepatch_vmap.text = NULL; + return -ENOMEM; + } + + livepatch_vmap.funcs = vmap_addr; + livepatch_vmap.va = funcs; + return 0; } @@ -54,10 +76,18 @@ void arch_livepatch_revive(void) */ invalidate_icache(); - if ( vmap_of_xen_text ) - vunmap(vmap_of_xen_text); + if ( livepatch_vmap.text ) + vunmap(livepatch_vmap.text); + + livepatch_vmap.text = NULL; + + if ( livepatch_vmap.funcs ) + vunmap(livepatch_vmap.funcs); + + livepatch_vmap.funcs = NULL; - vmap_of_xen_text = NULL; + livepatch_vmap.va = NULL; + livepatch_vmap.offset = 0; } int arch_livepatch_verify_func(const struct livepatch_func *func) @@ -78,7 +108,7 @@ void arch_livepatch_revert(const struct livepatch_func *func) uint32_t *new_ptr; unsigned int len; - new_ptr = func->old_addr - (void *)_start + vmap_of_xen_text; + new_ptr = func->old_addr - (void *)_start + livepatch_vmap.text; len = livepatch_insn_len(func); memcpy(new_ptr, func->opaque, len); diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index 48d20fdacd..8522fcbd36 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -14,7 +14,7 @@ #include <asm/nmi.h> #include <asm/livepatch.h> -int arch_livepatch_quiesce(void) +int arch_livepatch_quiesce(struct livepatch_func *func, unsigned int nfuncs) { /* Disable WP to allow changes to read-only pages. */ write_cr0(read_cr0() & ~X86_CR0_WP); diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index dbab8a3f6f..e707802279 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -571,7 +571,6 @@ static int prepare_payload(struct payload *payload, if ( rc ) return rc; } - sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load"); if ( sec ) { @@ -1070,7 +1069,7 @@ static int apply_payload(struct payload *data) printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n", data->name, data->nfuncs); - rc = arch_livepatch_quiesce(); + rc = arch_livepatch_quiesce(data->funcs, data->nfuncs); if ( rc ) { printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name); @@ -1111,7 +1110,7 @@ static int revert_payload(struct payload *data) printk(XENLOG_INFO LIVEPATCH "%s: Reverting\n", data->name); - rc = arch_livepatch_quiesce(); + rc = arch_livepatch_quiesce(data->funcs, data->nfuncs); if ( rc ) { printk(XENLOG_ERR LIVEPATCH "%s: unable to quiesce!\n", data->name); diff --git a/xen/include/asm-arm/livepatch.h b/xen/include/asm-arm/livepatch.h index 6bca79deb9..e030aedced 100644 --- a/xen/include/asm-arm/livepatch.h +++ b/xen/include/asm-arm/livepatch.h @@ -12,10 +12,17 @@ #define ARCH_PATCH_INSN_SIZE 4 /* - * The va of the hypervisor .text region. We need this as the - * normal va are write protected. + * The va of the hypervisor .text region and the livepatch_funcs. + * We need this as the normal va are write protected. */ -extern void *vmap_of_xen_text; +struct livepatch_vmap_stash { + void *text; /* vmap of hypervisor code. */ + void *funcs; /* vmap of the .livepatch.funcs. */ + unsigned int offset; /* Offset in 'funcs'. */ + struct livepatch_func *va; /* The original va. */ +}; + +extern struct livepatch_vmap_stash livepatch_vmap; /* These ranges are only for unconditional branches. */ #ifdef CONFIG_ARM_32 diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index e9bab87f28..a97afb92f9 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -104,7 +104,7 @@ static inline int livepatch_verify_distance(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. */ -int arch_livepatch_quiesce(void); +int arch_livepatch_quiesce(struct livepatch_func *func, unsigned int nfuncs); void arch_livepatch_revive(void); void arch_livepatch_apply(struct livepatch_func *func);
This was found when porting livepatch-build-tools to ARM64/32. When livepatch-build-tools are built (and test-case thanks to: livepatch/tests: Make sure all .livepatch.funcs sections are read-only) the .livepatch.funcs are in read-only section. However the hypervisor uses the 'opaque' for its own purpose, that is stashing the original code. But the .livepatch_funcs section is in the RO vmap area so on ARM[32,64] we get a fault. On x86 the same protection is in place. In 'arch_livepatch_quiesce' we disable WP to allow changes to read-only pages (and in arch_live_resume we enable the WP protection). On ARM[32,64] we do not have the luxury of a global register that can be changed after boot. In lieu of that we use the vmap to create a temporary virtual address in which we can use instead. To do this we need to stash during livepatch: vmap of the hypervisor code, vmap of the .livepatch_funcs (vmap comes in page aligned virtual addresses), offset in the vmap (in case it is not nicely aligned), and the original first livepatch_funcs to figure out the index. Equipped with that we can patch livepatch functions which have .livepatch_funcs in rodata section. An alternative is to add the 'W' flag during loading of the .livepatch_funcs which would result the section being in writeable region from the gecko. Note that this vmap solution could be extended to x86 as well. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- xen/arch/arm/arm32/livepatch.c | 11 ++++++--- xen/arch/arm/arm64/livepatch.c | 11 ++++++--- xen/arch/arm/livepatch.c | 52 ++++++++++++++++++++++++++++++++--------- xen/arch/x86/livepatch.c | 2 +- xen/common/livepatch.c | 5 ++-- xen/include/asm-arm/livepatch.h | 13 ++++++++--- xen/include/xen/livepatch.h | 2 +- 7 files changed, 71 insertions(+), 25 deletions(-)