| Message ID | 1472005332-32207-8-git-send-email-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 24.08.16 at 04:22, <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' 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. > But on x86 we could NOP instructions that are much > shorter or longer (up to 15 bytes). And we could NOP multiple instructions in one go, i.e. the new boundary you introduce is still arbitrary. Jan
On Wed, Aug 24, 2016 at 03:13:18AM -0600, Jan Beulich wrote: > >>> On 24.08.16 at 04:22, <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' 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. > > But on x86 we could NOP instructions that are much > > shorter or longer (up to 15 bytes). > > And we could NOP multiple instructions in one go, i.e. the new > boundary you introduce is still arbitrary. True. I am limited by the 'struct livepatch_func' -> opaque[31] size. I figured an OK limit would be up to a maximum platform instruction size. That is what the design document mentions too: " then `new_size` determines how many instruction bytes to NOP (up to platform limitations)." But in reality it could be up to 31 bytes - unless I rework the 'opaque' to have a void pointer to some bigger size structure - but if I do that then this gets complicated. Keep in mind that the person writting the payload can have multiple 'struct livepatch_func' - so you could NOP a stream of say 30 bytes using two 'struct livepatch_func'. If we allow the NOP to be up to the size of the 'opaque' size, then you could NOP a stream of instructions up to 62 bytes with two 'struct livepatch_func'. or such. Thought to keep this from blowing up on ARM I would say it has to be up to modulo 4. Do you have a preference on this? > > Jan >
>>> On 06.09.16 at 22:05, <konrad.wilk@oracle.com> wrote: > On Wed, Aug 24, 2016 at 03:13:18AM -0600, Jan Beulich wrote: >> >>> On 24.08.16 at 04:22, <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' 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. >> > But on x86 we could NOP instructions that are much >> > shorter or longer (up to 15 bytes). >> >> And we could NOP multiple instructions in one go, i.e. the new >> boundary you introduce is still arbitrary. > > True. > > I am limited by the 'struct livepatch_func' -> opaque[31] size. > > I figured an OK limit would be up to a maximum platform instruction size. > That is what the design document mentions too: > " then `new_size` determines how many instruction bytes to NOP (up to > platform limitations)." > > But in reality it could be up to 31 bytes - unless I rework the 'opaque' > to have a void pointer to some bigger size structure - but if I do that > then this gets complicated. > > Keep in mind that the person writting the payload can have multiple > 'struct livepatch_func' - so you could NOP a stream of say 30 bytes > using two 'struct livepatch_func'. > > If we allow the NOP to be up to the size of the 'opaque' size, then > you could NOP a stream of instructions up to 62 bytes with two > 'struct livepatch_func'. or such. Thought to keep this from blowing > up on ARM I would say it has to be up to modulo 4. > > Do you have a preference on this? Well, if it keeps the code meaningfully more simple with a restriction on the size, then I'd prefer fully leveraging opaque's size. Even better of course would be to not place such a restriction. Of course an option is to leave the limit in for now, but track a work item for it to get eliminated. Jan
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown index 7e82047..ad0df26 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 + platform limitations). * `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 1023fab..952e897 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -14,6 +14,7 @@ #include <asm/nmi.h> #define PATCH_INSN_SIZE 5 +#define MAX_INSN_SIZE 15 void arch_livepatch_quiesce(void) { @@ -29,8 +30,8 @@ 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 instruction size. */ + if ( !func->new_addr && func->new_size > MAX_INSN_SIZE ) return -EOPNOTSUPP; if ( func->old_size < PATCH_INSN_SIZE ) @@ -39,25 +40,48 @@ int arch_livepatch_verify_func(const struct livepatch_func *func) return 0; } +static size_t get_len(const struct livepatch_func *func) +{ + if ( !func->new_addr ) + return func->new_size; + + return PATCH_INSN_SIZE; +} + void arch_livepatch_apply_jmp(struct livepatch_func *func) { - int32_t val; uint8_t *old_ptr; + uint8_t insn[MAX_INSN_SIZE]; + size_t len; - BUILD_BUG_ON(PATCH_INSN_SIZE > sizeof(func->opaque)); - BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val))); + BUILD_BUG_ON(MAX_INSN_SIZE > sizeof(func->opaque)); old_ptr = func->old_addr; - memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE); + len = get_len(func); + if ( !len ) + return; + + memcpy(func->opaque, old_ptr, len); + if ( func->new_addr ) + { + 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, 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, get_len(func)); } /* Serialise the CPU pipeline. */ diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index c234424..dd24a07 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -566,7 +566,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 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' 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. But on x86 we could NOP instructions that are much shorter or longer (up to 15 bytes). 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. If `name` is provided with the 'symbol+0x<offset' format we figure out the EIP and then NOP 'new_size' amount of bytes at the 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 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. --- docs/misc/livepatch.markdown | 7 +++++-- xen/arch/x86/alternative.c | 2 +- xen/arch/x86/livepatch.c | 44 ++++++++++++++++++++++++++++++--------- xen/common/livepatch.c | 3 ++- xen/include/asm-x86/alternative.h | 1 + 5 files changed, 43 insertions(+), 14 deletions(-)