From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Julien Grall <julien.grall@arm.com>
Cc: sstabellini@kernel.org, ross.lagerwall@citrix.com,
andrew.cooper3@citrix.com, jbeulich@suse.com,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v3 09/17] livepatch/arm[32, 64]: Modify livepatch_funcs
Date: Mon, 18 Sep 2017 20:35:12 -0400 [thread overview]
Message-ID: <20170919003511.GB24480@x230.dumpdata.com> (raw)
In-Reply-To: <84466ea2-e398-0908-3cc5-13eff3760e86@arm.com>
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2017-09-19 0:35 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-12 0:37 [PATCH v3] Livepatching patch set for 4.10 Konrad Rzeszutek Wilk
2017-09-12 0:37 ` [PATCH v3 01/17] livepatch: Expand check for safe_for_reapply if livepatch has only .rodata Konrad Rzeszutek Wilk
2017-09-12 0:37 ` [PATCH v3 02/17] livepatch: Tighten alignment checks Konrad Rzeszutek Wilk
2017-09-12 14:28 ` Jan Beulich
2017-09-12 0:37 ` [PATCH v3 03/17] livepatch: Include sizes when an mismatch occurs Konrad Rzeszutek Wilk
2017-09-12 0:37 ` [PATCH v3 04/17] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong text alignment Konrad Rzeszutek Wilk
2017-09-14 11:36 ` Julien Grall
2017-09-12 0:37 ` [PATCH v3 05/17] alternative/x86/arm32: Align altinstructions (and altinstr_replacement) sections Konrad Rzeszutek Wilk
2017-09-12 14:40 ` Jan Beulich
2017-09-12 0:37 ` [PATCH v3 06/17] xen/livepatch/x86/arm32: Force .livepatch.depends section to be uint32_t aligned Konrad Rzeszutek Wilk
2017-09-14 12:27 ` Julien Grall
2017-09-19 0:32 ` Konrad Rzeszutek Wilk
2017-09-19 11:05 ` Julien Grall
2017-09-20 14:01 ` Wei Liu
2017-09-20 21:17 ` Konrad Rzeszutek Wilk
2017-09-12 0:37 ` [PATCH v3 07/17] livepatch/arm/x86: Strip note_depends symbol from test-cases Konrad Rzeszutek Wilk
2017-09-12 14:48 ` Jan Beulich
2017-09-12 23:46 ` Konrad Rzeszutek Wilk
2017-09-13 8:51 ` Jan Beulich
2017-09-13 16:28 ` Konrad Rzeszutek Wilk
2017-09-12 0:37 ` [PATCH v3 08/17] livepatch/tests: Make sure all .livepatch.funcs sections are read-only Konrad Rzeszutek Wilk
2017-09-12 14:49 ` Jan Beulich
2017-09-19 0:36 ` Konrad Rzeszutek Wilk
2017-09-12 0:37 ` [PATCH v3 09/17] livepatch/arm[32, 64]: Modify livepatch_funcs Konrad Rzeszutek Wilk
2017-09-14 13:20 ` Julien Grall
2017-09-19 0:35 ` Konrad Rzeszutek Wilk [this message]
2017-09-19 11:09 ` Julien Grall
2017-09-12 0:37 ` [PATCH v3 10/17] livepatch: Declare live patching as a supported feature Konrad Rzeszutek Wilk
2017-09-12 0:37 ` [PATCH v3 11/17] livepatch/x86/arm[32, 64]: Use common vmap code for applying Konrad Rzeszutek Wilk
2017-09-12 14:50 ` Andrew Cooper
2017-09-12 0:37 ` [PATCH v3 12/17] livepatch/x86/arm[32, 64]: Unify arch_livepatch_revert Konrad Rzeszutek Wilk
2017-09-14 13:23 ` Julien Grall
2017-09-12 0:37 ` [PATCH v3 13/17] livepatch: Expand spin_debug_disable in [apply|revert]_payload Konrad Rzeszutek Wilk
2017-09-14 13:47 ` Julien Grall
2017-09-12 0:37 ` [PATCH v3 14/17] livepatch/x86/arm: arch/x86/mm: generalize do_page_walk() and implement arch_livepatch_lookup_mfn Konrad Rzeszutek Wilk
2017-09-12 14:54 ` Jan Beulich
2017-09-13 0:23 ` Konrad Rzeszutek Wilk
2017-09-13 8:54 ` Jan Beulich
2017-09-14 13:54 ` Julien Grall
2017-09-12 0:37 ` [PATCH v3 15/17] livepatch/x86/arm: Utilize the arch_livepatch_lookup_mfn Konrad Rzeszutek Wilk
2017-09-12 0:37 ` [PATCH v3 16/17] livepatch: Add local and global symbol resolution Konrad Rzeszutek Wilk
2017-09-12 0:37 ` [PATCH v3 17/17] livepatch: Add xen_local_symbols test-case Konrad Rzeszutek Wilk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170919003511.GB24480@x230.dumpdata.com \
--to=konrad.wilk@oracle.com \
--cc=andrew.cooper3@citrix.com \
--cc=jbeulich@suse.com \
--cc=julien.grall@arm.com \
--cc=ross.lagerwall@citrix.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.