| Message ID | 20160811012831.GB11611@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/11/2016 02:28 AM, Konrad Rzeszutek Wilk wrote: > Hey Ross, > > I am running in a symbol dependency issue that I am not exactly > sure how to solve. > > I have an payload that introduces a new function (xen_foobar) which > will patch over xen_extra_version(). > snip > > As livepatch_symbols_lookup_by_name only looks for symbols that > have the ->new_symbol set. And xen_foobar does not. So the loading is > aborted. > > Which makes sense - we don't want to match the symbols as they haven't > really been "finally loaded" in. > > But what if the xen_foobar is applied. In that case we should > change the xen_foobar to be new_symbol=1? I think you're confused about the purpose of new_symbol. The purpose is to ensure that you link against the correct symbol from the base hypervisor or the live patch that first introduced it. So, new_symbol=0 is when a symbol overrides an existing symbol. new_symbol=1 is set when a symbol is new introduced in a live patch. Since all the linking happens during load and not apply, it is perfectly OK to link against a symbol that hasn't been applied -- the dependencies are there to ensure that you can't apply a patch which links against unapplied symbols. The assumption is that when overriding an existing symbol, the symbol in the payload has the same name as the one it is overriding. You're having issues above because you're breaking this assumption. > > This following patch does that, but I am wondering if there is a better > way? The patch is misusing new_symbol for something completely different from how it was intended so I hope there is a better way :-P > > P.S. > The reason for this is that I am trying to implement NOP patching. > And to have some regression testing of this I wrote an function > (xen_foobar) which calls two functions: foo and bar - and their output is what > the call to XENVER_extra_version will show (b/c we patch over > xen_extra_version()). > > Then there is another payload - which will want to NOP the call to > the 'bar' function inside xen_foobar. And for that I need to be able to > lookup the symbol of xen_foobar. This is quite a different use case from what currently exists. Currently we're only ever interested in writing over the start of the function pointed to by a symbol from the base hypervisor or first instance of a symbol in a live patch (aka new_symbol=1). Now you need to be able to lookup and write over an arbitrary symbol -- how do you choose between the n different loaded versions of the same symbol? I must admit to not seeing the point in NOP patching. It just seems to be a special case of arbitrary data patching that could be more easily achieved using other means. Let's have a discussion about this and the symbol issues here at the Xen Summit in a couple of weeks time.
On Thu, Aug 11, 2016 at 09:11:10AM +0100, Ross Lagerwall wrote: > On 08/11/2016 02:28 AM, Konrad Rzeszutek Wilk wrote: > > Hey Ross, > > > > I am running in a symbol dependency issue that I am not exactly > > sure how to solve. > > > > I have an payload that introduces a new function (xen_foobar) which > > will patch over xen_extra_version(). > > > snip > > > > As livepatch_symbols_lookup_by_name only looks for symbols that > > have the ->new_symbol set. And xen_foobar does not. So the loading is > > aborted. > > > > Which makes sense - we don't want to match the symbols as they haven't > > really been "finally loaded" in. > > > > But what if the xen_foobar is applied. In that case we should > > change the xen_foobar to be new_symbol=1? > > I think you're confused about the purpose of new_symbol. The purpose is to > ensure that you link against the correct symbol from the base hypervisor or > the live patch that first introduced it. So, new_symbol=0 is when a symbol > overrides an existing symbol. new_symbol=1 is set when a symbol is new But it does not (overrides the existing symbol). The patch (xen_foobar) introduces a new function called xen_foobar which is patching xen_extra_version. That is: static char foobar_patch_this_fnc[] = "xen_extra_version"; struct livepatch_func __section(".livepatch.funcs") livepatch_xen_foobar = { .version = LIVEPATCH_PAYLOAD_VERSION, .name = foobar_patch_this_fnc, .new_addr = xen_foobar, .old_addr = xen_extra_version, .new_size = NEW_CODE_SZ, .old_size = OLD_CODE_SZ, }; > introduced in a live patch. And this loop: for ( j = 0; j < payload->nfuncs; j++ ) { if ( symtab[i].value == (unsigned long)payload->funcs[j].new_addr ) { found = 1; break; } } Will force new_symbol=0 for xen_foobar. > > Since all the linking happens during load and not apply, it is perfectly OK > to link against a symbol that hasn't been applied -- the dependencies are > there to ensure that you can't apply a patch which links against unapplied > symbols. > > The assumption is that when overriding an existing symbol, the symbol in the > payload has the same name as the one it is overriding. You're having issues > above because you're breaking this assumption. Yes :-) > > > > > This following patch does that, but I am wondering if there is a better > > way? > > The patch is misusing new_symbol for something completely different from how > it was intended so I hope there is a better way :-P Well for my use-case I think I can just s/xen_foobar/xen_extra_version/ and we should be OK. > Let's have a discussion about this and the symbol issues here at the Xen > Summit in a couple of weeks time. /me nods. > > -- > Ross Lagerwall
On Fri, Aug 12, 2016 at 09:51:39AM -0400, Konrad Rzeszutek Wilk wrote: > On Thu, Aug 11, 2016 at 09:11:10AM +0100, Ross Lagerwall wrote: > > On 08/11/2016 02:28 AM, Konrad Rzeszutek Wilk wrote: > > > Hey Ross, > > > > > > I am running in a symbol dependency issue that I am not exactly > > > sure how to solve. > > > > > > I have an payload that introduces a new function (xen_foobar) which > > > will patch over xen_extra_version(). > > > > > snip > > > > > > As livepatch_symbols_lookup_by_name only looks for symbols that > > > have the ->new_symbol set. And xen_foobar does not. So the loading is > > > aborted. > > > > > > Which makes sense - we don't want to match the symbols as they haven't > > > really been "finally loaded" in. > > > > > > But what if the xen_foobar is applied. In that case we should > > > change the xen_foobar to be new_symbol=1? > > > > I think you're confused about the purpose of new_symbol. The purpose is to > > ensure that you link against the correct symbol from the base hypervisor or > > the live patch that first introduced it. So, new_symbol=0 is when a symbol > > overrides an existing symbol. new_symbol=1 is set when a symbol is new > > But it does not (overrides the existing symbol). > > The patch (xen_foobar) introduces a new function called xen_foobar > which is patching xen_extra_version. > > That is: > > static char foobar_patch_this_fnc[] = "xen_extra_version"; > > struct livepatch_func __section(".livepatch.funcs") livepatch_xen_foobar = { > .version = LIVEPATCH_PAYLOAD_VERSION, > .name = foobar_patch_this_fnc, > .new_addr = xen_foobar, > .old_addr = xen_extra_version, > .new_size = NEW_CODE_SZ, > .old_size = OLD_CODE_SZ, > }; > > > introduced in a live patch. > > And this loop: > > for ( j = 0; j < payload->nfuncs; j++ ) > { > if ( symtab[i].value == (unsigned long)payload->funcs[j].new_addr ) > { > found = 1; > break; > } > } > > Will force new_symbol=0 for xen_foobar. > > > > > Since all the linking happens during load and not apply, it is perfectly OK > > to link against a symbol that hasn't been applied -- the dependencies are > > there to ensure that you can't apply a patch which links against unapplied > > symbols. > > > > The assumption is that when overriding an existing symbol, the symbol in the > > payload has the same name as the one it is overriding. You're having issues > > above because you're breaking this assumption. > > Yes :-) > > > > > > > > > This following patch does that, but I am wondering if there is a better > > > way? > > > > The patch is misusing new_symbol for something completely different from how > > it was intended so I hope there is a better way :-P > > Well for my use-case I think I can just s/xen_foobar/xen_extra_version/ and we > should be OK. Ah no. It does work for xen_foo (so it replaces xen_extra_version with its own 'xen_extra_version'. But when I introduce xen_foobar_nop and it tries to look for 'xen_extra_version' it picks the hypervisor one (which has been patched over) instead of the livepatched version. This may require some extra lookup in the applied_list for the symbols before consulting and trying to match up the symbols in the built-in list.
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index 1023fab..ad2011f 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -44,11 +44,11 @@ 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 > sizeof(func->u.s.opaque)); BUILD_BUG_ON(PATCH_INSN_SIZE != (1 + sizeof(val))); old_ptr = func->old_addr; - memcpy(func->opaque, old_ptr, PATCH_INSN_SIZE); + memcpy(func->u.s.opaque, old_ptr, PATCH_INSN_SIZE); *old_ptr++ = 0xe9; /* Relative jump */ val = func->new_addr - func->old_addr - PATCH_INSN_SIZE; @@ -57,7 +57,7 @@ void arch_livepatch_apply_jmp(struct livepatch_func *func) void arch_livepatch_revert_jmp(const struct livepatch_func *func) { - memcpy(func->old_addr, func->opaque, PATCH_INSN_SIZE); + memcpy(func->old_addr, func->u.s.opaque, PATCH_INSN_SIZE); } /* Serialise the CPU pipeline. */ diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index 5da28a3..4c11286 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -496,6 +496,8 @@ static int prepare_payload(struct payload *payload, if ( rc ) return rc; + f->u.s.idx = -1; + /* Lookup function's old address if not already resolved. */ if ( !f->old_addr ) { @@ -742,6 +744,7 @@ static int build_symbol_table(struct payload *payload, if ( symtab[i].value == (unsigned long)payload->funcs[j].new_addr ) { found = 1; + payload->funcs[j].u.s.idx = i; break; } } @@ -1000,8 +1003,21 @@ static int apply_payload(struct payload *data) arch_livepatch_quiesce(); for ( i = 0; i < data->nfuncs; i++ ) + { + int idx; + + idx = data->funcs[i].u.s.idx; + if ( idx >= 0 ) + { + struct livepatch_symbol *s; + + s = (struct livepatch_symbol *)&data->symtab[idx]; + s->new_symbol = 1; + } + arch_livepatch_apply_jmp(&data->funcs[i]); + } arch_livepatch_revive(); /* @@ -1023,7 +1039,19 @@ static int revert_payload(struct payload *data) arch_livepatch_quiesce(); for ( i = 0; i < data->nfuncs; i++ ) + { + int idx; + + idx = data->funcs[i].u.s.idx; + if ( idx >= 0 ) + { + struct livepatch_symbol *s; + + s = (struct livepatch_symbol *)&data->symtab[idx]; + s->new_symbol = 0; + } arch_livepatch_revert_jmp(&data->funcs[i]); + } arch_livepatch_revive(); diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h index 8197c14..162cd0f 100644 --- a/xen/include/public/sysctl.h +++ b/xen/include/public/sysctl.h @@ -884,7 +884,13 @@ struct livepatch_func { uint32_t new_size; uint32_t old_size; uint8_t version; /* MUST be LIVEPATCH_PAYLOAD_VERSION. */ - uint8_t opaque[31]; + union { + uint8_t opaque[31]; + struct { + int32_t idx; + uint8_t opaque[27]; + } s; + } u; }; typedef struct livepatch_func livepatch_func_t; #endif
Hey Ross, I am running in a symbol dependency issue that I am not exactly sure how to solve. I have an payload that introduces a new function (xen_foobar) which will patch over xen_extra_version(). The loading says: (XEN) livepatch_elf.c:321: livepatch: xen_foobar: Symbol resolved: xen_foobar => 0xffff82d080409038 (.text) (XEN) livepatch.c:879: livepatch: xen_foobar: overriding symbol xen_foobar The later reason is b/c of: { bool_t found = 0; for ( j = 0; j < payload->nfuncs; j++ ) { if ( symtab[i].value == (unsigned long)payload->funcs[j].new_addr ) { found = 1; break; } } if ( !found ) { .... symtab[i].new_symbol = 1; } else { /* new_symbol is not set. */ ... Which makes sense - we do not want to introduce this symbol as a new one - as it has not been patched in. And there may be another payload which also has the same exact symbol to patch in. And either one may have the same exact .livepatch.depends (against the hypervisor) so either one could be loaded. Now imagine I have another payload which introduces xen_foobar2 which is suppose to patch over xen_foobar (bear with me, we could as well just do replace, but there is a good reason for this). Meaning the .livepatch.funcs has: .name = "xen_foobar", .new_addr = xen_foobar2, .old_addr = 0, (and the .livepatch.depends build-id is against one of the xen_foobar payloads). It gets loaded, and livepatch_elf_resolve_symbols is OK. But we choke in prepare_payload b/c of: f->old_addr = (void *)symbols_lookup_by_name(s); if ( !f->old_addr ) { f->old_addr = (void *)livepatch_symbols_lookup_by_name(s); if ( !f->old_addr ) { dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not resolve old address of %s\n", elf->name, f->name); return -ENOENT; As livepatch_symbols_lookup_by_name only looks for symbols that have the ->new_symbol set. And xen_foobar does not. So the loading is aborted. Which makes sense - we don't want to match the symbols as they haven't really been "finally loaded" in. But what if the xen_foobar is applied. In that case we should change the xen_foobar to be new_symbol=1? This following patch does that, but I am wondering if there is a better way? P.S. The reason for this is that I am trying to implement NOP patching. And to have some regression testing of this I wrote an function (xen_foobar) which calls two functions: foo and bar - and their output is what the call to XENVER_extra_version will show (b/c we patch over xen_extra_version()). Then there is another payload - which will want to NOP the call to the 'bar' function inside xen_foobar. And for that I need to be able to lookup the symbol of xen_foobar.