| Message ID | 1471211546-2235-8-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote: > The NOP functionality will NOP any of the code at > the 'old_addr' or at 'name' if the 'new_addr' and 'new_size' > are both zero. The purpose of this is to NOP out calls, such as: > > e9 <4-bytes-offset> Except that E9 is JMP; CALL is E8. > (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 > 5 instructions (on x86) at that location. > > If `name` is provided with the symbol+0x<offset/<len> > we make sure that <len> is 5 (on x86) and upon retrieving the > EIP based on `name` will NOP that location. So why does this need to be restricted to 5-byte (on x86) code blocks? I.e. what's wrong with NOP-ing out other code. > @@ -46,18 +42,27 @@ 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; > + uint8_t insn[PATCH_INSN_SIZE]; > > BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque)); > - BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val))); > > old_ptr = func->old_addr; > memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE); > > - *old_ptr++ = 0xe9; /* Relative jump */ > - val = func->new_addr - func->old_addr - PATCH_INSN_SIZE; > - memcpy(old_ptr, &val, sizeof(val)); > + if ( func->new_size ) > + { > + int32_t val; > + > + BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val))); > + > + insn[0] = 0xe9; > + val = func->new_addr - func->old_addr - PATCH_INSN_SIZE; > + memcpy(&insn[1], &val, sizeof(val)); > + } else Style. > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -561,11 +561,15 @@ static int prepare_payload(struct payload *payload, > return -EOPNOTSUPP; > } > > - if ( !f->new_addr || !f->new_size ) > + /* If both are zero then we are NOPing. */ > + if ( (!f->new_addr || !f->new_size) ) Comment and condition contradict one another. And you're adding unnecessary parentheses. Jan
On Mon, Aug 15, 2016 at 04:59:52AM -0600, Jan Beulich wrote: > >>> On 14.08.16 at 23:52, <konrad.wilk@oracle.com> wrote: > > The NOP functionality will NOP any of the code at > > the 'old_addr' or at 'name' if the 'new_addr' and 'new_size' > > are both zero. The purpose of this is to NOP out calls, such as: > > > > e9 <4-bytes-offset> > > Except that E9 is JMP; CALL is E8. > > > (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 > > 5 instructions (on x86) at that location. > > > > If `name` is provided with the symbol+0x<offset/<len> > > we make sure that <len> is 5 (on x86) and upon retrieving the > > EIP based on `name` will NOP that location. > > So why does this need to be restricted to 5-byte (on x86) code > blocks? I.e. what's wrong with NOP-ing out other code. It can most certainly nop variable sizes. I will have to update the design to make it clear that if 'new_addr' is zero then we will NOP (and the .new_size will determine the amount of NOPs to sprinkle). Let me do that along with your comments. Thanks! > > > @@ -46,18 +42,27 @@ 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; > > + uint8_t insn[PATCH_INSN_SIZE]; > > > > BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque)); > > - BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val))); > > > > old_ptr = func->old_addr; > > memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE); > > > > - *old_ptr++ = 0xe9; /* Relative jump */ > > - val = func->new_addr - func->old_addr - PATCH_INSN_SIZE; > > - memcpy(old_ptr, &val, sizeof(val)); > > + if ( func->new_size ) > > + { > > + int32_t val; > > + > > + BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val))); > > + > > + insn[0] = 0xe9; > > + val = func->new_addr - func->old_addr - PATCH_INSN_SIZE; > > + memcpy(&insn[1], &val, sizeof(val)); > > + } else > > Style. > > > --- a/xen/common/livepatch.c > > +++ b/xen/common/livepatch.c > > @@ -561,11 +561,15 @@ static int prepare_payload(struct payload *payload, > > return -EOPNOTSUPP; > > } > > > > - if ( !f->new_addr || !f->new_size ) > > + /* If both are zero then we are NOPing. */ > > + if ( (!f->new_addr || !f->new_size) ) > > Comment and condition contradict one another. And you're adding > unnecessary parentheses. > > Jan >
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown index f1a5147..590757d 100644 --- a/docs/misc/livepatch.markdown +++ b/docs/misc/livepatch.markdown @@ -320,10 +320,12 @@ 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** 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_size` is _zero_ + the code at location of `old_addr` (or `name` when resolved) will be NOPed out. * `version` is to be one. diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c index be40b13..fd8528e 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 df64b00..cabd0c1 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -34,10 +34,6 @@ int arch_verify_insn_length(unsigned long len) int arch_livepatch_verify_func(const struct livepatch_func *func) { - /* No NOP patching yet. */ - if ( !func->new_size ) - return -EOPNOTSUPP; - if ( func->old_size < PATCH_INSN_SIZE ) return -EINVAL; @@ -46,18 +42,27 @@ 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; + uint8_t insn[PATCH_INSN_SIZE]; BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque)); - BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val))); old_ptr = func->old_addr; memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE); - *old_ptr++ = 0xe9; /* Relative jump */ - val = func->new_addr - func->old_addr - PATCH_INSN_SIZE; - memcpy(old_ptr, &val, sizeof(val)); + if ( func->new_size ) + { + int32_t val; + + BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val))); + + insn[0] = 0xe9; + val = func->new_addr - func->old_addr - PATCH_INSN_SIZE; + memcpy(&insn[1], &val, sizeof(val)); + } else + add_nops(&insn, PATCH_INSN_SIZE); + + memcpy(old_ptr, insn, PATCH_INSN_SIZE); } void arch_livepatch_revert_jmp(const struct livepatch_func *func) diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index e752949..6f82a9e 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -561,11 +561,15 @@ static int prepare_payload(struct payload *payload, return -EOPNOTSUPP; } - if ( !f->new_addr || !f->new_size ) + /* If both are zero then we are NOPing. */ + if ( (!f->new_addr || !f->new_size) ) { - dprintk(XENLOG_ERR, LIVEPATCH "%s: Address or size fields are zero!\n", - elf->name); - return -EINVAL; + if ( f->new_addr || f->new_size ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: Address or size fields are zero!\n", + elf->name); + return -EINVAL; + } } rc = arch_livepatch_verify_func(f); diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h index bce959f..acaeded 100644 --- a/xen/include/asm-x86/alternative.h +++ b/xen/include/asm-x86/alternative.h @@ -23,6 +23,7 @@ struct alt_instr { u8 replacementlen; /* length of new instruction, <= instrlen */ }; +extern void add_nops(void *insns, unsigned int len); /* Similar to apply_alternatives except it can be run with IRQs enabled. */ extern void apply_alternatives_nocheck(struct alt_instr *start, struct alt_instr *end);
The NOP functionality will NOP any of the code at the 'old_addr' or at 'name' if the 'new_addr' and 'new_size' are both zero. The purpose of this is to NOP out calls, such as: e9 <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 5 instructions (on x86) at that location. If `name` is provided with the symbol+0x<offset/<len> we make sure that <len> is 5 (on x86) and upon retrieving the EIP based on `name` will NOP that location. 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). 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 --- docs/misc/livepatch.markdown | 6 ++++-- xen/arch/x86/alternative.c | 2 +- xen/arch/x86/livepatch.c | 23 ++++++++++++++--------- xen/common/livepatch.c | 12 ++++++++---- xen/include/asm-x86/alternative.h | 1 + 5 files changed, 28 insertions(+), 16 deletions(-)