xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* Livepatch, symbol resolutions between two livepatchs (new_symbol=0)
@ 2016-08-11  1:28 Konrad Rzeszutek Wilk
  2016-08-11  8:11 ` Ross Lagerwall
  0 siblings, 1 reply; 4+ messages in thread
From: Konrad Rzeszutek Wilk @ 2016-08-11  1:28 UTC (permalink / raw)
  To: ross.lagerwall, xen-devel

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.

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

^ permalink raw reply related	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-08-12 15:46 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-11  1:28 Livepatch, symbol resolutions between two livepatchs (new_symbol=0) Konrad Rzeszutek Wilk
2016-08-11  8:11 ` Ross Lagerwall
2016-08-12 13:51   ` Konrad Rzeszutek Wilk
2016-08-12 15:45     ` Konrad Rzeszutek Wilk

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).