* [PATCH v6 1/6] livepatch: Disallow applying after an revert
2016-09-16 15:29 [PATCH v6] Livepatch fixes and general features for Xen4.8 Konrad Rzeszutek Wilk
@ 2016-09-16 15:29 ` Konrad Rzeszutek Wilk
2016-09-19 8:40 ` Jan Beulich
2016-09-21 12:47 ` Ross Lagerwall
2016-09-16 15:29 ` [PATCH v6 2/6] livepatch: Add limit of 2MB to payload .bss sections Konrad Rzeszutek Wilk
` (4 subsequent siblings)
5 siblings, 2 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-16 15:29 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall, andrew.cooper3
Cc: Jan Beulich, Konrad Rzeszutek Wilk
On general this is unhealthy - as the payload's .bss (definitly)
or .data (maybe) will be modified once the payload is running.
Doing an revert and then re-applying the payload with a non-pristine
.bss or .data can lead to unforseen consequences (.bss are assumed
to always contain zero value but now they may have a different value).
There is one exception - if the payload contains only one .data section
- the .livepatch.funcs, then it is OK to re-apply an revert.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
v6: New submission
---
docs/misc/livepatch.markdown | 7 +++++++
xen/common/livepatch.c | 27 ++++++++++++++++++++++++++-
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 89c1050..81f4fc9 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -1061,6 +1061,13 @@ depending on the current state of data. As such it should not be attempted.
That said we should provide hook functions so that the existing data
can be changed during payload application.
+To guarantee safety we disallow re-applying an payload after it has been
+reverted. This is because we cannot guarantee that the state of .bss
+and .data to be exactly as it was during loading. Hence the administrator
+MUST unload the payload and upload it again to apply it.
+
+There is an exception to this: if the payload only has .livepatch.funcs;
+and no .data or .bss sections.
### Inline patching
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 23e4d51..967985c 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -52,6 +52,8 @@ struct livepatch_build_id {
struct payload {
uint32_t state; /* One of the LIVEPATCH_STATE_*. */
int32_t rc; /* 0 or -XEN_EXX. */
+ bool_t reverted; /* Whether it was reverted. */
+ bool_t safe_to_reapply; /* Can apply safely after revert. */
struct list_head list; /* Linked to 'payload_list'. */
const void *text_addr; /* Virtual address of .text. */
size_t text_size; /* .. and its size. */
@@ -308,7 +310,7 @@ static void calc_section(const struct livepatch_elf_sec *sec, size_t *size,
static int move_payload(struct payload *payload, struct livepatch_elf *elf)
{
void *text_buf, *ro_buf, *rw_buf;
- unsigned int i;
+ unsigned int i, rw_buf_sec, rw_buf_cnt = 0;
size_t size = 0;
unsigned int *offset;
int rc = 0;
@@ -381,7 +383,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
if ( elf->sec[i].sec->sh_flags & SHF_EXECINSTR )
buf = text_buf;
else if ( elf->sec[i].sec->sh_flags & SHF_WRITE )
+ {
buf = rw_buf;
+ rw_buf_sec = i;
+ rw_buf_cnt++;
+ }
else
buf = ro_buf;
@@ -402,6 +408,10 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
}
}
+ /* No .bss and no .data, and only on RW - .livepatch.funcs section. */
+ if ( rw_buf_cnt == 1 &&
+ !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) )
+ payload->safe_to_reapply = true;
out:
xfree(offset);
@@ -1057,6 +1067,7 @@ static int revert_payload(struct payload *data)
list_del_rcu(&data->applied_list);
unregister_virtual_region(&data->region);
+ data->reverted = true;
return 0;
}
@@ -1438,6 +1449,20 @@ static int livepatch_action(xen_sysctl_livepatch_action_t *action)
case LIVEPATCH_ACTION_APPLY:
if ( data->state == LIVEPATCH_STATE_CHECKED )
{
+ /*
+ * It is unsafe to apply an reverted payload as the .data (or .bss)
+ * may not be in in pristine condition. Hence MUST unload and then
+ * apply patch again. Unless the payload has no .bss or only one
+ * RW section (.livepatch.funcs).
+ */
+ if ( data->reverted && !data->safe_to_reapply )
+ {
+ dprintk(XENLOG_ERR, "%s%s: can't revert as payload has .data. Please unload!\n",
+ LIVEPATCH, data->name);
+ data->rc = -EINVAL;
+ break;
+ }
+
rc = build_id_dep(data, !!list_empty(&applied_list));
if ( rc )
break;
--
2.5.5
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 1/6] livepatch: Disallow applying after an revert
2016-09-16 15:29 ` [PATCH v6 1/6] livepatch: Disallow applying after an revert Konrad Rzeszutek Wilk
@ 2016-09-19 8:40 ` Jan Beulich
2016-09-21 12:47 ` Ross Lagerwall
1 sibling, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2016-09-19 8:40 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: andrew.cooper3, xen-devel, ross.lagerwall
>>> On 16.09.16 at 17:29, <konrad.wilk@oracle.com> wrote:
> On general this is unhealthy - as the payload's .bss (definitly)
> or .data (maybe) will be modified once the payload is running.
>
> Doing an revert and then re-applying the payload with a non-pristine
> .bss or .data can lead to unforseen consequences (.bss are assumed
> to always contain zero value but now they may have a different value).
>
> There is one exception - if the payload contains only one .data section
> - the .livepatch.funcs, then it is OK to re-apply an revert.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
I think Ross is actually the primary person to be listed here.
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -52,6 +52,8 @@ struct livepatch_build_id {
> struct payload {
> uint32_t state; /* One of the LIVEPATCH_STATE_*. */
> int32_t rc; /* 0 or -XEN_EXX. */
> + bool_t reverted; /* Whether it was reverted. */
> + bool_t safe_to_reapply; /* Can apply safely after revert. */
You nicely use "true" int the code below - please also use plain "bool"
here.
> @@ -381,7 +383,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> if ( elf->sec[i].sec->sh_flags & SHF_EXECINSTR )
> buf = text_buf;
> else if ( elf->sec[i].sec->sh_flags & SHF_WRITE )
> + {
> buf = rw_buf;
> + rw_buf_sec = i;
> + rw_buf_cnt++;
Wouldn't it make sense to exclude zero-size sections here? Perhaps
even worth skipping them together with !SHF_ALLOC ones (and then
of course also in the other earlier loop)? The more that ISTR you
mentioning that empty .data and .bss get created by the assembler
even if there's no respective source contribution?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 1/6] livepatch: Disallow applying after an revert
2016-09-16 15:29 ` [PATCH v6 1/6] livepatch: Disallow applying after an revert Konrad Rzeszutek Wilk
2016-09-19 8:40 ` Jan Beulich
@ 2016-09-21 12:47 ` Ross Lagerwall
1 sibling, 0 replies; 21+ messages in thread
From: Ross Lagerwall @ 2016-09-21 12:47 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: xen-devel, andrew.cooper3, Jan Beulich
On 09/16/2016 04:29 PM, Konrad Rzeszutek Wilk wrote:
> On general this is unhealthy - as the payload's .bss (definitly)
> or .data (maybe) will be modified once the payload is running.
>
> Doing an revert and then re-applying the payload with a non-pristine
> .bss or .data can lead to unforseen consequences (.bss are assumed
> to always contain zero value but now they may have a different value).
>
> There is one exception - if the payload contains only one .data section
> - the .livepatch.funcs, then it is OK to re-apply an revert.
>
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
This looks reasonable, with Jan's comments addressed.
--
Ross Lagerwall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 2/6] livepatch: Add limit of 2MB to payload .bss sections.
2016-09-16 15:29 [PATCH v6] Livepatch fixes and general features for Xen4.8 Konrad Rzeszutek Wilk
2016-09-16 15:29 ` [PATCH v6 1/6] livepatch: Disallow applying after an revert Konrad Rzeszutek Wilk
@ 2016-09-16 15:29 ` Konrad Rzeszutek Wilk
2016-09-19 8:43 ` Jan Beulich
2016-09-16 15:29 ` [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero Konrad Rzeszutek Wilk
` (3 subsequent siblings)
5 siblings, 1 reply; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-16 15:29 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall, andrew.cooper3
Cc: Jan Beulich, Konrad Rzeszutek Wilk
The initial patch: 11ff40fa7bb5fdcc69a58d0fec49c904ffca4793
"xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op" caps the
size of the binary at 2MB. We follow that in capping the size
of the .BSSes to be at maximum 2MB.
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
v5: Initial submission. Came about from conversation about
"livepatch: Clear .bss when payload is reverted"
- Use only one sh_flags comparison instead of two.
- And check for the _right_ combination (WA).
v6: Remove the logging
Move the MB(2) to a #define in the header file.
Add the newline after the addition in livepatch_elf.c.
Added Reviewed-by from Ross.
---
xen/common/livepatch_elf.c | 4 ++++
xen/include/xen/livepatch.h | 2 ++
2 files changed, 6 insertions(+)
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index cda9b27..79c290e 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -86,6 +86,10 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
delta < sizeof(Elf_Ehdr) ? "at ELF header" : "is past end");
return -EINVAL;
}
+ else if ( (sec[i].sec->sh_flags & (SHF_WRITE | SHF_ALLOC)) &&
+ sec[i].sec->sh_type == SHT_NOBITS &&
+ sec[i].sec->sh_size > BSS_MAX_SIZE )
+ return -EINVAL;
sec[i].data = data + delta;
/* Name is populated in elf_resolve_section_names. */
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 243e240..46b9fc2 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -30,6 +30,8 @@ struct xen_sysctl_livepatch_op;
#define ELF_LIVEPATCH_FUNC ".livepatch.funcs"
#define ELF_LIVEPATCH_DEPENDS ".livepatch.depends"
#define ELF_BUILD_ID_NOTE ".note.gnu.build-id"
+/* Arbitrary limit. */
+#define BSS_MAX_SIZE MB(2)
struct livepatch_symbol {
const char *name;
--
2.5.5
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 2/6] livepatch: Add limit of 2MB to payload .bss sections.
2016-09-16 15:29 ` [PATCH v6 2/6] livepatch: Add limit of 2MB to payload .bss sections Konrad Rzeszutek Wilk
@ 2016-09-19 8:43 ` Jan Beulich
0 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2016-09-19 8:43 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: andrew.cooper3, xen-devel, ross.lagerwall
>>> On 16.09.16 at 17:29, <konrad.wilk@oracle.com> wrote:
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -86,6 +86,10 @@ static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
> delta < sizeof(Elf_Ehdr) ? "at ELF header" : "is past end");
> return -EINVAL;
> }
> + else if ( (sec[i].sec->sh_flags & (SHF_WRITE | SHF_ALLOC)) &&
> + sec[i].sec->sh_type == SHT_NOBITS &&
> + sec[i].sec->sh_size > BSS_MAX_SIZE )
> + return -EINVAL;
>
> sec[i].data = data + delta;
> /* Name is populated in elf_resolve_section_names. */
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -30,6 +30,8 @@ struct xen_sysctl_livepatch_op;
> #define ELF_LIVEPATCH_FUNC ".livepatch.funcs"
> #define ELF_LIVEPATCH_DEPENDS ".livepatch.depends"
> #define ELF_BUILD_ID_NOTE ".note.gnu.build-id"
> +/* Arbitrary limit. */
> +#define BSS_MAX_SIZE MB(2)
Hmm, this wasn't quite what I was thinking about in the v5
comments: I really meant to unify this and the other 2Mb limit
into one (and then obviously with a name that's more generic).
I'm sorry for not having expressed this in an explicit enough
way.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.
2016-09-16 15:29 [PATCH v6] Livepatch fixes and general features for Xen4.8 Konrad Rzeszutek Wilk
2016-09-16 15:29 ` [PATCH v6 1/6] livepatch: Disallow applying after an revert Konrad Rzeszutek Wilk
2016-09-16 15:29 ` [PATCH v6 2/6] livepatch: Add limit of 2MB to payload .bss sections Konrad Rzeszutek Wilk
@ 2016-09-16 15:29 ` Konrad Rzeszutek Wilk
2016-09-19 8:59 ` Jan Beulich
` (2 more replies)
2016-09-16 15:29 ` [PATCH v6 4/6] livepach: Add .livepatch.hooks functions and test-case Konrad Rzeszutek Wilk
` (2 subsequent siblings)
5 siblings, 3 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-16 15:29 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall, andrew.cooper3
Cc: Jan Beulich, Konrad Rzeszutek Wilk
The NOP functionality will NOP any of the code at
the 'old_addr' or at 'name' if the 'new_addr' is zero.
The purpose of this is to NOP out calls, such as:
e8 <4-bytes-offset>
(5 byte insn), or on ARM a 4 byte insn for branching.
We need the EIP of where we need to the NOP, and that can
be provided via the `old_addr` or `name`.
If the `old_addr` is provided we will NOP 'new_size'
amount of bytes at that location.
The amount is up to 31 instructions if desired (which is
the size of the opaque member). If there is a need to NOP
more then: a) more 'struct livepatch_func' structures need
to be present, b) we have to implement a variable size
buffer (in the future), or c) first byte an unconditional
branch skipping the to be disabled code (of course provided
there are no branch targets in the middle).
While at it, also unify the code on x86 patching so
it is a bit simpler (instead of two seperate writes
just make it one memcpy).
And introduce a general livepatch_insn_len inline function
that would depend on platform specific instruction size
(for a unconditional branch). As such we also rename the
PATCH_INSN_SIZE to ARCH_PATCH_INSN_SIZE.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
v3: First submission
v4: Fix description - e9 -> e8
Remove the restriction of only doing 5 or 4 bytes.
Redo the patching code to deal with variable size of new_size.
Expand the amount of bytes we can NOP.
Move the PATCH_INSN_SIZE definition in platform specific headers
Move the get_len to livepatch_get_insn_len inline function.
v5: s/PATCH_INSN_SIZE/ARCH_PATCH_INSN_SIZE/
s/arch_livepatch_insn_len/livepatch_insn_len/
s/size_t len/unsigned int len/
Add in commit description the c) mechanism (insert an unconditional
branch).
---
docs/misc/livepatch.markdown | 7 +++++--
xen/arch/x86/alternative.c | 2 +-
xen/arch/x86/livepatch.c | 40 +++++++++++++++++++++++++--------------
xen/common/livepatch.c | 3 ++-
xen/include/asm-x86/alternative.h | 1 +
xen/include/asm-x86/livepatch.h | 21 ++++++++++++++++++++
xen/include/xen/livepatch.h | 9 +++++++++
7 files changed, 65 insertions(+), 18 deletions(-)
create mode 100644 xen/include/asm-x86/livepatch.h
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 81f4fc9..a8e70a8 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -320,10 +320,13 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be
* `new_addr` is the address of the function that is replacing the old
function. The address is filled in during relocation. The value **MUST** be
- the address of the new function in the file.
+ either the address of the new function in the file, or zero if we are NOPing out
+ at `old_addr` (and `new_size` **MUST** not be zero).
* `old_size` and `new_size` contain the sizes of the respective functions in bytes.
- The value of `old_size` **MUST** not be zero.
+ The value of `old_size` **MUST** not be zero. If the value of `new_addr` is
+ zero then `new_size` determines how many instruction bytes to NOP (up to
+ opaque size modulo smallest platform instruction - 1 byte x86 and 4 bytes on ARM).
* `version` is to be one.
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 05e3eb8..6eaa10f 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -101,7 +101,7 @@ static void __init arch_init_ideal_nops(void)
}
/* Use this to add nops to a buffer, then text_poke the whole buffer. */
-static void init_or_livepatch add_nops(void *insns, unsigned int len)
+void init_or_livepatch add_nops(void *insns, unsigned int len)
{
while ( len > 0 )
{
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 725b3f6..0b8642b 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -12,8 +12,7 @@
#include <xen/livepatch.h>
#include <asm/nmi.h>
-
-#define PATCH_INSN_SIZE 5
+#include <asm/livepatch.h>
int arch_livepatch_quiesce(void)
{
@@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
int arch_livepatch_verify_func(const struct livepatch_func *func)
{
- /* No NOP patching yet. */
- if ( !func->new_size )
+ /* If NOPing only do up to maximum amount we can put in the ->opaque. */
+ if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
return -EOPNOTSUPP;
- if ( func->old_size < PATCH_INSN_SIZE )
+ if ( func->old_size < ARCH_PATCH_INSN_SIZE )
return -EINVAL;
return 0;
@@ -43,23 +42,36 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
void arch_livepatch_apply_jmp(struct livepatch_func *func)
{
- int32_t val;
uint8_t *old_ptr;
-
- BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
- BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
+ uint8_t insn[sizeof(func->opaque)];
+ unsigned int len;
old_ptr = func->old_addr;
- memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
+ len = livepatch_insn_len(func);
+ if ( !len )
+ return;
+
+ memcpy(func->opaque, old_ptr, len);
+ if ( func->new_addr )
+ {
+ int32_t val;
+
+ BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
+
+ insn[0] = 0xe9;
+ val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
+
+ memcpy(&insn[1], &val, sizeof(val));
+ }
+ else
+ add_nops(&insn, len);
- *old_ptr++ = 0xe9; /* Relative jump */
- val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
- memcpy(old_ptr, &val, sizeof(val));
+ memcpy(old_ptr, insn, len);
}
void arch_livepatch_revert_jmp(const struct livepatch_func *func)
{
- memcpy(func->old_addr, func->opaque, PATCH_INSN_SIZE);
+ memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
}
/* Serialise the CPU pipeline. */
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 967985c..218b389 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -520,7 +520,8 @@ static int prepare_payload(struct payload *payload,
return -EOPNOTSUPP;
}
- if ( !f->new_addr || !f->new_size )
+ /* 'old_addr', 'new_addr', 'new_size' can all be zero. */
+ if ( !f->old_size )
{
dprintk(XENLOG_ERR, LIVEPATCH "%s: Address or size fields are zero!\n",
elf->name);
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index 67fc0d2..db4f08e 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -27,6 +27,7 @@ struct alt_instr {
#define ALT_ORIG_PTR(a) __ALT_PTR(a, instr_offset)
#define ALT_REPL_PTR(a) __ALT_PTR(a, repl_offset)
+extern void add_nops(void *insns, unsigned int len);
/* Similar to alternative_instructions except it can be run with IRQs enabled. */
extern void apply_alternatives(const struct alt_instr *start,
const struct alt_instr *end);
diff --git a/xen/include/asm-x86/livepatch.h b/xen/include/asm-x86/livepatch.h
new file mode 100644
index 0000000..5e04aa1
--- /dev/null
+++ b/xen/include/asm-x86/livepatch.h
@@ -0,0 +1,21 @@
+/*
+ * Copyright (c) 2016 Oracle and/or its affiliates. All rights reserved.
+ *
+ */
+
+#ifndef __XEN_X86_LIVEPATCH_H__
+#define __XEN_X86_LIVEPATCH_H__
+
+#define ARCH_PATCH_INSN_SIZE 5
+
+#endif /* __XEN_X86_LIVEPATCH_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 46b9fc2..b714fbc 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -68,7 +68,16 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type types
void arch_livepatch_init(void);
#include <public/sysctl.h> /* For struct livepatch_func. */
+#include <asm/livepatch.h> /* For ARCH_PATCH_INSN_SIZE. */
int arch_livepatch_verify_func(const struct livepatch_func *func);
+
+static inline size_t livepatch_insn_len(const struct livepatch_func *func)
+{
+ if ( !func->new_addr )
+ return func->new_size;
+
+ return ARCH_PATCH_INSN_SIZE;
+}
/*
* These functions are called around the critical region patching live code,
* for an architecture to take make appropratie global state adjustments.
--
2.5.5
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.
2016-09-16 15:29 ` [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero Konrad Rzeszutek Wilk
@ 2016-09-19 8:59 ` Jan Beulich
2016-09-19 16:11 ` Konrad Rzeszutek Wilk
2016-09-19 9:21 ` Jan Beulich
2016-09-21 13:21 ` Ross Lagerwall
2 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2016-09-19 8:59 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: andrew.cooper3, xen-devel, ross.lagerwall
>>> On 16.09.16 at 17:29, <konrad.wilk@oracle.com> wrote:
> @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
>
> int arch_livepatch_verify_func(const struct livepatch_func *func)
> {
> - /* No NOP patching yet. */
> - if ( !func->new_size )
> + /* If NOPing only do up to maximum amount we can put in the ->opaque. */
> + if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
> return -EOPNOTSUPP;
>
> - if ( func->old_size < PATCH_INSN_SIZE )
> + if ( func->old_size < ARCH_PATCH_INSN_SIZE )
> return -EINVAL;
Is that indeed a requirement when NOPing? You can easily NOP out
just a single byte on x86. Or shouldn't in that case old_size == new_size
anyway? In which case the comment further down stating that new_size
can be zero would also be wrong.
> @@ -43,23 +42,36 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
>
> void arch_livepatch_apply_jmp(struct livepatch_func *func)
> {
> - int32_t val;
> uint8_t *old_ptr;
> -
> - BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> - BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> + uint8_t insn[sizeof(func->opaque)];
> + unsigned int len;
>
> old_ptr = func->old_addr;
> - memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> + len = livepatch_insn_len(func);
> + if ( !len )
> + return;
> +
> + memcpy(func->opaque, old_ptr, len);
> + if ( func->new_addr )
> + {
> + int32_t val;
> +
> + BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
> +
> + insn[0] = 0xe9;
> + val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
> +
> + memcpy(&insn[1], &val, sizeof(val));
> + }
> + else
> + add_nops(&insn, len);
>
> - *old_ptr++ = 0xe9; /* Relative jump */
Are you btw intentionally getting rid of this comment? And with the
NOP addition here, perhaps worth dropping the _jmp from the
function name (and its revert counterpart)?
> +static inline size_t livepatch_insn_len(const struct livepatch_func *func)
I think it would be nice to use consistent types: The current sole caller
stores the result of the function in an unsigned int, and I see no reason
why the function couldn't also return such.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.
2016-09-19 8:59 ` Jan Beulich
@ 2016-09-19 16:11 ` Konrad Rzeszutek Wilk
2016-09-19 16:31 ` Jan Beulich
0 siblings, 1 reply; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-19 16:11 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, xen-devel, ross.lagerwall
On Mon, Sep 19, 2016 at 02:59:32AM -0600, Jan Beulich wrote:
> >>> On 16.09.16 at 17:29, <konrad.wilk@oracle.com> wrote:
> > @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
> >
> > int arch_livepatch_verify_func(const struct livepatch_func *func)
> > {
> > - /* No NOP patching yet. */
> > - if ( !func->new_size )
> > + /* If NOPing only do up to maximum amount we can put in the ->opaque. */
> > + if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
> > return -EOPNOTSUPP;
> >
> > - if ( func->old_size < PATCH_INSN_SIZE )
> > + if ( func->old_size < ARCH_PATCH_INSN_SIZE )
> > return -EINVAL;
>
> Is that indeed a requirement when NOPing? You can easily NOP out
> just a single byte on x86. Or shouldn't in that case old_size == new_size
> anyway? In which case the comment further down stating that new_size
The original intent behind .old_size was to guard against patching
functions that were less than our relative jump.
(The tools end up computing the .old_size as the size of the whole function
which is fine).
But with this NOPing support, you are right - we could have now an
function that is say 4 bytes long and we only need to NOP three bytes
out of it (the last instruction I assume would be 'ret').
So perhaps this check needs just needs an 'else if' , like so:
int arch_livepatch_verify_func(const struct livepatch_func *func)
{
/* If NOPing.. */
if ( !func->new_addr )
{
/* Only do up to maximum amount we can put in the ->opaque. */
if ( func->new_size > sizeof(func->opaque) )
return -EOPNOTSUPP;
/* One instruction for 'ret' and the other to NOP. */
if ( func->old_size < 2 )
return -EINVAL;
}
else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
return -EINVAL;
return 0;
}
[And update the design]
> can be zero would also be wrong.
>
> > @@ -43,23 +42,36 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
> >
> > void arch_livepatch_apply_jmp(struct livepatch_func *func)
> > {
> > - int32_t val;
> > uint8_t *old_ptr;
> > -
> > - BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> > - BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> > + uint8_t insn[sizeof(func->opaque)];
> > + unsigned int len;
> >
> > old_ptr = func->old_addr;
> > - memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> > + len = livepatch_insn_len(func);
> > + if ( !len )
> > + return;
> > +
> > + memcpy(func->opaque, old_ptr, len);
> > + if ( func->new_addr )
> > + {
> > + int32_t val;
> > +
> > + BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
> > +
> > + insn[0] = 0xe9;
> > + val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
> > +
> > + memcpy(&insn[1], &val, sizeof(val));
> > + }
> > + else
> > + add_nops(&insn, len);
> >
> > - *old_ptr++ = 0xe9; /* Relative jump */
>
> Are you btw intentionally getting rid of this comment? And with the
Not at all. Just missed it.
> NOP addition here, perhaps worth dropping the _jmp from the
> function name (and its revert counterpart)?
Ooh, good idea. But I think it maybe better as a seperate patch (as it
also touches the ARM code).
>
> > +static inline size_t livepatch_insn_len(const struct livepatch_func *func)
>
> I think it would be nice to use consistent types: The current sole caller
> stores the result of the function in an unsigned int, and I see no reason
> why the function couldn't also return such.
/me nods.
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.
2016-09-19 16:11 ` Konrad Rzeszutek Wilk
@ 2016-09-19 16:31 ` Jan Beulich
2016-09-19 17:02 ` Konrad Rzeszutek Wilk
0 siblings, 1 reply; 21+ messages in thread
From: Jan Beulich @ 2016-09-19 16:31 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: andrew.cooper3, xen-devel, ross.lagerwall
>>> On 19.09.16 at 18:11, <konrad.wilk@oracle.com> wrote:
> On Mon, Sep 19, 2016 at 02:59:32AM -0600, Jan Beulich wrote:
>> >>> On 16.09.16 at 17:29, <konrad.wilk@oracle.com> wrote:
>> > @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
>> >
>> > int arch_livepatch_verify_func(const struct livepatch_func *func)
>> > {
>> > - /* No NOP patching yet. */
>> > - if ( !func->new_size )
>> > + /* If NOPing only do up to maximum amount we can put in the ->opaque. */
>> > + if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
>> > return -EOPNOTSUPP;
>> >
>> > - if ( func->old_size < PATCH_INSN_SIZE )
>> > + if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>> > return -EINVAL;
>>
>> Is that indeed a requirement when NOPing? You can easily NOP out
>> just a single byte on x86. Or shouldn't in that case old_size == new_size
>> anyway? In which case the comment further down stating that new_size
>
> The original intent behind .old_size was to guard against patching
> functions that were less than our relative jump.
>
> (The tools end up computing the .old_size as the size of the whole function
> which is fine).
>
> But with this NOPing support, you are right - we could have now an
> function that is say 4 bytes long and we only need to NOP three bytes
> out of it (the last instruction I assume would be 'ret').
>
> So perhaps this check needs just needs an 'else if' , like so:
>
> int arch_livepatch_verify_func(const struct livepatch_func *func)
> {
> /* If NOPing.. */
> if ( !func->new_addr )
> {
> /* Only do up to maximum amount we can put in the ->opaque. */
> if ( func->new_size > sizeof(func->opaque) )
> return -EOPNOTSUPP;
>
> /* One instruction for 'ret' and the other to NOP. */
> if ( func->old_size < 2 )
> return -EINVAL;
> }
> else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
> return -EINVAL;
>
> return 0;
> }
Except that I wouldn't use 2, to not exclude patching out some
single byte in the middle of a function, without regard to what the
function's actual size is.
>> can be zero would also be wrong.
>>
>> > @@ -43,23 +42,36 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
>> >
>> > void arch_livepatch_apply_jmp(struct livepatch_func *func)
>> > {
>> > - int32_t val;
>> > uint8_t *old_ptr;
>> > -
>> > - BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
>> > - BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
>> > + uint8_t insn[sizeof(func->opaque)];
>> > + unsigned int len;
>> >
>> > old_ptr = func->old_addr;
>> > - memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
>> > + len = livepatch_insn_len(func);
>> > + if ( !len )
>> > + return;
>> > +
>> > + memcpy(func->opaque, old_ptr, len);
>> > + if ( func->new_addr )
>> > + {
>> > + int32_t val;
>> > +
>> > + BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
>> > +
>> > + insn[0] = 0xe9;
>> > + val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
>> > +
>> > + memcpy(&insn[1], &val, sizeof(val));
>> > + }
>> > + else
>> > + add_nops(&insn, len);
>> >
>> > - *old_ptr++ = 0xe9; /* Relative jump */
>>
>> Are you btw intentionally getting rid of this comment? And with the
>
> Not at all. Just missed it.
>> NOP addition here, perhaps worth dropping the _jmp from the
>> function name (and its revert counterpart)?
>
> Ooh, good idea. But I think it maybe better as a seperate patch (as it
> also touches the ARM code).
That's in the other series, isn't it?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.
2016-09-19 16:31 ` Jan Beulich
@ 2016-09-19 17:02 ` Konrad Rzeszutek Wilk
2016-09-19 20:44 ` Konrad Rzeszutek Wilk
2016-09-20 6:58 ` Jan Beulich
0 siblings, 2 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-19 17:02 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, xen-devel, ross.lagerwall
On Mon, Sep 19, 2016 at 10:31:23AM -0600, Jan Beulich wrote:
> >>> On 19.09.16 at 18:11, <konrad.wilk@oracle.com> wrote:
> > On Mon, Sep 19, 2016 at 02:59:32AM -0600, Jan Beulich wrote:
> >> >>> On 16.09.16 at 17:29, <konrad.wilk@oracle.com> wrote:
> >> > @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
> >> >
> >> > int arch_livepatch_verify_func(const struct livepatch_func *func)
> >> > {
> >> > - /* No NOP patching yet. */
> >> > - if ( !func->new_size )
> >> > + /* If NOPing only do up to maximum amount we can put in the ->opaque. */
> >> > + if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
> >> > return -EOPNOTSUPP;
> >> >
> >> > - if ( func->old_size < PATCH_INSN_SIZE )
> >> > + if ( func->old_size < ARCH_PATCH_INSN_SIZE )
> >> > return -EINVAL;
> >>
> >> Is that indeed a requirement when NOPing? You can easily NOP out
> >> just a single byte on x86. Or shouldn't in that case old_size == new_size
> >> anyway? In which case the comment further down stating that new_size
> >
> > The original intent behind .old_size was to guard against patching
> > functions that were less than our relative jump.
> >
> > (The tools end up computing the .old_size as the size of the whole function
> > which is fine).
> >
> > But with this NOPing support, you are right - we could have now an
> > function that is say 4 bytes long and we only need to NOP three bytes
> > out of it (the last instruction I assume would be 'ret').
> >
> > So perhaps this check needs just needs an 'else if' , like so:
> >
> > int arch_livepatch_verify_func(const struct livepatch_func *func)
> > {
> > /* If NOPing.. */
> > if ( !func->new_addr )
> > {
> > /* Only do up to maximum amount we can put in the ->opaque. */
> > if ( func->new_size > sizeof(func->opaque) )
> > return -EOPNOTSUPP;
> >
> > /* One instruction for 'ret' and the other to NOP. */
> > if ( func->old_size < 2 )
> > return -EINVAL;
> > }
> > else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
> > return -EINVAL;
> >
> > return 0;
> > }
>
> Except that I wouldn't use 2, to not exclude patching out some
> single byte in the middle of a function, without regard to what the
> function's actual size is.
Uh-uh.
The _new_size_ determines how many bytes to NOP (in the context of
this patch). The old_size (where we check to be at min 2) is a safety
valve to make sure we don't NOP something outside the function.
..snip..
> >> NOP addition here, perhaps worth dropping the _jmp from the
> >> function name (and its revert counterpart)?
> >
> > Ooh, good idea. But I think it maybe better as a seperate patch (as it
> > also touches the ARM code).
>
> That's in the other series, isn't it?
It expands the existing ones. Right now in 'staging' branch we have an
arch/arm/livepatch.c which has these functions in it.
Granted nothing compiles them, so I could break it in this patch.
But I already cobbled up the patch so may as well use it?
From 45abdd6a0c3a6a2ca7180c7340032ac5b2b186a4 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 19 Sep 2016 12:20:27 -0400
Subject: [PATCH] livepatch: Drop _jmp from arch_livepatch_[apply,revert]_jmp
With "livepatch: NOP if func->new_addr is zero." that name
makes no more sense.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v7: New submission
---
xen/arch/arm/livepatch.c | 4 ++--
xen/arch/x86/livepatch.c | 4 ++--
xen/include/xen/livepatch.h | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 755f596..7f067a0 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -21,11 +21,11 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
return -ENOSYS;
}
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
{
}
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func)
{
}
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 118770e..36bbc5f 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -47,7 +47,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
return 0;
}
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
{
uint8_t *old_ptr;
uint8_t insn[sizeof(func->opaque)];
@@ -76,7 +76,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
memcpy(old_ptr, insn, len);
}
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func)
{
memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
}
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 174af06..b7f66d4 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -86,8 +86,8 @@ unsigned int livepatch_insn_len(const struct livepatch_func *func)
int arch_livepatch_quiesce(void);
void arch_livepatch_revive(void);
-void arch_livepatch_apply_jmp(struct livepatch_func *func);
-void arch_livepatch_revert_jmp(const struct livepatch_func *func);
+void arch_livepatch_apply(struct livepatch_func *func);
+void arch_livepatch_revert(const struct livepatch_func *func);
void arch_livepatch_post_action(void);
void arch_livepatch_mask(void);
--
2.4.11
>
> Jan
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.
2016-09-19 17:02 ` Konrad Rzeszutek Wilk
@ 2016-09-19 20:44 ` Konrad Rzeszutek Wilk
2016-09-20 6:58 ` Jan Beulich
1 sibling, 0 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-19 20:44 UTC (permalink / raw)
To: Jan Beulich; +Cc: andrew.cooper3, xen-devel, ross.lagerwall
> > > Ooh, good idea. But I think it maybe better as a seperate patch (as it
> > > also touches the ARM code).
> >
> > That's in the other series, isn't it?
>
> It expands the existing ones. Right now in 'staging' branch we have an
> arch/arm/livepatch.c which has these functions in it.
>
> Granted nothing compiles them, so I could break it in this patch.
>
> But I already cobbled up the patch so may as well use it?
>
Tested version:
From 0b0ee8f270219f5c9092960ed8560d8e039332a9 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 19 Sep 2016 12:20:27 -0400
Subject: [PATCH] livepatch: Drop _jmp from arch_livepatch_[apply,revert]_jmp
With "livepatch: NOP if func->new_addr is zero." that name
makes no more sense.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v7: New submission
---
xen/arch/arm/livepatch.c | 4 ++--
xen/arch/x86/livepatch.c | 4 ++--
xen/common/livepatch.c | 4 ++--
xen/include/xen/livepatch.h | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 755f596..7f067a0 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -21,11 +21,11 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
return -ENOSYS;
}
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
{
}
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func)
{
}
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 118770e..36bbc5f 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -47,7 +47,7 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
return 0;
}
-void arch_livepatch_apply_jmp(struct livepatch_func *func)
+void arch_livepatch_apply(struct livepatch_func *func)
{
uint8_t *old_ptr;
uint8_t insn[sizeof(func->opaque)];
@@ -76,7 +76,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func)
memcpy(old_ptr, insn, len);
}
-void arch_livepatch_revert_jmp(const struct livepatch_func *func)
+void arch_livepatch_revert(const struct livepatch_func *func)
{
memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
}
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index ed41f39..9d2e86d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1033,7 +1033,7 @@ static int apply_payload(struct payload *data)
}
for ( i = 0; i < data->nfuncs; i++ )
- arch_livepatch_apply_jmp(&data->funcs[i]);
+ arch_livepatch_apply(&data->funcs[i]);
arch_livepatch_revive();
@@ -1062,7 +1062,7 @@ static int revert_payload(struct payload *data)
}
for ( i = 0; i < data->nfuncs; i++ )
- arch_livepatch_revert_jmp(&data->funcs[i]);
+ arch_livepatch_revert(&data->funcs[i]);
arch_livepatch_revive();
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 174af06..b7f66d4 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -86,8 +86,8 @@ unsigned int livepatch_insn_len(const struct livepatch_func *func)
int arch_livepatch_quiesce(void);
void arch_livepatch_revive(void);
-void arch_livepatch_apply_jmp(struct livepatch_func *func);
-void arch_livepatch_revert_jmp(const struct livepatch_func *func);
+void arch_livepatch_apply(struct livepatch_func *func);
+void arch_livepatch_revert(const struct livepatch_func *func);
void arch_livepatch_post_action(void);
void arch_livepatch_mask(void);
--
2.4.11
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.
2016-09-19 17:02 ` Konrad Rzeszutek Wilk
2016-09-19 20:44 ` Konrad Rzeszutek Wilk
@ 2016-09-20 6:58 ` Jan Beulich
1 sibling, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2016-09-20 6:58 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: andrew.cooper3, xen-devel, ross.lagerwall
>>> On 19.09.16 at 19:02, <konrad.wilk@oracle.com> wrote:
> On Mon, Sep 19, 2016 at 10:31:23AM -0600, Jan Beulich wrote:
>> >>> On 19.09.16 at 18:11, <konrad.wilk@oracle.com> wrote:
>> > On Mon, Sep 19, 2016 at 02:59:32AM -0600, Jan Beulich wrote:
>> >> >>> On 16.09.16 at 17:29, <konrad.wilk@oracle.com> wrote:
>> >> > @@ -31,11 +30,11 @@ void arch_livepatch_revive(void)
>> >> >
>> >> > int arch_livepatch_verify_func(const struct livepatch_func *func)
>> >> > {
>> >> > - /* No NOP patching yet. */
>> >> > - if ( !func->new_size )
>> >> > + /* If NOPing only do up to maximum amount we can put in the ->opaque.
> */
>> >> > + if ( !func->new_addr && func->new_size > sizeof(func->opaque) )
>> >> > return -EOPNOTSUPP;
>> >> >
>> >> > - if ( func->old_size < PATCH_INSN_SIZE )
>> >> > + if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>> >> > return -EINVAL;
>> >>
>> >> Is that indeed a requirement when NOPing? You can easily NOP out
>> >> just a single byte on x86. Or shouldn't in that case old_size == new_size
>> >> anyway? In which case the comment further down stating that new_size
>> >
>> > The original intent behind .old_size was to guard against patching
>> > functions that were less than our relative jump.
>> >
>> > (The tools end up computing the .old_size as the size of the whole function
>> > which is fine).
>> >
>> > But with this NOPing support, you are right - we could have now an
>> > function that is say 4 bytes long and we only need to NOP three bytes
>> > out of it (the last instruction I assume would be 'ret').
>> >
>> > So perhaps this check needs just needs an 'else if' , like so:
>> >
>> > int arch_livepatch_verify_func(const struct livepatch_func *func)
>> > {
>> > /* If NOPing.. */
>> > if ( !func->new_addr )
>> > {
>> > /* Only do up to maximum amount we can put in the ->opaque. */
>> > if ( func->new_size > sizeof(func->opaque) )
>> > return -EOPNOTSUPP;
>> >
>> > /* One instruction for 'ret' and the other to NOP. */
>> > if ( func->old_size < 2 )
>> > return -EINVAL;
>> > }
>> > else if ( func->old_size < ARCH_PATCH_INSN_SIZE )
>> > return -EINVAL;
>> >
>> > return 0;
>> > }
>>
>> Except that I wouldn't use 2, to not exclude patching out some
>> single byte in the middle of a function, without regard to what the
>> function's actual size is.
>
> Uh-uh.
>
> The _new_size_ determines how many bytes to NOP (in the context of
> this patch). The old_size (where we check to be at min 2) is a safety
> valve to make sure we don't NOP something outside the function.
Well, all this looks a little fishy to me: I don't see the relation to
functions at all here. Patching can be done anywhere - at the start
of a function, in its middle, at the end, or - in extreme cases - even
spanning function boundaries.
So perhaps all you really want then (without altering that basic
concept) is new_size <= old_size?
>> >> NOP addition here, perhaps worth dropping the _jmp from the
>> >> function name (and its revert counterpart)?
>> >
>> > Ooh, good idea. But I think it maybe better as a seperate patch (as it
>> > also touches the ARM code).
>>
>> That's in the other series, isn't it?
>
> It expands the existing ones. Right now in 'staging' branch we have an
> arch/arm/livepatch.c which has these functions in it.
>
> Granted nothing compiles them, so I could break it in this patch.
>
> But I already cobbled up the patch so may as well use it?
Oh, sure.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.
2016-09-16 15:29 ` [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero Konrad Rzeszutek Wilk
2016-09-19 8:59 ` Jan Beulich
@ 2016-09-19 9:21 ` Jan Beulich
2016-09-21 13:21 ` Ross Lagerwall
2 siblings, 0 replies; 21+ messages in thread
From: Jan Beulich @ 2016-09-19 9:21 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: andrew.cooper3, xen-devel, ross.lagerwall
>>> On 16.09.16 at 17:29, <konrad.wilk@oracle.com> wrote:
> docs/misc/livepatch.markdown | 7 +++++--
> xen/arch/x86/alternative.c | 2 +-
> xen/arch/x86/livepatch.c | 40 +++++++++++++++++++++++++--------------
> xen/common/livepatch.c | 3 ++-
> xen/include/asm-x86/alternative.h | 1 +
> xen/include/asm-x86/livepatch.h | 21 ++++++++++++++++++++
> xen/include/xen/livepatch.h | 9 +++++++++
> 7 files changed, 65 insertions(+), 18 deletions(-)
> create mode 100644 xen/include/asm-x86/livepatch.h
I've noticed only while reviewing the other series that this addition
should imo be accompanied by a change to ./MAINTAINERS.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero.
2016-09-16 15:29 ` [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero Konrad Rzeszutek Wilk
2016-09-19 8:59 ` Jan Beulich
2016-09-19 9:21 ` Jan Beulich
@ 2016-09-21 13:21 ` Ross Lagerwall
2 siblings, 0 replies; 21+ messages in thread
From: Ross Lagerwall @ 2016-09-21 13:21 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk; +Cc: xen-devel, andrew.cooper3, Jan Beulich
On 09/16/2016 04:29 PM, Konrad Rzeszutek Wilk wrote:
> The NOP functionality will NOP any of the code at
> the 'old_addr' or at 'name' if the 'new_addr' is zero.
> The purpose of this is to NOP out calls, such as:
>
> e8 <4-bytes-offset>
>
> (5 byte insn), or on ARM a 4 byte insn for branching.
>
> We need the EIP of where we need to the NOP, and that can
> be provided via the `old_addr` or `name`.
>
> If the `old_addr` is provided we will NOP 'new_size'
> amount of bytes at that location.
>
> The amount is up to 31 instructions if desired (which is
> the size of the opaque member). If there is a need to NOP
> more then: a) more 'struct livepatch_func' structures need
> to be present, b) we have to implement a variable size
> buffer (in the future), or c) first byte an unconditional
> branch skipping the to be disabled code (of course provided
> there are no branch targets in the middle).
>
> While at it, also unify the code on x86 patching so
> it is a bit simpler (instead of two seperate writes
> just make it one memcpy).
>
> And introduce a general livepatch_insn_len inline function
> that would depend on platform specific instruction size
> (for a unconditional branch). As such we also rename the
> PATCH_INSN_SIZE to ARCH_PATCH_INSN_SIZE.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
snip
> diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
> index 81f4fc9..a8e70a8 100644
> --- a/docs/misc/livepatch.markdown
> +++ b/docs/misc/livepatch.markdown
> @@ -320,10 +320,13 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be
>
> * `new_addr` is the address of the function that is replacing the old
> function. The address is filled in during relocation. The value **MUST** be
> - the address of the new function in the file.
> + either the address of the new function in the file, or zero if we are NOPing out
> + at `old_addr` (and `new_size` **MUST** not be zero).
The preceding untouched lines "...is the address of the function that is
replacing the old function. The address is filled in during
relocation..." are a bit stale now. Perhaps the whole paragraph needs to
be replaced?
>
> * `old_size` and `new_size` contain the sizes of the respective functions in bytes.
> - The value of `old_size` **MUST** not be zero.
> + The value of `old_size` **MUST** not be zero. If the value of `new_addr` is
> + zero then `new_size` determines how many instruction bytes to NOP (up to
> + opaque size modulo smallest platform instruction - 1 byte x86 and 4 bytes on ARM).
This line is probably also stale now: "...contain the sizes of the
respective functions in bytes..."
>
> * `version` is to be one.
>
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 05e3eb8..6eaa10f 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -101,7 +101,7 @@ static void __init arch_init_ideal_nops(void)
> }
>
> /* Use this to add nops to a buffer, then text_poke the whole buffer. */
> -static void init_or_livepatch add_nops(void *insns, unsigned int len)
> +void init_or_livepatch add_nops(void *insns, unsigned int len)
> {
> while ( len > 0 )
> {
snip
> void arch_livepatch_apply_jmp(struct livepatch_func *func)
> {
> - int32_t val;
> uint8_t *old_ptr;
> -
> - BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque));
> - BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val)));
> + uint8_t insn[sizeof(func->opaque)];
> + unsigned int len;
>
> old_ptr = func->old_addr;
> - memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE);
> + len = livepatch_insn_len(func);
> + if ( !len )
> + return;
> +
> + memcpy(func->opaque, old_ptr, len);
> + if ( func->new_addr )
> + {
> + int32_t val;
> +
> + BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE != (1 + sizeof(val)));
> +
> + insn[0] = 0xe9;
> + val = func->new_addr - func->old_addr - ARCH_PATCH_INSN_SIZE;
> +
> + memcpy(&insn[1], &val, sizeof(val));
> + }
> + else
> + add_nops(&insn, len);
It probably doesn't make a difference, but I'd prefer this to be
"add_nops(insn, len);".
>
> - *old_ptr++ = 0xe9; /* Relative jump */
> - val = func->new_addr - func->old_addr - PATCH_INSN_SIZE;
> - memcpy(old_ptr, &val, sizeof(val));
> + memcpy(old_ptr, insn, len);
> }
>
> void arch_livepatch_revert_jmp(const struct livepatch_func *func)
> {
> - memcpy(func->old_addr, func->opaque, PATCH_INSN_SIZE);
> + memcpy(func->old_addr, func->opaque, livepatch_insn_len(func));
> }
>
--
Ross Lagerwall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 4/6] livepach: Add .livepatch.hooks functions and test-case
2016-09-16 15:29 [PATCH v6] Livepatch fixes and general features for Xen4.8 Konrad Rzeszutek Wilk
` (2 preceding siblings ...)
2016-09-16 15:29 ` [PATCH v6 3/6] livepatch: NOP if func->new_addr is zero Konrad Rzeszutek Wilk
@ 2016-09-16 15:29 ` Konrad Rzeszutek Wilk
2016-09-16 15:29 ` [PATCH v6 5/6] livepatch/tests: Make .livepatch.depends be read-only Konrad Rzeszutek Wilk
2016-09-16 15:29 ` [PATCH v6 6/6] livepatch/tests: Move the .name value to .rodata Konrad Rzeszutek Wilk
5 siblings, 0 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-16 15:29 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall, andrew.cooper3
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap,
Ian Jackson, Tim Deegan, Jan Beulich
From: Ross Lagerwall <ross.lagerwall@citrix.com>
Add hook functions which run during patch apply and patch revert.
Hook functions are used by livepatch payloads to manipulate data
structures during patching, etc.
One use case is the XSA91. As Martin mentions it:
"If we have shadow variables, we also need an unload hook to garbage
collect all the variables introduced by a hotpatch to prevent memory
leaks. Potentially, we also want to pre-reserve memory for static or
existing dynamic objects in the load-hook instead of on the fly.
For testing and debugging, various applications are possible.
In general, the hooks provide flexibility when having to deal with
unforeseen cases, but their application should be rarely required (<
10%)."
Furthermore include a test-case for it.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
v0.4..v0.11: Defered for v4.8
v0.12: s/xsplice/livepatch/
v3: Clarify the comments about spin_debug_enable
Rename one of the hooks to lower-case (Z->z) to guarantee it being
called last.
Learned a lot of about 'const'
v4: Do the actual const of the hooks.
Remove the requirement for the tests-case hooks to be sorted
(they never were to start with).
Fix up the comment about spin_debug_enable so more.
v5: Added Reviewed-by from Andrew.
Dropped the check_fnc hook
Added the check_fnc hook back again as "livepatch: Disallow applying
after an revert" guarantees we won't hit the BUG_ON check.
---
docs/misc/livepatch.markdown | 23 +++++++++++++++++
xen/arch/x86/test/xen_hello_world.c | 34 +++++++++++++++++++++++++
xen/common/livepatch.c | 50 ++++++++++++++++++++++++++++++++++++-
xen/include/xen/livepatch_payload.h | 49 ++++++++++++++++++++++++++++++++++++
4 files changed, 155 insertions(+), 1 deletion(-)
create mode 100644 xen/include/xen/livepatch_payload.h
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index a8e70a8..9e72897 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -343,6 +343,13 @@ When reverting a patch, the hypervisor iterates over each `livepatch_func`
and the core code copies the data from the undo buffer (private internal copy)
to `old_addr`.
+It optionally may contain the address of functions to be called right before
+being applied and after being reverted:
+
+ * `.livepatch.hooks.load` - an array of function pointers.
+ * `.livepatch.hooks.unload` - an array of function pointers.
+
+
### Example of .livepatch.funcs
A simple example of what a payload file can be:
@@ -380,6 +387,22 @@ struct livepatch_func livepatch_hello_world = {
Code must be compiled with -fPIC.
+### .livepatch.hooks.load and .livepatch.hooks.unload
+
+This section contains an array of function pointers to be executed
+before payload is being applied (.livepatch.funcs) or after reverting
+the payload. This is useful to prepare data structures that need to
+be modified patching.
+
+Each entry in this array is eight bytes.
+
+The type definition of the function are as follow:
+
+<pre>
+typedef void (*livepatch_loadcall_t)(void);
+typedef void (*livepatch_unloadcall_t)(void);
+</pre>
+
### .livepatch.depends and .note.gnu.build-id
To support dependencies checking and safe loading (to load the
diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
index 422bdf1..cb5e27a 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -4,14 +4,48 @@
*/
#include "config.h"
+#include <xen/lib.h>
#include <xen/types.h>
#include <xen/version.h>
#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
#include <public/sysctl.h>
static char hello_world_patch_this_fnc[] = "xen_extra_version";
extern const char *xen_hello_world(void);
+static unsigned int cnt;
+
+static void apply_hook(void)
+{
+ printk(KERN_DEBUG "Hook executing.\n");
+}
+
+static void revert_hook(void)
+{
+ printk(KERN_DEBUG "Hook unloaded.\n");
+}
+
+static void hi_func(void)
+{
+ printk(KERN_DEBUG "%s: Hi! (called %u times)\n", __func__, ++cnt);
+};
+
+static void check_fnc(void)
+{
+ printk(KERN_DEBUG "%s: Hi func called %u times\n", __func__, cnt);
+ BUG_ON(cnt == 0 || cnt > 2);
+}
+
+LIVEPATCH_LOAD_HOOK(apply_hook);
+LIVEPATCH_UNLOAD_HOOK(revert_hook);
+
+/* Imbalance here. Two load and three unload. */
+
+LIVEPATCH_LOAD_HOOK(hi_func);
+LIVEPATCH_UNLOAD_HOOK(hi_func);
+
+LIVEPATCH_UNLOAD_HOOK(check_fnc);
struct livepatch_func __section(".livepatch.funcs") livepatch_xen_hello_world = {
.version = LIVEPATCH_PAYLOAD_VERSION,
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 218b389..3729409 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -23,6 +23,7 @@
#include <xen/wait.h>
#include <xen/livepatch_elf.h>
#include <xen/livepatch.h>
+#include <xen/livepatch_payload.h>
#include <asm/event.h>
@@ -72,7 +73,11 @@ struct payload {
unsigned int nsyms; /* Nr of entries in .strtab and symbols. */
struct livepatch_build_id id; /* ELFNOTE_DESC(.note.gnu.build-id) of the payload. */
struct livepatch_build_id dep; /* ELFNOTE_DESC(.livepatch.depends). */
- char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
+ livepatch_loadcall_t *const *load_funcs; /* The array of funcs to call after */
+ livepatch_unloadcall_t *const *unload_funcs;/* load and unload of the payload. */
+ unsigned int n_load_funcs; /* Nr of the funcs to load and execute. */
+ unsigned int n_unload_funcs; /* Nr of funcs to call durung unload. */
+ char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
};
/* Defines an outstanding patching action. */
@@ -537,6 +542,25 @@ static int prepare_payload(struct payload *payload,
return rc;
}
+ sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.load");
+ if ( sec )
+ {
+ if ( sec->sec->sh_size % sizeof(*payload->load_funcs) )
+ return -EINVAL;
+
+ payload->load_funcs = sec->load_addr;
+ payload->n_load_funcs = sec->sec->sh_size / sizeof(*payload->load_funcs);
+ }
+
+ sec = livepatch_elf_sec_by_name(elf, ".livepatch.hooks.unload");
+ if ( sec )
+ {
+ if ( sec->sec->sh_size % sizeof(*payload->unload_funcs) )
+ return -EINVAL;
+
+ payload->unload_funcs = sec->load_addr;
+ payload->n_unload_funcs = sec->sec->sh_size / sizeof(*payload->unload_funcs);
+ }
sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
if ( sec )
{
@@ -1027,6 +1051,18 @@ static int apply_payload(struct payload *data)
return rc;
}
+ /*
+ * Since we are running with IRQs disabled and the hooks may call common
+ * code - which expects certain spinlocks to run with IRQs enabled - we
+ * temporarily disable the spin locks IRQ state checks.
+ */
+ spin_debug_disable();
+ for ( i = 0; i < data->n_load_funcs; i++ )
+ data->load_funcs[i]();
+ spin_debug_enable();
+
+ ASSERT(!local_irq_is_enabled());
+
for ( i = 0; i < data->nfuncs; i++ )
arch_livepatch_apply_jmp(&data->funcs[i]);
@@ -1059,6 +1095,18 @@ static int revert_payload(struct payload *data)
for ( i = 0; i < data->nfuncs; i++ )
arch_livepatch_revert_jmp(&data->funcs[i]);
+ /*
+ * Since we are running with IRQs disabled and the hooks may call common
+ * code - which expects certain spinlocks to run with IRQs enabled - we
+ * temporarily disable the spin locks IRQ state checks.
+ */
+ spin_debug_disable();
+ for ( i = 0; i < data->n_unload_funcs; i++ )
+ data->unload_funcs[i]();
+ spin_debug_enable();
+
+ ASSERT(!local_irq_is_enabled());
+
arch_livepatch_revive();
/*
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
new file mode 100644
index 0000000..8f38cc2
--- /dev/null
+++ b/xen/include/xen/livepatch_payload.h
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2016 Citrix Systems R&D Ltd.
+ */
+
+#ifndef __XEN_LIVEPATCH_PAYLOAD_H__
+#define __XEN_LIVEPATCH_PAYLOAD_H__
+
+/*
+ * The following definitions are to be used in patches. They are taken
+ * from kpatch.
+ */
+typedef void livepatch_loadcall_t(void);
+typedef void livepatch_unloadcall_t(void);
+
+/*
+ * LIVEPATCH_LOAD_HOOK macro
+ *
+ * Declares a function pointer to be allocated in a new
+ * .livepatch.hook.load section. This livepatch_load_data symbol is later
+ * stripped by create-diff-object so that it can be declared in multiple
+ * objects that are later linked together, avoiding global symbol
+ * collision. Since multiple hooks can be registered, the
+ * .livepatch.hook.load section is a table of functions that will be
+ * executed in series by the livepatch infrastructure at patch load time.
+ */
+#define LIVEPATCH_LOAD_HOOK(_fn) \
+ livepatch_loadcall_t *__attribute__((weak)) \
+ const livepatch_load_data_##_fn __section(".livepatch.hooks.load") = _fn;
+
+/*
+ * LIVEPATCH_UNLOAD_HOOK macro
+ *
+ * Same as LOAD hook with s/load/unload/
+ */
+#define LIVEPATCH_UNLOAD_HOOK(_fn) \
+ livepatch_unloadcall_t *__attribute__((weak)) \
+ const livepatch_unload_data_##_fn __section(".livepatch.hooks.unload") = _fn;
+
+#endif /* __XEN_LIVEPATCH_PAYLOAD_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.5.5
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v6 5/6] livepatch/tests: Make .livepatch.depends be read-only
2016-09-16 15:29 [PATCH v6] Livepatch fixes and general features for Xen4.8 Konrad Rzeszutek Wilk
` (3 preceding siblings ...)
2016-09-16 15:29 ` [PATCH v6 4/6] livepach: Add .livepatch.hooks functions and test-case Konrad Rzeszutek Wilk
@ 2016-09-16 15:29 ` Konrad Rzeszutek Wilk
2016-09-21 12:47 ` Ross Lagerwall
2016-09-16 15:29 ` [PATCH v6 6/6] livepatch/tests: Move the .name value to .rodata Konrad Rzeszutek Wilk
5 siblings, 1 reply; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-16 15:29 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall, andrew.cooper3
Cc: Jan Beulich, Konrad Rzeszutek Wilk
As currently during the injection of the build-id it ends up
being marked as AW. We want it to be read-only.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
v6: New submission.
---
xen/arch/x86/test/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/test/Makefile b/xen/arch/x86/test/Makefile
index 23dff1d..48ff843 100644
--- a/xen/arch/x86/test/Makefile
+++ b/xen/arch/x86/test/Makefile
@@ -55,7 +55,7 @@ $(LIVEPATCH): xen_hello_world_func.o xen_hello_world.o note.o
note.o:
$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(BASEDIR)/xen-syms $@.bin
$(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \
- --rename-section=.data=.livepatch.depends -S $@.bin $@
+ --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@
rm -f $@.bin
#
@@ -66,7 +66,7 @@ note.o:
hello_world_note.o: $(LIVEPATCH)
$(OBJCOPY) -O binary --only-section=.note.gnu.build-id $(LIVEPATCH) $@.bin
$(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 \
- --rename-section=.data=.livepatch.depends -S $@.bin $@
+ --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S $@.bin $@
rm -f $@.bin
xen_bye_world.o: config.h
--
2.5.5
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v6 6/6] livepatch/tests: Move the .name value to .rodata
2016-09-16 15:29 [PATCH v6] Livepatch fixes and general features for Xen4.8 Konrad Rzeszutek Wilk
` (4 preceding siblings ...)
2016-09-16 15:29 ` [PATCH v6 5/6] livepatch/tests: Make .livepatch.depends be read-only Konrad Rzeszutek Wilk
@ 2016-09-16 15:29 ` Konrad Rzeszutek Wilk
2016-09-19 9:06 ` Jan Beulich
2016-09-21 12:49 ` Ross Lagerwall
5 siblings, 2 replies; 21+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-09-16 15:29 UTC (permalink / raw)
To: xen-devel, konrad, ross.lagerwall, andrew.cooper3
Cc: Jan Beulich, Konrad Rzeszutek Wilk
Right now the contents of 'name' are all located in
the .data section. We want them in the .rodata section
so change the type to have const on them.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
v3: First submission (as part of the ARM 32 and 64 support of Livepatch)
v6: Now moved to the Common Livepatch fixes for 4.8
---
xen/arch/x86/test/xen_bye_world.c | 2 +-
xen/arch/x86/test/xen_hello_world.c | 2 +-
xen/arch/x86/test/xen_replace_world.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/test/xen_bye_world.c b/xen/arch/x86/test/xen_bye_world.c
index b75e0b1..2700f0e 100644
--- a/xen/arch/x86/test/xen_bye_world.c
+++ b/xen/arch/x86/test/xen_bye_world.c
@@ -11,7 +11,7 @@
#include <public/sysctl.h>
-static char bye_world_patch_this_fnc[] = "xen_extra_version";
+static const char bye_world_patch_this_fnc[] = "xen_extra_version";
extern const char *xen_bye_world(void);
struct livepatch_func __section(".livepatch.funcs") livepatch_xen_bye_world = {
diff --git a/xen/arch/x86/test/xen_hello_world.c b/xen/arch/x86/test/xen_hello_world.c
index cb5e27a..02f3f85 100644
--- a/xen/arch/x86/test/xen_hello_world.c
+++ b/xen/arch/x86/test/xen_hello_world.c
@@ -12,7 +12,7 @@
#include <public/sysctl.h>
-static char hello_world_patch_this_fnc[] = "xen_extra_version";
+static const char hello_world_patch_this_fnc[] = "xen_extra_version";
extern const char *xen_hello_world(void);
static unsigned int cnt;
diff --git a/xen/arch/x86/test/xen_replace_world.c b/xen/arch/x86/test/xen_replace_world.c
index a2a221a..78a8f52 100644
--- a/xen/arch/x86/test/xen_replace_world.c
+++ b/xen/arch/x86/test/xen_replace_world.c
@@ -10,7 +10,7 @@
#include <public/sysctl.h>
-static char xen_replace_world_name[] = "xen_extra_version";
+static const char xen_replace_world_name[] = "xen_extra_version";
extern const char *xen_replace_world(void);
struct livepatch_func __section(".livepatch.funcs") livepatch_xen_replace_world = {
--
2.5.5
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 21+ messages in thread