[PATCH v4] Livepatch fixes and features for v4.8.

[PATCH v4] Livepatch fixes and features for v4.8.

[Konrad Rzeszutek Wilk]

Hey!

Since v3: [https://lists.xen.org/archives/html/xen-devel/2016-08/msg01825.html]
 - Acked on reviews
v2, v1:
 - Left over fixes and features that didn't get quite done in 4.7

Included are:
 - Bug-fixes
 - Parsing of symbol names encoded as: symbol+0x<offset>
 - NOP patching
 - Generating an symbol map file with Xen's unique symbols (file#symbol)
   so tools generating livepatch payloads can verify the right names.
 - Hooks

The 'hooks' are the most controversial part of this and are left as the
last patch :-)

The legend is:
 r - Reviewed

   [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted
rr [PATCH v4 2/9] livepatch: Deal with payloads without any .text
rr [PATCH v4 3/9] version/livepatch: Move xen_build_id_check to
   [PATCH v4 4/9] version: Print build-id at bootup.
   [PATCH v4 5/9] livepatch: Move code from prepare_payload to own
   [PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>
   [PATCH v4 7/9] livepatch: NOP if func->new_[addr] is zero.
   [PATCH v4 8/9] symbols: Generate an xen-sym.map
   [PATCH v4 9/9] livepach: Add .livepatch.hooks functions and test-case

so one can ignore #2 and #3.

Thanks!

The git tree `

 git://enbits.xen.org/people/konradwilk/xen.git livepatch.v4.8.v4  

contains all the following patches (and more):

 .gitignore                          |   1 +
 docs/misc/livepatch.markdown        |  32 +++++-
 xen/Makefile                        |   6 +-
 xen/arch/arm/Makefile               |   3 +
 xen/arch/x86/Makefile               |   7 +-
 xen/arch/x86/alternative.c          |   2 +-
 xen/arch/x86/livepatch.c            |  44 ++++++--
 xen/arch/x86/test/xen_hello_world.c |  34 ++++++
 xen/common/livepatch.c              | 204 ++++++++++++++++++++++++++++--------
 xen/common/version.c                |   7 +-
 xen/include/asm-x86/alternative.h   |   1 +
 xen/include/xen/livepatch.h         |   3 +-
 xen/include/xen/livepatch_payload.h |  49 +++++++++
 xen/include/xen/version.h           |   8 ++
 xen/tools/symbols.c                 |  12 ++-
 15 files changed, 348 insertions(+), 65 deletions(-)

Konrad Rzeszutek Wilk (8):
      livepatch: Clear .bss when payload is reverted
      livepatch: Deal with payloads without any .text
      version/livepatch: Move xen_build_id_check to version.h
      version: Print build-id at bootup.
      livepatch: Move code from prepare_payload to own routine
      livepatch: Add parsing for the symbol+0x<offset>
      livepatch: NOP if func->new_[addr] is zero.
      symbols: Generate an xen-sym.map

Ross Lagerwall (1):
      livepach: Add .livepatch.hooks functions and test-case


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

[PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Konrad Rzeszutek Wilk]

So that when we apply the patch again the .bss is cleared.
Otherwise we may find some variables containing old values.

The payloads may contain various .bss - especially if -fdata-sections
is used which can create .bss.<name> sections.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>

v3: Initial submission
v4: s/EINVAL/EOPNOTSUPP/
    Do memset in a single place
    Support multiple BSS sections.
---
 xen/common/livepatch.c | 60 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 46 insertions(+), 14 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 5da28a3..b5aef57 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -70,6 +70,9 @@ 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). */
+    void **bss;                          /* .bss's of the payload. */
+    size_t *bss_size;                    /* and their sizes. */
+    size_t n_bss;                        /* Size of the array. */
     char name[XEN_LIVEPATCH_NAME_SIZE]; /* Name of it. */
 };
 
@@ -255,12 +258,18 @@ static struct payload *find_payload(const char *name)
 static void free_payload_data(struct payload *payload)
 {
     /* Set to zero until "move_payload". */
-    if ( !payload->pages )
-        return;
-
-    vfree((void *)payload->text_addr);
+    if ( payload->pages )
+    {
+        vfree((void *)payload->text_addr);
+        payload->pages = 0;
+    }
 
-    payload->pages = 0;
+    if ( payload->n_bss )
+    {
+        xfree(payload->bss);
+        xfree(payload->bss_size);
+        payload->n_bss = 0;
+    }
 }
 
 /*
@@ -287,6 +296,7 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
     unsigned int i;
     size_t size = 0;
     unsigned int *offset;
+    unsigned int n_bss = 0;
     int rc = 0;
 
     offset = xmalloc_array(unsigned int, elf->hdr->e_shnum);
@@ -309,7 +319,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
             calc_section(&elf->sec[i], &payload->text_size, &offset[i]);
         else if ( !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
                   (elf->sec[i].sec->sh_flags & SHF_WRITE) )
+        {
             calc_section(&elf->sec[i], &payload->rw_size, &offset[i]);
+            if ( elf->sec[i].sec->sh_type == SHT_NOBITS )
+                n_bss++;
+        }
         else if ( !(elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
                   !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
             calc_section(&elf->sec[i], &payload->ro_size, &offset[i]);
@@ -334,12 +348,8 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
     size = PFN_UP(size); /* Nr of pages. */
     text_buf = vmalloc_xen(size * PAGE_SIZE);
     if ( !text_buf )
-    {
-        dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for payload!\n",
-                elf->name);
-        rc = -ENOMEM;
-        goto out;
-    }
+        goto out_mem;
+
     rw_buf = text_buf + PAGE_ALIGN(payload->text_size);
     ro_buf = rw_buf + PAGE_ALIGN(payload->rw_size);
 
@@ -348,6 +358,14 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
     payload->rw_addr = rw_buf;
     payload->ro_addr = ro_buf;
 
+    payload->bss = xmalloc_array(void *, n_bss);
+    payload->bss_size = xmalloc_array(size_t, n_bss);
+    if ( !payload->bss || !payload->bss_size )
+        goto out_mem;
+
+    payload->n_bss = n_bss;
+    n_bss = 0; /* Reusing as counter. */
+
     for ( i = 1; i < elf->hdr->e_shnum; i++ )
     {
         if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
@@ -374,14 +392,24 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
                         elf->name, elf->sec[i].name, elf->sec[i].load_addr);
             }
             else
-                memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
+            {
+                payload->bss[n_bss] = elf->sec[i].load_addr;
+                payload->bss_size[n_bss++] = elf->sec[i].sec->sh_size;
+            }
         }
     }
+    ASSERT(n_bss == payload->n_bss);
 
  out:
     xfree(offset);
 
     return rc;
+
+ out_mem:
+    dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for payload!\n",
+            elf->name);
+    rc = -ENOMEM;
+    goto out;
 }
 
 static int secure_payload(struct payload *payload, struct livepatch_elf *elf)
@@ -997,6 +1025,10 @@ static int apply_payload(struct payload *data)
     printk(XENLOG_INFO LIVEPATCH "%s: Applying %u functions\n",
             data->name, data->nfuncs);
 
+    /* And clear the BSS for subsequent operation. */
+    for ( i = 0; i < data->n_bss; i++ )
+        memset(data->bss[i], 0, data->bss_size[i]);
+
     arch_livepatch_quiesce();
 
     for ( i = 0; i < data->nfuncs; i++ )
@@ -1513,9 +1545,9 @@ static void livepatch_printall(unsigned char key)
 
     list_for_each_entry ( data, &payload_list, list )
     {
-        printk(" name=%s state=%s(%d) %p (.data=%p, .rodata=%p) using %u pages.\n",
+        printk(" name=%s state=%s(%d) %p (.data=%p, .rodata=%p) using %u pages (%zu .bss).\n",
                data->name, state2str(data->state), data->state, data->text_addr,
-               data->rw_addr, data->ro_addr, data->pages);
+               data->rw_addr, data->ro_addr, data->pages, data->n_bss);
 
         for ( i = 0; i < data->nfuncs; i++ )
         {
-- 
2.4.11


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

[PATCH v4 2/9] livepatch: Deal with payloads without any .text

[Konrad Rzeszutek Wilk]

It is possible. Especially if the only thing they do is
NOP functions - in which case there is only .livepatch.funcs
sections.

Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
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: Add Jan's and Ross's Reviewed-by
---
 xen/common/livepatch.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index b5aef57..88f1543 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -414,16 +414,17 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
 
 static int secure_payload(struct payload *payload, struct livepatch_elf *elf)
 {
-    int rc;
+    int rc = 0;
     unsigned int text_pages, rw_pages, ro_pages;
 
     text_pages = PFN_UP(payload->text_size);
-    ASSERT(text_pages);
-
-    rc = arch_livepatch_secure(payload->text_addr, text_pages, LIVEPATCH_VA_RX);
-    if ( rc )
-        return rc;
 
+    if ( text_pages )
+    {
+        rc = arch_livepatch_secure(payload->text_addr, text_pages, LIVEPATCH_VA_RX);
+        if ( rc )
+            return rc;
+    }
     rw_pages = PFN_UP(payload->rw_size);
     if ( rw_pages )
     {
-- 
2.4.11


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

[PATCH v4 3/9] version/livepatch: Move xen_build_id_check to version.h

[Konrad Rzeszutek Wilk]

It makes more sense for it to be there. However that
means the version.h has now a dependency on <xen/elfstructs.h>
as the Elf_Note is a macro.

The elfstructs.h has a dependency on types.h as well so
we need that. We cannot put that #include <xen/types.h>
in elfstructs.h as that file is used by tools and they
do not have such file.

Reviewed-by: Jan Beulich <jbeulich@suse.com>
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>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3: First submission
v4: Added Ross's and Jan's Reviewed-by.
---
 xen/include/xen/livepatch.h | 2 --
 xen/include/xen/version.h   | 8 ++++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index ed49843..02f4572 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -42,8 +42,6 @@ int livepatch_op(struct xen_sysctl_livepatch_op *);
 void check_for_livepatch_work(void);
 unsigned long livepatch_symbols_lookup_by_name(const char *symname);
 bool_t is_patch(const void *addr);
-int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
-                       const void **p, unsigned int *len);
 
 /* Arch hooks. */
 int arch_livepatch_verify_elf(const struct livepatch_elf *elf);
diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h
index 400160f..97c247a 100644
--- a/xen/include/xen/version.h
+++ b/xen/include/xen/version.h
@@ -1,6 +1,9 @@
 #ifndef __XEN_VERSION_H__
 #define __XEN_VERSION_H__
 
+#include <xen/types.h>
+#include <xen/elfstructs.h>
+
 const char *xen_compile_date(void);
 const char *xen_compile_time(void);
 const char *xen_compile_by(void);
@@ -15,4 +18,9 @@ const char *xen_banner(void);
 const char *xen_deny(void);
 int xen_build_id(const void **p, unsigned int *len);
 
+#ifdef BUILD_ID
+int xen_build_id_check(const Elf_Note *n, unsigned int n_sz,
+                       const void **p, unsigned int *len);
+#endif
+
 #endif /* __XEN_VERSION_H__ */
-- 
2.4.11


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

[PATCH v4 4/9] version: Print build-id at bootup.

[Konrad Rzeszutek Wilk]

Livepatch expected at some point to be able to print the
build-id during bootup, which it did not.  The reason is
that xen_build_init and livepatch_init are both __initcall
type routines. This meant that when livepatch_init called
xen_build_id, it would return -ENODATA as build_id_len was
not setup yet (b/c xen_build_init would be called later).

The original patch fixed this by calling xen_build_init in
livepatch_init which allows us to print the build-id of
the hypervisor.

However the x86 maintainers pointed out that build-id
is independent of Livepatch and in fact should print
regardless whether Livepatch is enabled or not.

Therefore this patch moves the logic of printing the build-id
to version.c.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>

v2: Move xen_build_init in version.h instead of livepatch.h
v3: Posted as "livepatch: Sync cache of build-id before using it first time"
v4: Move the printing of build-id to version.c.
    Change title
---
 xen/common/livepatch.c | 6 ------
 xen/common/version.c   | 7 ++++++-
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 88f1543..bd65712 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1579,12 +1579,6 @@ static void livepatch_printall(unsigned char key)
 
 static int __init livepatch_init(void)
 {
-    const void *binary_id;
-    unsigned int len;
-
-    if ( !xen_build_id(&binary_id, &len) )
-        printk(XENLOG_INFO LIVEPATCH ": build-id: %*phN\n", len, binary_id);
-
     register_keyhandler('x', livepatch_printall, "print livepatch info", 1);
 
     arch_livepatch_init();
diff --git a/xen/common/version.c b/xen/common/version.c
index b2afe96..74346b8 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -121,6 +121,7 @@ static int __init xen_build_init(void)
 {
     const Elf_Note *n = __note_gnu_build_id_start;
     unsigned int sz;
+    int rc;
 
     /* --build-id invoked with wrong parameters. */
     if ( __note_gnu_build_id_end <= &n[0] )
@@ -132,7 +133,11 @@ static int __init xen_build_init(void)
 
     sz = (void *)__note_gnu_build_id_end - (void *)n;
 
-    return xen_build_id_check(n, sz, &build_id_p, &build_id_len);
+    rc = xen_build_id_check(n, sz, &build_id_p, &build_id_len);
+    if ( !rc )
+        printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p);
+
+    return rc;
 }
 __initcall(xen_build_init);
 #endif
-- 
2.4.11


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

[PATCH v4 5/9] livepatch: Move code from prepare_payload to own routine

[Konrad Rzeszutek Wilk]

Specifically the code that is looking up f->old_addr - which
can be in its own routine instead of having it part of prepare_payload.

No functional change.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>

v3: First submission.
v4: Added const to last parameter of  livepatch_symbols_lookup function.
    Rename function to resolve_old_address
    Move comment describing function purpose above function
---
 xen/common/livepatch.c | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index bd65712..e5968a7 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -233,6 +233,30 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
     return n;
 }
 
+/* Lookup function's old address if not already resolved. */
+static int resolve_old_address(struct livepatch_func *f,
+                               const struct livepatch_elf *elf)
+{
+    if ( f->old_addr )
+        return 0;
+
+    f->old_addr = (void *)symbols_lookup_by_name(f->name);
+    if ( !f->old_addr )
+    {
+        f->old_addr = (void *)livepatch_symbols_lookup_by_name(f->name);
+        if ( !f->old_addr )
+        {
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not resolve old address of %s\n",
+                    elf->name, f->name);
+            return -ENOENT;
+        }
+    }
+    dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Resolved old address %s => %p\n",
+            elf->name, f->name, f->old_addr);
+
+    return 0;
+}
+
 static struct payload *find_payload(const char *name)
 {
     struct payload *data, *found = NULL;
@@ -525,23 +549,9 @@ static int prepare_payload(struct payload *payload,
         if ( rc )
             return rc;
 
-        /* Lookup function's old address if not already resolved. */
-        if ( !f->old_addr )
-        {
-            f->old_addr = (void *)symbols_lookup_by_name(f->name);
-            if ( !f->old_addr )
-            {
-                f->old_addr = (void *)livepatch_symbols_lookup_by_name(f->name);
-                if ( !f->old_addr )
-                {
-                    dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not resolve old address of %s\n",
-                            elf->name, f->name);
-                    return -ENOENT;
-                }
-            }
-            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Resolved old address %s => %p\n",
-                    elf->name, f->name, f->old_addr);
-        }
+        rc = resolve_old_address(f, elf);
+        if ( rc )
+            return rc;
     }
 
     sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
-- 
2.4.11


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

[PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>

[Konrad Rzeszutek Wilk]

in case we want to patch at specific offsets inside
a function. (for example if we want to do NOP patching).

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: Stefano Stabellini <sstabellini@kernel.org>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3: First submission
v4: Drop the /<len> part of the symbol.
---
 docs/misc/livepatch.markdown |  2 +-
 xen/common/livepatch.c       | 32 ++++++++++++++++++++++++++++++--
 xen/include/xen/livepatch.h  |  1 +
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 89c1050..7e82047 100644
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -312,7 +312,7 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be
 
 * `name` is the symbol name of the old function. Only used if `old_addr` is
    zero, otherwise will be used during dynamic linking (when hypervisor loads
-   the payload).
+   the payload). The format can be _symbol_ or _symbol+0x<offset>_.
 
 * `old_addr` is the address of the function to be patched and is filled in at
   payload generation time if hypervisor function address is known. If unknown,
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index e5968a7..c234424 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -237,13 +237,34 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
 static int resolve_old_address(struct livepatch_func *f,
                                const struct livepatch_elf *elf)
 {
+    const char *s;
+    char *plus = NULL;
+    unsigned long offset = 0;
+
     if ( f->old_addr )
         return 0;
 
-    f->old_addr = (void *)symbols_lookup_by_name(f->name);
+    s = f->name;
+    /* +<offset> */
+    plus = strchr(f->name, '+');
+    if ( plus )
+    {
+        const char *endp = NULL;
+
+        offset = simple_strtoul(plus + 1, &endp, 16);
+
+        if ( *endp != '\0' )
+            return -EINVAL;
+
+        /* So that symbol lookup works. */
+        *plus = '\0';
+        s = f->name;
+    }
+
+    f->old_addr = (void *)symbols_lookup_by_name(s);
     if ( !f->old_addr )
     {
-        f->old_addr = (void *)livepatch_symbols_lookup_by_name(f->name);
+        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",
@@ -251,6 +272,13 @@ static int resolve_old_address(struct livepatch_func *f,
             return -ENOENT;
         }
     }
+
+    if ( plus )
+    {
+        *plus = '+';
+        f->old_addr += offset;
+    }
+
     dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Resolved old address %s => %p\n",
             elf->name, f->name, f->old_addr);
 
diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
index 02f4572..2e64686 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -44,6 +44,7 @@ unsigned long livepatch_symbols_lookup_by_name(const char *symname);
 bool_t is_patch(const void *addr);
 
 /* Arch hooks. */
+int arch_verify_insn_length(unsigned long len);
 int arch_livepatch_verify_elf(const struct livepatch_elf *elf);
 int arch_livepatch_perform_rel(struct livepatch_elf *elf,
                                const struct livepatch_elf_sec *base,
-- 
2.4.11


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

[PATCH v4 7/9] livepatch: NOP if func->new_[addr] is zero.

[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.
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(-)

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);
-- 
2.4.11


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

[PATCH v4 8/9] symbols: Generate an xen-sym.map

[Konrad Rzeszutek Wilk]

You could construct _most_ of the names of the functions
by doing 'nm --defined' but unfortunatly you do not get the
<file> prefix that is added on in Xen . For example:

$ cat xen-syms.symbols |grep do_domain_pause
0xffff82d080104920 t domain.c#do_domain_pause
$ nm --defined xen-syms|grep do_domain_pause
ffff82d080104920 t do_domain_pause

This is normally not an issue, but if one is doing livepatching and
wants during build-time verify that the symbols the livepatch payloads
will patch do correspond to the one the hypervisor has built - this helps a lot.

Note that during runtime one can do:
[root@localhost xen]# cat /proc/xen/xensyms |grep do_domain_pause
ffff82d080104920 t domain.c#do_domain_pause

But one may not want to build and verify a livepatch on the same host.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
Cc: Ross Lagerwall <ross.lagerwall@citrix.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>

v1: First submission
v2: Do not use intermediary object file but the final one
v3: Fix comment to have the example from /proc/xen/xensyms. Using '#'
    at start of line is a BAD IDEA.
v4: Also do it for EFI builds.
    Do not use --warn-dup
---
 .gitignore            |  1 +
 xen/Makefile          |  6 +++++-
 xen/arch/arm/Makefile |  3 +++
 xen/arch/x86/Makefile |  7 ++++++-
 xen/tools/symbols.c   | 12 +++++++++++-
 5 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/.gitignore b/.gitignore
index d193820..44cc7bf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -287,6 +287,7 @@ tools/flask/policy/policy.conf
 tools/flask/policy/xenpolicy-*
 xen/xen
 xen/xen-syms
+xen/xen-syms.map
 xen/xen.*
 unmodified_drivers/linux-2.6/.tmp_versions
 unmodified_drivers/linux-2.6/*.cmd
diff --git a/xen/Makefile b/xen/Makefile
index 76b60bc..294fb9e 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -62,10 +62,12 @@ _install: $(TARGET)$(CONFIG_XEN_INSTALL_SUFFIX)
 	ln -f -s $(T)-$(XEN_FULLVERSION)$(Z) $(D)$(BOOT_DIR)/$(T)$(Z)
 	[ -d "$(D)$(DEBUG_DIR)" ] || $(INSTALL_DIR) $(D)$(DEBUG_DIR)
 	$(INSTALL_DATA) $(TARGET)-syms $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION)
+	$(INSTALL_DATA) $(TARGET)-syms.map $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map
 	$(INSTALL_DATA) $(KCONFIG_CONFIG) $(D)$(BOOT_DIR)/$(T)-$(XEN_FULLVERSION).config
 	if [ -r $(TARGET).efi -a -n '$(EFI_DIR)' ]; then \
 		[ -d $(D)$(EFI_DIR) ] || $(INSTALL_DIR) $(D)$(EFI_DIR); \
 		$(INSTALL_DATA) $(TARGET).efi $(D)$(EFI_DIR)/$(T)-$(XEN_FULLVERSION).efi; \
+		$(INSTALL_DATA) $(TARGET)-efi.map $(D)$(DEBUG_DIR)/$(T)-$(XEN_FULLVERSION).efi.map \
 		ln -sf $(T)-$(XEN_FULLVERSION).efi $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).efi; \
 		ln -sf $(T)-$(XEN_FULLVERSION).efi $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).efi; \
 		ln -sf $(T)-$(XEN_FULLVERSION).efi $(D)$(EFI_DIR)/$(T).efi; \
@@ -91,8 +93,10 @@ _uninstall:
 	rm -f $(D)$(BOOT_DIR)/$(T)-$(XEN_VERSION)$(Z)
 	rm -f $(D)$(BOOT_DIR)/$(T)$(Z)
 	rm -f $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION)
+	rm -f $(D)$(DEBUG_DIR)/$(T)-syms-$(XEN_FULLVERSION).map
 	rm -f $(D)$(EFI_DIR)/$(T)-$(XEN_FULLVERSION).efi
 	rm -f $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).$(XEN_SUBVERSION).efi
+	rm -f $(D)$(DEBUG_DIR)/$(T)-$(XEN_FULLVERSION).efi.map
 	rm -f $(D)$(EFI_DIR)/$(T)-$(XEN_VERSION).efi
 	rm -f $(D)$(EFI_DIR)/$(T).efi
 	rm -f $(D)$(EFI_MOUNTPOINT)/efi/$(EFI_VENDOR)/$(T)-$(XEN_FULLVERSION).efi
@@ -112,7 +116,7 @@ _clean: delete-unfresh-files
 	$(MAKE) -f $(BASEDIR)/Rules.mk -C arch/$(TARGET_ARCH) clean
 	$(MAKE) -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) clean
 	find . \( -name "*.o" -o -name ".*.d" \) -exec rm -f {} \;
-	rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET)-syms *~ core
+	rm -f include/asm $(TARGET) $(TARGET).gz $(TARGET).efi $(TARGET).efi.map $(TARGET)-syms $(TARGET)-syms.map *~ core
 	rm -f include/asm-*/asm-offsets.h
 	rm -f .banner
 
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 23aaf52..0a96713 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -102,6 +102,9 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
 	    $(@D)/.$(@F).1.o -o $@
+	$(NM) -pa --format=sysv $(@D)/$(@F) \
+		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort \
+		>$(@D)/$(@F).map
 	rm -f $(@D)/.$(@F).[0-9]*
 
 asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 5b9e9da..7209560 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -138,6 +138,9 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o $(build_id_linker) \
 	    $(@D)/.$(@F).1.o -o $@
+	$(NM) -pa --format=sysv $(@D)/$(@F) \
+		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort \
+		>$(@D)/$(@F).map
 	rm -f $(@D)/.$(@F).[0-9]*
 
 note.o: $(TARGET)-syms
@@ -182,7 +185,9 @@ $(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbol
 	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o
 	$(guard) $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T efi.lds -N $< \
 	                $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o $(note_file) -o $@
-	if $(guard) false; then rm -f $@; echo 'EFI support disabled'; fi
+	if $(guard) false; then rm -f $@; echo 'EFI support disabled'; \
+	else $(NM) -pa --format=sysv $(@D)/$(@F) \
+		| $(BASEDIR)/tools/symbols --xensyms --sysv --sort >$(@D)/$(@F).map; fi
 	rm -f $(@D)/.$(@F).[0-9]*
 
 efi/boot.init.o efi/runtime.o efi/compat.o: $(BASEDIR)/arch/x86/efi/built_in.o
diff --git a/xen/tools/symbols.c b/xen/tools/symbols.c
index 941fbe7..8c5842d 100644
--- a/xen/tools/symbols.c
+++ b/xen/tools/symbols.c
@@ -52,6 +52,7 @@ static unsigned int table_size, table_cnt;
 static unsigned long long _stext, _etext, _sinittext, _einittext, _sextratext, _eextratext;
 static int all_symbols = 0;
 static int sort_by_name = 0;
+static int map_only = 0;
 static char symbol_prefix_char = '\0';
 static enum { fmt_bsd, fmt_sysv } input_format;
 static int compare_name(const void *p1, const void *p2);
@@ -181,7 +182,7 @@ static int read_symbol(FILE *in, struct sym_entry *s)
 		*sym++ = '#';
 	}
 	strcpy(sym, str);
-	if (sort_by_name) {
+	if (sort_by_name || map_only) {
 		s->orig_symbol = strdup(SYMBOL_NAME(s));
 		s->type = stype; /* As s->sym[0] ends mangled. */
 	}
@@ -307,6 +308,13 @@ static void write_src(void)
 	unsigned int *markers;
 	char buf[KSYM_NAME_LEN+1];
 
+	if (map_only) {
+		for (i = 0; i < table_cnt; i++)
+			printf("%#llx %c %s\n", table[i].addr, table[i].type,
+						table[i].orig_symbol);
+
+		return;
+	}
 	printf("#include <xen/config.h>\n");
 	printf("#include <asm/types.h>\n");
 	printf("#if BITS_PER_LONG == 64 && !defined(SYMBOLS_ORIGIN)\n");
@@ -609,6 +617,8 @@ int main(int argc, char **argv)
 				sort_by_name = 1;
 			else if (strcmp(argv[i], "--warn-dup") == 0)
 				warn_dup = true;
+			else if (strcmp(argv[i], "--xensyms") == 0)
+				map_only = true;
 			else
 				usage();
 		}
-- 
2.4.11


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

[PATCH v4 9/9] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk]

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.

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.
---
 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 ad0df26..9f52aeb 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 dd24a07..528c0c9 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>
 
@@ -73,7 +74,11 @@ struct payload {
     void **bss;                          /* .bss's of the payload. */
     size_t *bss_size;                    /* and their sizes. */
     size_t n_bss;                        /* Size of the array. */
-    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. */
@@ -583,6 +588,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 )
     {
@@ -1071,6 +1095,18 @@ static int apply_payload(struct payload *data)
 
     arch_livepatch_quiesce();
 
+    /*
+     * 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]);
 
@@ -1097,6 +1133,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.4.11


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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Jan Beulich]

>>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -70,6 +70,9 @@ 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). */
> +    void **bss;                          /* .bss's of the payload. */
> +    size_t *bss_size;                    /* and their sizes. */

Is size_t wide enough in the extreme case? Perhaps yes, because I
don't think we'll ever load 64-bit ELF on a 32-bit platform.

> +    size_t n_bss;                        /* Size of the array. */

As opposed to that, I think this one could be unsigned int (or else
you end up with inconsistencies in {move,apply}_payload()).

> @@ -374,14 +392,24 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
>                          elf->name, elf->sec[i].name, elf->sec[i].load_addr);
>              }
>              else
> -                memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> +            {
> +                payload->bss[n_bss] = elf->sec[i].load_addr;
> +                payload->bss_size[n_bss++] = elf->sec[i].sec->sh_size;
> +            }
>          }
>      }
> +    ASSERT(n_bss == payload->n_bss);
>  
>   out:
>      xfree(offset);
>  
>      return rc;
> +
> + out_mem:
> +    dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for payload!\n",
> +            elf->name);
> +    rc = -ENOMEM;
> +    goto out;

You leak any of the three buffers here which you managed to
successfully allocate.

Jan


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

Re: [PATCH v4 4/9] version: Print build-id at bootup.

[Jan Beulich]

>>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> Livepatch expected at some point to be able to print the
> build-id during bootup, which it did not.  The reason is
> that xen_build_init and livepatch_init are both __initcall
> type routines. This meant that when livepatch_init called
> xen_build_id, it would return -ENODATA as build_id_len was
> not setup yet (b/c xen_build_init would be called later).
> 
> The original patch fixed this by calling xen_build_init in
> livepatch_init which allows us to print the build-id of
> the hypervisor.
> 
> However the x86 maintainers pointed out that build-id
> is independent of Livepatch and in fact should print
> regardless whether Livepatch is enabled or not.
> 
> Therefore this patch moves the logic of printing the build-id
> to version.c.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>

[Jan Beulich]

>>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -237,13 +237,34 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
>  static int resolve_old_address(struct livepatch_func *f,
>                                 const struct livepatch_elf *elf)
>  {
> +    const char *s;
> +    char *plus = NULL;

Pointless initializer.

> +    unsigned long offset = 0;
> +
>      if ( f->old_addr )
>          return 0;
>  
> -    f->old_addr = (void *)symbols_lookup_by_name(f->name);
> +    s = f->name;

Otoh this could become s'es initializer.

> +    /* +<offset> */
> +    plus = strchr(f->name, '+');

And I think you should prefer using the local variable here.

Furthermore you're losing const here - does f->name really point
to memory that doesn't get mapped r/o?

> +    if ( plus )
> +    {
> +        const char *endp = NULL;

Pointless initializer again (or else ...

> +        offset = simple_strtoul(plus + 1, &endp, 16);
> +
> +        if ( *endp != '\0' )

... the deref here couldn't be unconditional).

> +            return -EINVAL;
> +
> +        /* So that symbol lookup works. */
> +        *plus = '\0';
> +        s = f->name;

Why? f->name didn't change afaict.

Overall - are you sure you want to disallow symbol names containing
+ characters? I.e. you don't want to add support for some form of
quoting?

Jan


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

Re: [PATCH v4 7/9] livepatch: NOP if func->new_[addr] is zero.

[Jan Beulich]

>>> 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


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

Re: [PATCH v4 8/9] symbols: Generate an xen-sym.map

[Jan Beulich]

>>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> You could construct _most_ of the names of the functions
> by doing 'nm --defined' but unfortunatly you do not get the
> <file> prefix that is added on in Xen . For example:
> 
> $ cat xen-syms.symbols |grep do_domain_pause
> 0xffff82d080104920 t domain.c#do_domain_pause
> $ nm --defined xen-syms|grep do_domain_pause
> ffff82d080104920 t do_domain_pause
> 
> This is normally not an issue, but if one is doing livepatching and
> wants during build-time verify that the symbols the livepatch payloads
> will patch do correspond to the one the hypervisor has built - this helps a 
> lot.
> 
> Note that during runtime one can do:
> [root@localhost xen]# cat /proc/xen/xensyms |grep do_domain_pause
> ffff82d080104920 t domain.c#do_domain_pause
> 
> But one may not want to build and verify a livepatch on the same host.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v4 5/9] livepatch: Move code from prepare_payload to own routine

[Ross Lagerwall]

On 08/23/2016 10:22 PM, Konrad Rzeszutek Wilk wrote:
> Specifically the code that is looking up f->old_addr - which
> can be in its own routine instead of having it part of prepare_payload.
>
> No functional change.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> ---
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
>
> v3: First submission.
> v4: Added const to last parameter of  livepatch_symbols_lookup function.
>      Rename function to resolve_old_address
>      Move comment describing function purpose above function
> ---
>   xen/common/livepatch.c | 44 +++++++++++++++++++++++++++-----------------
>   1 file changed, 27 insertions(+), 17 deletions(-)
>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Andrew Cooper]

On 24/08/16 09:55, Jan Beulich wrote:
>>>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
>> --- a/xen/common/livepatch.c
>> +++ b/xen/common/livepatch.c
>> @@ -70,6 +70,9 @@ 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). */
>> +    void **bss;                          /* .bss's of the payload. */
>> +    size_t *bss_size;                    /* and their sizes. */
> Is size_t wide enough in the extreme case? Perhaps yes, because I
> don't think we'll ever load 64-bit ELF on a 32-bit platform.

Even if we did, there is no chance that more than a single size_t's 
worth of data needs clearing, or the payload wouldn't fit in the current 
virtual address space.

~Andrew

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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Konrad Rzeszutek Wilk]

On Wed, Aug 24, 2016 at 02:55:21AM -0600, Jan Beulich wrote:
> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -70,6 +70,9 @@ 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). */
> > +    void **bss;                          /* .bss's of the payload. */
> > +    size_t *bss_size;                    /* and their sizes. */
> 
> Is size_t wide enough in the extreme case? Perhaps yes, because I
> don't think we'll ever load 64-bit ELF on a 32-bit platform.

Nonethless having a huge .bss is a kind of extreme? Perhaps we should
have an seperate patch that checks the SHT_NOBITS and disallows .bss's
bigger than say 2MB?

I am using 2MB as that is the limit of the livepatch binaries right
now (see verify_payload:

                                                                             
 127     if ( upload->size > MB(2) )                                                 
 128         return -EINVAL;    


> 
> > +    size_t n_bss;                        /* Size of the array. */
> 
> As opposed to that, I think this one could be unsigned int (or else
> you end up with inconsistencies in {move,apply}_payload()).

/me nods. Changed to unsitned int.
> 
> > @@ -374,14 +392,24 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >                          elf->name, elf->sec[i].name, elf->sec[i].load_addr);
> >              }
> >              else
> > -                memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> > +            {
> > +                payload->bss[n_bss] = elf->sec[i].load_addr;
> > +                payload->bss_size[n_bss++] = elf->sec[i].sec->sh_size;
> > +            }
> >          }
> >      }
> > +    ASSERT(n_bss == payload->n_bss);
> >  
> >   out:
> >      xfree(offset);
> >  
> >      return rc;
> > +
> > + out_mem:
> > +    dprintk(XENLOG_ERR, LIVEPATCH "%s: Could not allocate memory for payload!\n",
> > +            elf->name);
> > +    rc = -ENOMEM;
> > +    goto out;
> 
> You leak any of the three buffers here which you managed to
> successfully allocate.

I added a call to 'free_payload_data(payload)' there to make it a direct call to it.
It is not needed per say as the caller unconditionally calls free_payload_data() if
the return of any of the functions is non-zero. But in case things get moved around
and that assumption goes away - having a call to free_payload_data makes sense
in the function (plus it looks much more nicer to free/alloc in the same function).

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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2016 at 05:08:16PM +0100, Andrew Cooper wrote:
> On 24/08/16 09:55, Jan Beulich wrote:
> > > > > On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> > > --- a/xen/common/livepatch.c
> > > +++ b/xen/common/livepatch.c
> > > @@ -70,6 +70,9 @@ 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). */
> > > +    void **bss;                          /* .bss's of the payload. */
> > > +    size_t *bss_size;                    /* and their sizes. */
> > Is size_t wide enough in the extreme case? Perhaps yes, because I
> > don't think we'll ever load 64-bit ELF on a 32-bit platform.
> 
> Even if we did, there is no chance that more than a single size_t's worth of
> data needs clearing, or the payload wouldn't fit in the current virtual
> address space.

Perhaps go even in further an add an arbitrary limit?
Like so (compile tested only):

From 22d0a6e0c6fc61a9e257ec4db78c2e58978b2976 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 6 Sep 2016 12:45:50 -0400
Subject: [PATCH] livepatch: Add limit of 2MB to payload .bss sections.

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.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/common/livepatch_elf.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index 789e8fc..4a4111d 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -86,7 +86,16 @@ 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_EXECINSTR) &&
+                  (sec[i].sec->sh_flags & SHF_WRITE) &&
+                  sec[i].sec->sh_type == SHT_NOBITS &&
+                  sec[i].sec->sh_size > MB(2) )
+        {
+            /* Arbitrary limit. */
+            dprintk(XENLOG_ERR, LIVEPATCH "%s: Section [%u] .bss is bigger than 2MB!\n",
+                    elf->name, i);
+            return -EINVAL;
+        }
         sec[i].data = data + delta;
         /* Name is populated in elf_resolve_section_names. */
         sec[i].name = NULL;
-- 
2.4.11


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

Re: [PATCH v4 4/9] version: Print build-id at bootup.

[Konrad Rzeszutek Wilk]

On Wed, Aug 24, 2016 at 02:58:48AM -0600, Jan Beulich wrote:
> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> > Livepatch expected at some point to be able to print the
> > build-id during bootup, which it did not.  The reason is
> > that xen_build_init and livepatch_init are both __initcall
> > type routines. This meant that when livepatch_init called
> > xen_build_id, it would return -ENODATA as build_id_len was
> > not setup yet (b/c xen_build_init would be called later).
> > 
> > The original patch fixed this by calling xen_build_init in
> > livepatch_init which allows us to print the build-id of
> > the hypervisor.
> > 
> > However the x86 maintainers pointed out that build-id
> > is independent of Livepatch and in fact should print
> > regardless whether Livepatch is enabled or not.
> > 
> > Therefore this patch moves the logic of printing the build-id
> > to version.c.
> > 
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thank you.

I had a slight change due to rebasing on "x86/EFI: use less crude a way of generating the build"
which was quite simple to fix so I retained your Reviewed-by tag.

The final patch looks as so:

From 927c9accac9a49b586f616060e5567d7b03e3e77 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Tue, 6 Sep 2016 12:18:10 -0400
Subject: [PATCH] version: Print build-id at bootup.

Livepatch expected at some point to be able to print the
build-id during bootup, which it did not.  The reason is
that xen_build_init and livepatch_init are both __initcall
type routines. This meant that when livepatch_init called
xen_build_id, it would return -ENODATA as build_id_len was
not setup yet (b/c xen_build_init would be called later).

The original patch fixed this by calling xen_build_init in
livepatch_init which allows us to print the build-id of
the hypervisor.

However the x86 maintainers pointed out that build-id
is independent of Livepatch and in fact should print
regardless whether Livepatch is enabled or not.

Therefore this patch moves the logic of printing the build-id
to version.c.

Reviewed-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

---
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>

v2: Move xen_build_init in version.h instead of livepatch.h
v3: Posted as "livepatch: Sync cache of build-id before using it first time"
v4: Move the printing of build-id to version.c.
    Change title
v5: Rebased on top "x86/EFI: use less crude a way of generating the build ID"
    Added Jan's Ack.
---
 xen/common/livepatch.c | 6 ------
 xen/common/version.c   | 2 ++
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 78b3731..4c3056c 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1580,12 +1580,6 @@ static void livepatch_printall(unsigned char key)
 
 static int __init livepatch_init(void)
 {
-    const void *binary_id;
-    unsigned int len;
-
-    if ( !xen_build_id(&binary_id, &len) )
-        printk(XENLOG_INFO LIVEPATCH ": build-id: %*phN\n", len, binary_id);
-
     register_keyhandler('x', livepatch_printall, "print livepatch info", 1);
 
     arch_livepatch_init();
diff --git a/xen/common/version.c b/xen/common/version.c
index 4375ea2..0d31e38 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -186,6 +186,8 @@ static int __init xen_build_init(void)
         }
     }
 #endif
+    if ( !rc )
+        printk(XENLOG_INFO "build-id: %*phN\n", build_id_len, build_id_p);
 
     return rc;
 }
-- 
2.4.11

> 

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

Re: [PATCH v4 9/9] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk]

On Tue, Aug 23, 2016 at 10:22:12PM -0400, Konrad Rzeszutek Wilk wrote:
> 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.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>


So... anybody willing to review it :-)

> 
> ---
> 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>


In regards to this going in v4.8 my recollection is that:

 George: 0
 Andrew: +1
 Jan: 0 (with a slight leaning towards -1)?

I think that means folks are OK or 'don't care'.

And the livepatch maintainers:
 Ross: +1 (obviously since he wrote it)
 Konrad: +1

> 
> 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.
> ---
>  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 ad0df26..9f52aeb 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 dd24a07..528c0c9 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>
>  
> @@ -73,7 +74,11 @@ struct payload {
>      void **bss;                          /* .bss's of the payload. */
>      size_t *bss_size;                    /* and their sizes. */
>      size_t n_bss;                        /* Size of the array. */
> -    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. */
> @@ -583,6 +588,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 )
>      {
> @@ -1071,6 +1095,18 @@ static int apply_payload(struct payload *data)
>  
>      arch_livepatch_quiesce();
>  
> +    /*
> +     * 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]);
>  
> @@ -1097,6 +1133,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.4.11
> 

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

Re: [PATCH v4 9/9] livepach: Add .livepatch.hooks functions and test-case

[Andrew Cooper]

On 06/09/16 18:22, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 23, 2016 at 10:22:12PM -0400, Konrad Rzeszutek Wilk wrote:
>> 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.
>>
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>
> So... anybody willing to review it :-)
>
>> ---
>> 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>
>
> In regards to this going in v4.8 my recollection is that:
>
>  George: 0
>  Andrew: +1
>  Jan: 0 (with a slight leaning towards -1)?
>
> I think that means folks are OK or 'don't care'.
>
> And the livepatch maintainers:
>  Ross: +1 (obviously since he wrote it)
>  Konrad: +1

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>

[Konrad Rzeszutek Wilk]

On Wed, Aug 24, 2016 at 03:08:01AM -0600, Jan Beulich wrote:
> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -237,13 +237,34 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
> >  static int resolve_old_address(struct livepatch_func *f,
> >                                 const struct livepatch_elf *elf)
> >  {
> > +    const char *s;
> > +    char *plus = NULL;
> 
> Pointless initializer.

We need that otherwise this part (which is at the bottom of this function):

    if ( plus )
    {
        *plus = '+';
        f->old_addr += offset;
    }


May be invoked for symbols that that don't have the '+' in them.
> 
> > +    unsigned long offset = 0;
> > +
> >      if ( f->old_addr )
> >          return 0;
> >  
> > -    f->old_addr = (void *)symbols_lookup_by_name(f->name);
> > +    s = f->name;
> 
> Otoh this could become s'es initializer.

<nods>
> 
> > +    /* +<offset> */
> > +    plus = strchr(f->name, '+');
> 
> And I think you should prefer using the local variable here.

<nods>

> 
> Furthermore you're losing const here - does f->name really point
> to memory that doesn't get mapped r/o?

Yes.

The 'struct livepatch_func' contains the the ->opaque array of 31 bytes
(from which we use 5 bytes) which the hypervisor uses to stash the original
instructions.
> 
> > +    if ( plus )
> > +    {
> > +        const char *endp = NULL;
> 
> Pointless initializer again (or else ...
> 
> > +        offset = simple_strtoul(plus + 1, &endp, 16);
> > +
> > +        if ( *endp != '\0' )
> 
> ... the deref here couldn't be unconditional).
> 
> > +            return -EINVAL;
> > +
> > +        /* So that symbol lookup works. */
> > +        *plus = '\0';
> > +        s = f->name;
> 
> Why? f->name didn't change afaict.

Ugh. Stale code.
> 
> Overall - are you sure you want to disallow symbol names containing
> + characters? I.e. you don't want to add support for some form of
> quoting?

Can you actually have + in a function or object?

Or are you asking in terms of the '\0' overwritting the '+' ?
We put it back later on so that the symbol still has + in it:

 274     if ( plus )                                                                 
 275     {                                                                           
 276         *plus = '+';                                                            
 277         f->old_addr += offset;                                                  
 278     }                                           

I will change the comment from 'So that symbol lookup works.' to
'Temporary mutilation of name so that symbol lookup works.'

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

Re: [PATCH v4 7/9] livepatch: NOP if func->new_[addr] is zero.

[Konrad Rzeszutek Wilk]

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
> 

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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Jan Beulich]

>>> On 06.09.16 at 18:47, <konrad.wilk@oracle.com> wrote:
> On Wed, Aug 24, 2016 at 02:55:21AM -0600, Jan Beulich wrote:
>> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
>> > --- a/xen/common/livepatch.c
>> > +++ b/xen/common/livepatch.c
>> > @@ -70,6 +70,9 @@ 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). */
>> > +    void **bss;                          /* .bss's of the payload. */
>> > +    size_t *bss_size;                    /* and their sizes. */
>> 
>> Is size_t wide enough in the extreme case? Perhaps yes, because I
>> don't think we'll ever load 64-bit ELF on a 32-bit platform.
> 
> Nonethless having a huge .bss is a kind of extreme? Perhaps we should
> have an seperate patch that checks the SHT_NOBITS and disallows .bss's
> bigger than say 2MB?

Well, the extra check certainly wouldn't hurt, but I think before
hitting the size_t limit you'd run out of address space to place
the payload in (as that's iirc a less than 1Gb area).

Jan


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

Re: [PATCH v4 4/9] version: Print build-id at bootup.

[Jan Beulich]

>>> On 06.09.16 at 18:57, <konrad.wilk@oracle.com> wrote:
> On Wed, Aug 24, 2016 at 02:58:48AM -0600, Jan Beulich wrote:
>> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
>> > Livepatch expected at some point to be able to print the
>> > build-id during bootup, which it did not.  The reason is
>> > that xen_build_init and livepatch_init are both __initcall
>> > type routines. This meant that when livepatch_init called
>> > xen_build_id, it would return -ENODATA as build_id_len was
>> > not setup yet (b/c xen_build_init would be called later).
>> > 
>> > The original patch fixed this by calling xen_build_init in
>> > livepatch_init which allows us to print the build-id of
>> > the hypervisor.
>> > 
>> > However the x86 maintainers pointed out that build-id
>> > is independent of Livepatch and in fact should print
>> > regardless whether Livepatch is enabled or not.
>> > 
>> > Therefore this patch moves the logic of printing the build-id
>> > to version.c.
>> > 
>> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> 
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Thank you.
> 
> I had a slight change due to rebasing on "x86/EFI: use less crude a way of 
> generating the build"
> which was quite simple to fix so I retained your Reviewed-by tag.

Of course.

Jan


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

Re: [PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>

[Jan Beulich]

>>> On 06.09.16 at 21:56, <konrad.wilk@oracle.com> wrote:
> On Wed, Aug 24, 2016 at 03:08:01AM -0600, Jan Beulich wrote:
>> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
>> > --- a/xen/common/livepatch.c
>> > +++ b/xen/common/livepatch.c
>> > @@ -237,13 +237,34 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
>> >  static int resolve_old_address(struct livepatch_func *f,
>> >                                 const struct livepatch_elf *elf)
>> >  {
>> > +    const char *s;
>> > +    char *plus = NULL;
>> 
>> Pointless initializer.
> 
> We need that otherwise this part (which is at the bottom of this function):
> 
>     if ( plus )
>     {
>         *plus = '+';
>         f->old_addr += offset;
>     }
> 
> 
> May be invoked for symbols that that don't have the '+' in them.

I don't see how it would. This

    plus = strchr(f->name, '+');

comes ahead of any paths leading to the code you quote.

>> > +    /* +<offset> */
>> > +    plus = strchr(f->name, '+');
>> 
>> And I think you should prefer using the local variable here.
> 
> <nods>
> 
>> 
>> Furthermore you're losing const here - does f->name really point
>> to memory that doesn't get mapped r/o?
> 
> Yes.
> 
> The 'struct livepatch_func' contains the the ->opaque array of 31 bytes
> (from which we use 5 bytes) which the hypervisor uses to stash the original
> instructions.

How does the patch name end up in (5 bytes of) the opaque field?
In any event the correctness of deliberately stripping const should
be explained in a comment (if, of course, it can't be avoided in the
first place).

>> Overall - are you sure you want to disallow symbol names containing
>> + characters? I.e. you don't want to add support for some form of
>> quoting?
> 
> Can you actually have + in a function or object?

Why not? The ELF spec, iirc, doesn't put any restrictions on what
characters (other than nul of course) can be used in symbol names.
gas actually has received full quoting support a year or two ago,
to no longer needlessly restrict the character set available here.

Jan


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

Re: [PATCH v4 7/9] livepatch: NOP if func->new_[addr] is zero.

[Jan Beulich]

>>> 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


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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Jan Beulich]

>>> On 06.09.16 at 18:51, <konrad.wilk@oracle.com> wrote:
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -86,7 +86,16 @@ 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_EXECINSTR) &&
> +                  (sec[i].sec->sh_flags & SHF_WRITE) &&
> +                  sec[i].sec->sh_type == SHT_NOBITS &&
> +                  sec[i].sec->sh_size > MB(2) )

What good do the two sh_flags checks do here?

Jan


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

Re: [PATCH v4 9/9] livepach: Add .livepatch.hooks functions and test-case

[Konrad Rzeszutek Wilk]

On Tue, Sep 06, 2016 at 07:25:23PM +0100, Andrew Cooper wrote:
> On 06/09/16 18:22, Konrad Rzeszutek Wilk wrote:
> > On Tue, Aug 23, 2016 at 10:22:12PM -0400, Konrad Rzeszutek Wilk wrote:
> >> 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.
> >>
> >> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> >> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >
> > So... anybody willing to review it :-)
> >
> >> ---
> >> 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>
> >
> > In regards to this going in v4.8 my recollection is that:
> >
> >  George: 0
> >  Andrew: +1
> >  Jan: 0 (with a slight leaning towards -1)?
> >
> > I think that means folks are OK or 'don't care'.
> >
> > And the livepatch maintainers:
> >  Ross: +1 (obviously since he wrote it)
> >  Konrad: +1
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thank you!

If nobody objects I will push this patch (along with some
other ones that have been Reviewed) on Friday morning.


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

Re: [PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>

[Konrad Rzeszutek Wilk]

On Wed, Sep 07, 2016 at 02:10:43AM -0600, Jan Beulich wrote:
> >>> On 06.09.16 at 21:56, <konrad.wilk@oracle.com> wrote:
> > On Wed, Aug 24, 2016 at 03:08:01AM -0600, Jan Beulich wrote:
> >> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> >> > --- a/xen/common/livepatch.c
> >> > +++ b/xen/common/livepatch.c
> >> > @@ -237,13 +237,34 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
> >> >  static int resolve_old_address(struct livepatch_func *f,
> >> >                                 const struct livepatch_elf *elf)
> >> >  {
> >> > +    const char *s;
> >> > +    char *plus = NULL;
> >> 
> >> Pointless initializer.
> > 
> > We need that otherwise this part (which is at the bottom of this function):
> > 
> >     if ( plus )
> >     {
> >         *plus = '+';
> >         f->old_addr += offset;
> >     }
> > 
> > 
> > May be invoked for symbols that that don't have the '+' in them.
> 
> I don't see how it would. This
> 
>     plus = strchr(f->name, '+');
> 
> comes ahead of any paths leading to the code you quote.

Ah. Stale information - the earlier patch had 'slash' and 'plus' variables
to look for - and that was why I needed it.

But with the code you are quoting - it is not needed.
> 
> >> > +    /* +<offset> */
> >> > +    plus = strchr(f->name, '+');
> >> 
> >> And I think you should prefer using the local variable here.
> > 
> > <nods>
> > 
> >> 
> >> Furthermore you're losing const here - does f->name really point
> >> to memory that doesn't get mapped r/o?
> > 
> > Yes.
> > 
> > The 'struct livepatch_func' contains the the ->opaque array of 31 bytes
> > (from which we use 5 bytes) which the hypervisor uses to stash the original
> > instructions.
> 
> How does the patch name end up in (5 bytes of) the opaque field?

I was (ineptly) saying that the struct livepatch_func has fields that are
modified, hence it ends up in .data section.

Wait a minute. The f->name should have a relocation to point to .rodata
instead of .data! And that should have crashed when I modified it.

Ah, they are all 'static char name[] = "blah"' instead of
'static const char name[] = "blah"'.

Patch queued up.

> In any event the correctness of deliberately stripping const should
> be explained in a comment (if, of course, it can't be avoided in the
> first place).
> 
> >> Overall - are you sure you want to disallow symbol names containing
> >> + characters? I.e. you don't want to add support for some form of
> >> quoting?
> > 
> > Can you actually have + in a function or object?
> 
> Why not? The ELF spec, iirc, doesn't put any restrictions on what
> characters (other than nul of course) can be used in symbol names.
> gas actually has received full quoting support a year or two ago,
> to no longer needlessly restrict the character set available here.

I was thinking of + in the C land. But that is irrelevant to this
discussion.

Let me dig in the gas code to find examples of this - but in the
meantime (and if you recall), you meant something like this:

"do_domain_pause+something"

?
Which would mean for offset purposes I would need to deal with:

"do_domain_pause+something"+0x10

or
'do_domain_pause+something'+0x10



> 
> Jan
> 

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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Konrad Rzeszutek Wilk]

On Wed, Sep 07, 2016 at 02:02:44AM -0600, Jan Beulich wrote:
> >>> On 06.09.16 at 18:47, <konrad.wilk@oracle.com> wrote:
> > On Wed, Aug 24, 2016 at 02:55:21AM -0600, Jan Beulich wrote:
> >> >>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
> >> > --- a/xen/common/livepatch.c
> >> > +++ b/xen/common/livepatch.c
> >> > @@ -70,6 +70,9 @@ 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). */
> >> > +    void **bss;                          /* .bss's of the payload. */
> >> > +    size_t *bss_size;                    /* and their sizes. */
> >> 
> >> Is size_t wide enough in the extreme case? Perhaps yes, because I
> >> don't think we'll ever load 64-bit ELF on a 32-bit platform.
> > 
> > Nonethless having a huge .bss is a kind of extreme? Perhaps we should
> > have an seperate patch that checks the SHT_NOBITS and disallows .bss's
> > bigger than say 2MB?
> 
> Well, the extra check certainly wouldn't hurt, but I think before
> hitting the size_t limit you'd run out of address space to place
> the payload in (as that's iirc a less than 1Gb area).

True. And on ARM 32|64 even smaller (2MB). Let me force an
even smaller width type - say 'unsigned int'.
> 
> Jan
> 

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

Re: [PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>

[Jan Beulich]

>>> On 08.09.16 at 11:22, <konrad.wilk@oracle.com> wrote:
> On Wed, Sep 07, 2016 at 02:10:43AM -0600, Jan Beulich wrote:
>> >>> On 06.09.16 at 21:56, <konrad.wilk@oracle.com> wrote:
>> > On Wed, Aug 24, 2016 at 03:08:01AM -0600, Jan Beulich wrote:
>> >> Overall - are you sure you want to disallow symbol names containing
>> >> + characters? I.e. you don't want to add support for some form of
>> >> quoting?
>> > 
>> > Can you actually have + in a function or object?
>> 
>> Why not? The ELF spec, iirc, doesn't put any restrictions on what
>> characters (other than nul of course) can be used in symbol names.
>> gas actually has received full quoting support a year or two ago,
>> to no longer needlessly restrict the character set available here.
> 
> I was thinking of + in the C land. But that is irrelevant to this
> discussion.
> 
> Let me dig in the gas code to find examples of this - but in the
> meantime (and if you recall), you meant something like this:
> 
> "do_domain_pause+something"
> 
> ?

Yes.

> Which would mean for offset purposes I would need to deal with:
> 
> "do_domain_pause+something"+0x10
> 
> or
> 'do_domain_pause+something'+0x10

Yes. Or, as said - at least queue it up as a work item, as I certainly
agree it's not the highest priority thing to deal with right away.

Jan


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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Ross Lagerwall]

On 08/24/2016 03:22 AM, Konrad Rzeszutek Wilk wrote:
> So that when we apply the patch again the .bss is cleared.
> Otherwise we may find some variables containing old values.
>
> The payloads may contain various .bss - especially if -fdata-sections
> is used which can create .bss.<name> sections.
>

After having thought about this again, I'm not sure it makes much sense. 
Any data sections in the payload are not reset to their initial values, 
so resetting the bss only may result in an unexpected combination of new 
& old data/bss.

Perhaps it just needs to be documented that a payload's bss/data is 
untouched across revert/apply?

-- 
Ross Lagerwall

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

Re: [PATCH v4 4/9] version: Print build-id at bootup.

[Ross Lagerwall]

On 09/06/2016 05:57 PM, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 24, 2016 at 02:58:48AM -0600, Jan Beulich wrote:
>>>>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
>>> Livepatch expected at some point to be able to print the
>>> build-id during bootup, which it did not.  The reason is
>>> that xen_build_init and livepatch_init are both __initcall
>>> type routines. This meant that when livepatch_init called
>>> xen_build_id, it would return -ENODATA as build_id_len was
>>> not setup yet (b/c xen_build_init would be called later).
>>>
>>> The original patch fixed this by calling xen_build_init in
>>> livepatch_init which allows us to print the build-id of
>>> the hypervisor.
>>>
>>> However the x86 maintainers pointed out that build-id
>>> is independent of Livepatch and in fact should print
>>> regardless whether Livepatch is enabled or not.
>>>
>>> Therefore this patch moves the logic of printing the build-id
>>> to version.c.
>>>
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> Thank you.
>
> I had a slight change due to rebasing on "x86/EFI: use less crude a way of generating the build"
> which was quite simple to fix so I retained your Reviewed-by tag.
>
> The final patch looks as so:
>
> From 927c9accac9a49b586f616060e5567d7b03e3e77 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Tue, 6 Sep 2016 12:18:10 -0400
> Subject: [PATCH] version: Print build-id at bootup.
>
> Livepatch expected at some point to be able to print the
> build-id during bootup, which it did not.  The reason is
> that xen_build_init and livepatch_init are both __initcall
> type routines. This meant that when livepatch_init called
> xen_build_id, it would return -ENODATA as build_id_len was
> not setup yet (b/c xen_build_init would be called later).
>
> The original patch fixed this by calling xen_build_init in
> livepatch_init which allows us to print the build-id of
> the hypervisor.
>
> However the x86 maintainers pointed out that build-id
> is independent of Livepatch and in fact should print
> regardless whether Livepatch is enabled or not.
>
> Therefore this patch moves the logic of printing the build-id
> to version.c.
>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

-- 
Ross Lagerwall

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

Re: [PATCH v4 8/9] symbols: Generate an xen-sym.map

[Ross Lagerwall]

On 08/24/2016 03:22 AM, Konrad Rzeszutek Wilk wrote:
> You could construct _most_ of the names of the functions
> by doing 'nm --defined' but unfortunatly you do not get the
> <file> prefix that is added on in Xen . For example:
>
> $ cat xen-syms.symbols |grep do_domain_pause
> 0xffff82d080104920 t domain.c#do_domain_pause
> $ nm --defined xen-syms|grep do_domain_pause
> ffff82d080104920 t do_domain_pause
>
> This is normally not an issue, but if one is doing livepatching and
> wants during build-time verify that the symbols the livepatch payloads
> will patch do correspond to the one the hypervisor has built - this helps a lot.
>
> Note that during runtime one can do:
> [root@localhost xen]# cat /proc/xen/xensyms |grep do_domain_pause
> ffff82d080104920 t domain.c#do_domain_pause
>
> But one may not want to build and verify a livepatch on the same host.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---

Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>

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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Konrad Rzeszutek Wilk]

On Fri, Sep 09, 2016 at 02:33:18PM +0100, Ross Lagerwall wrote:
> On 08/24/2016 03:22 AM, Konrad Rzeszutek Wilk wrote:
> > So that when we apply the patch again the .bss is cleared.
> > Otherwise we may find some variables containing old values.
> > 
> > The payloads may contain various .bss - especially if -fdata-sections
> > is used which can create .bss.<name> sections.
> > 
> 
> After having thought about this again, I'm not sure it makes much sense. Any
> data sections in the payload are not reset to their initial values, so
> resetting the bss only may result in an unexpected combination of new & old
> data/bss.

Regardless of that I think clearing the .bss upon applying the livepatch is
still the right thing to do.

Regarding of the .data - we could have a copy of the .data the first time
we load - and then during application copy over it from the original one?.

> 
> Perhaps it just needs to be documented that a payload's bss/data is
> untouched across revert/apply?

It really cuts down on bugs if we clear the .bss. It is kind of ingrained in every
developer that the .bss is zero-ed out at startup.
> 
> -- 
> Ross Lagerwall

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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Ross Lagerwall]

On 09/09/2016 02:50 PM, Konrad Rzeszutek Wilk wrote:
> On Fri, Sep 09, 2016 at 02:33:18PM +0100, Ross Lagerwall wrote:
>> On 08/24/2016 03:22 AM, Konrad Rzeszutek Wilk wrote:
>>> So that when we apply the patch again the .bss is cleared.
>>> Otherwise we may find some variables containing old values.
>>>
>>> The payloads may contain various .bss - especially if -fdata-sections
>>> is used which can create .bss.<name> sections.
>>>
>>
>> After having thought about this again, I'm not sure it makes much sense. Any
>> data sections in the payload are not reset to their initial values, so
>> resetting the bss only may result in an unexpected combination of new & old
>> data/bss.
>
> Regardless of that I think clearing the .bss upon applying the livepatch is
> still the right thing to do.
>
> Regarding of the .data - we could have a copy of the .data the first time
> we load - and then during application copy over it from the original one?.
>
>>
>> Perhaps it just needs to be documented that a payload's bss/data is
>> untouched across revert/apply?
>
> It really cuts down on bugs if we clear the .bss. It is kind of ingrained in every
> developer that the .bss is zero-ed out at startup.
>>

Sure.

IMO clearing one but not resetting the other is even more unexpected. 
However, if we agree that it is desirable to do both, then this patch is 
acceptable as a step in the right direction.

-- 
Ross Lagerwall

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

Re: [PATCH v4 6/9] livepatch: Add parsing for the symbol+0x<offset>

[Ross Lagerwall]

On 09/08/2016 10:22 AM, Konrad Rzeszutek Wilk wrote:
> On Wed, Sep 07, 2016 at 02:10:43AM -0600, Jan Beulich wrote:
>>>>> On 06.09.16 at 21:56, <konrad.wilk@oracle.com> wrote:
>>> On Wed, Aug 24, 2016 at 03:08:01AM -0600, Jan Beulich wrote:
>>>>>>> On 24.08.16 at 04:22, <konrad.wilk@oracle.com> wrote:
>>>>> --- a/xen/common/livepatch.c
>>>>> +++ b/xen/common/livepatch.c
>>>>> @@ -237,13 +237,34 @@ static const char *livepatch_symbols_lookup(unsigned long addr,
>>>>>  static int resolve_old_address(struct livepatch_func *f,
>>>>>                                 const struct livepatch_elf *elf)
>>>>>  {
>>>>> +    const char *s;
>>>>> +    char *plus = NULL;
>>>>
>>>> Pointless initializer.
>>>
>>> We need that otherwise this part (which is at the bottom of this function):
>>>
>>>     if ( plus )
>>>     {
>>>         *plus = '+';
>>>         f->old_addr += offset;
>>>     }
>>>
>>>
>>> May be invoked for symbols that that don't have the '+' in them.
>>
>> I don't see how it would. This
>>
>>     plus = strchr(f->name, '+');
>>
>> comes ahead of any paths leading to the code you quote.
>
> Ah. Stale information - the earlier patch had 'slash' and 'plus' variables
> to look for - and that was why I needed it.
>
> But with the code you are quoting - it is not needed.
>>
>>>>> +    /* +<offset> */
>>>>> +    plus = strchr(f->name, '+');
>>>>
>>>> And I think you should prefer using the local variable here.
>>>
>>> <nods>
>>>
>>>>
>>>> Furthermore you're losing const here - does f->name really point
>>>> to memory that doesn't get mapped r/o?
>>>
>>> Yes.
>>>
>>> The 'struct livepatch_func' contains the the ->opaque array of 31 bytes
>>> (from which we use 5 bytes) which the hypervisor uses to stash the original
>>> instructions.
>>
>> How does the patch name end up in (5 bytes of) the opaque field?
>
> I was (ineptly) saying that the struct livepatch_func has fields that are
> modified, hence it ends up in .data section.
>
> Wait a minute. The f->name should have a relocation to point to .rodata
> instead of .data! And that should have crashed when I modified it.
>

livepatch-build-tools puts the function names in a read only section 
(.livepatch.strings), so the approach of fiddling with f->name probably 
needs to change.

-- 
Ross Lagerwall

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

Re: [PATCH v4 1/9] livepatch: Clear .bss when payload is reverted

[Jan Beulich]

>>> On 09.09.16 at 15:50, <konrad.wilk@oracle.com> wrote:
> On Fri, Sep 09, 2016 at 02:33:18PM +0100, Ross Lagerwall wrote:
>> On 08/24/2016 03:22 AM, Konrad Rzeszutek Wilk wrote:
>> > So that when we apply the patch again the .bss is cleared.
>> > Otherwise we may find some variables containing old values.
>> > 
>> > The payloads may contain various .bss - especially if -fdata-sections
>> > is used which can create .bss.<name> sections.
>> > 
>> 
>> After having thought about this again, I'm not sure it makes much sense. Any
>> data sections in the payload are not reset to their initial values, so
>> resetting the bss only may result in an unexpected combination of new & old
>> data/bss.
> 
> Regardless of that I think clearing the .bss upon applying the livepatch is
> still the right thing to do.
> 
> Regarding of the .data - we could have a copy of the .data the first time
> we load - and then during application copy over it from the original one?.
> 
>> 
>> Perhaps it just needs to be documented that a payload's bss/data is
>> untouched across revert/apply?
> 
> It really cuts down on bugs if we clear the .bss. It is kind of ingrained in every
> developer that the .bss is zero-ed out at startup.

I don't think Ross meant to suggest to not clear it at all: Clearing it
at load time is the equivalent of loading actual data into initialized
sections.

Jan


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