livepatch: Disallow applying after an revert

On general this is unhealthy - as the payload's .bss (definitly)
or .data (maybe) will be modified once the payload is running.

Doing an revert and then re-applying the payload with a non-pristine
.bss or .data can lead to unforseen consequences (.bss are assumed
to always contain zero value but now they may have a different value).

There is one exception - if the payload contains only one .data section
- the .livepatch.funcs, then it is OK to re-apply an revert.
We detect this rather simply (if there is one RW section and its name
is .livepatch.funcs) - but the payload may have many other RW sections
that are not used at all (such as .bss or .data sections with zero
length). To not account those we also ignore sections with sh_size
being zero.

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

diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown
index 89c10507723e1fb66296a2d68c881b8ca75f4d0d..a674037e1566e8c1396f2f2c308095eed2a2fb28 100644 (file)
--- a/docs/misc/livepatch.markdown
+++ b/docs/misc/livepatch.markdown
@@ -1061,6 +1061,13 @@ depending on the current state of data. As such it should not be attempted.
 That said we should provide hook functions so that the existing data
 can be changed during payload application.
 
+To guarantee safety we disallow re-applying an payload after it has been
+reverted. This is because we cannot guarantee that the state of .bss
+and .data to be exactly as it was during loading. Hence the administrator
+MUST unload the payload and upload it again to apply it.
+
+There is an exception to this: if the payload only has .livepatch.funcs;
+and the .data or .bss sections are of zero length.
 
 ### Inline patching
 

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 23e4d515223d20689ac113f1fdf16cef5ebf03d8..912729e96957ba92dc8e596f740c2fcbf6867217 100644 (file)
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -52,6 +52,8 @@ struct livepatch_build_id {
 struct payload {
     uint32_t state;                      /* One of the LIVEPATCH_STATE_*. */
     int32_t rc;                          /* 0 or -XEN_EXX. */
+    bool reverted;                       /* Whether it was reverted. */
+    bool safe_to_reapply;                /* Can apply safely after revert. */
     struct list_head list;               /* Linked to 'payload_list'. */
     const void *text_addr;               /* Virtual address of .text. */
     size_t text_size;                    /* .. and its size. */
@@ -308,7 +310,7 @@ static void calc_section(const struct livepatch_elf_sec *sec, size_t *size,
 static int move_payload(struct payload *payload, struct livepatch_elf *elf)
 {
     void *text_buf, *ro_buf, *rw_buf;
-    unsigned int i;
+    unsigned int i, rw_buf_sec, rw_buf_cnt = 0;
     size_t size = 0;
     unsigned int *offset;
     int rc = 0;
@@ -325,8 +327,11 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
          * and .shstrtab. For the non-relocate we allocate and copy these
          * via other means - and the .rel we can ignore as we only use it
          * once during loading.
+         *
+         * Also ignore sections with zero size. Those can be for example:
+         * data, or .bss.
          */
-        if ( !(elf->sec[i].sec->sh_flags & SHF_ALLOC) )
+        if ( livepatch_elf_ignore_section(elf->sec[i].sec) )
             offset[i] = UINT_MAX;
         else if ( (elf->sec[i].sec->sh_flags & SHF_EXECINSTR) &&
                    !(elf->sec[i].sec->sh_flags & SHF_WRITE) )
@@ -374,14 +379,18 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
 
     for ( i = 1; i < elf->hdr->e_shnum; i++ )
     {
-        if ( elf->sec[i].sec->sh_flags & SHF_ALLOC )
+        if ( !livepatch_elf_ignore_section(elf->sec[i].sec) )
         {
             void *buf;
 
             if ( elf->sec[i].sec->sh_flags & SHF_EXECINSTR )
                 buf = text_buf;
             else if ( elf->sec[i].sec->sh_flags & SHF_WRITE )
+            {
                 buf = rw_buf;
+                rw_buf_sec = i;
+                rw_buf_cnt++;
+            }
             else
                 buf = ro_buf;
 
@@ -402,6 +411,10 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
         }
     }
 
+    /* Only one RW section with non-zero size: .livepatch.funcs */
+    if ( rw_buf_cnt == 1 &&
+         !strcmp(elf->sec[rw_buf_sec].name, ELF_LIVEPATCH_FUNC) )
+        payload->safe_to_reapply = true;
  out:
     xfree(offset);
 
@@ -1057,6 +1070,7 @@ static int revert_payload(struct payload *data)
     list_del_rcu(&data->applied_list);
     unregister_virtual_region(&data->region);
 
+    data->reverted = true;
     return 0;
 }
 
@@ -1438,6 +1452,20 @@ static int livepatch_action(xen_sysctl_livepatch_action_t *action)
     case LIVEPATCH_ACTION_APPLY:
         if ( data->state == LIVEPATCH_STATE_CHECKED )
         {
+            /*
+             * It is unsafe to apply an reverted payload as the .data (or .bss)
+             * may not be in in pristine condition. Hence MUST unload and then
+             * apply patch again. Unless the payload has only one
+             * RW section (.livepatch.funcs).
+             */
+            if ( data->reverted && !data->safe_to_reapply )
+            {
+                dprintk(XENLOG_ERR, "%s%s: can't revert as payload has .data. Please unload!\n",
+                        LIVEPATCH, data->name);
+                data->rc = -EINVAL;
+                break;
+            }
+
             rc = build_id_dep(data, !!list_empty(&applied_list));
             if ( rc )
                 break;

diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index cda9b2730eeb7289c4bb1d4a7e9fb4d229dfcb6d..6c7773bf75b2df681ea7d753c6974644b353db3a 100644 (file)
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -310,8 +310,7 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf *elf)
                 break;
             }
 
-            /* Matches 'move_payload' which ignores such sections. */
-            if ( !(elf->sec[idx].sec->sh_flags & SHF_ALLOC) )
+            if ( livepatch_elf_ignore_section(elf->sec[idx].sec) )
                 break;
 
             st_value += (unsigned long)elf->sec[idx].load_addr;

diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
index 7e7c86eb5ab8d0fa2bc4182ce48b9a18d3875501..9ad499ee8b0fff83a94346584a21effe2712f68b 100644 (file)
--- a/xen/include/xen/livepatch_elf.h
+++ b/xen/include/xen/livepatch_elf.h
@@ -46,6 +46,10 @@ void livepatch_elf_free(struct livepatch_elf *elf);
 int livepatch_elf_resolve_symbols(struct livepatch_elf *elf);
 int livepatch_elf_perform_relocs(struct livepatch_elf *elf);
 
+static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec)
+{
+    return !(sec->sh_flags & SHF_ALLOC) || sec->sh_size == 0;
+}
 #endif /* __XEN_LIVEPATCH_ELF_H__ */
 
 /*