|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 09/17] livepatch/arm[32, 64]: Modify livepatch_funcs
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@xxxxxxxxxx>
> > ---
> > 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
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |