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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).