[PATCH v2 0/6] xen/livepatch: improvements to loading

[PATCH v2 0/6] xen/livepatch: improvements to loading

[Roger Pau Monne]

Hello,

Series started as a bugfix to do the build-id checks earlier, but has
expanded a bit into alternatives also.

Thanks, Roger.

Roger Pau Monne (6):
  xen/livepatch: remove useless check for duplicated sections
  xen/livepatch: zero pointer to temporary load buffer
  xen/livepatch: simplify and unify logic in prepare_payload()
  xen/livepatch: do Xen build-id check earlier
  x86/alternatives: do not BUG during apply
  x86/alternative: build time check feature is in range

 xen/arch/x86/alternative.c                 |  46 +++--
 xen/arch/x86/include/asm/alternative-asm.h |   3 +
 xen/arch/x86/include/asm/alternative.h     |   6 +-
 xen/common/livepatch.c                     | 194 +++++++++++----------
 xen/include/xen/livepatch_payload.h        |   1 -
 5 files changed, 146 insertions(+), 104 deletions(-)

-- 
2.46.0

[PATCH v2 1/6] xen/livepatch: remove useless check for duplicated sections

[Roger Pau Monne]

The current check for duplicated sections in a payload is not effective.  Such
check is done inside a loop that iterates over the sections names, it's
logically impossible for the bitmap to be set more than once.

The usage of a bitmap in check_patching_sections() has been replaced with a
boolean, since the function just cares that at least one of the special
sections is present.

No functional change intended, as the check was useless.

Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.')
Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/common/livepatch.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d93a556bcda2..df41dcce970a 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -473,7 +473,6 @@ static int check_special_sections(const struct livepatch_elf *elf)
     static const char *const names[] = { ELF_LIVEPATCH_DEPENDS,
                                          ELF_LIVEPATCH_XEN_DEPENDS,
                                          ELF_BUILD_ID_NOTE};
-    DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 };
 
     for ( i = 0; i < ARRAY_SIZE(names); i++ )
     {
@@ -493,13 +492,6 @@ static int check_special_sections(const struct livepatch_elf *elf)
                    elf->name, names[i]);
             return -EINVAL;
         }
-
-        if ( test_and_set_bit(i, found) )
-        {
-            printk(XENLOG_ERR LIVEPATCH "%s: %s was seen more than once\n",
-                   elf->name, names[i]);
-            return -EINVAL;
-        }
     }
 
     return 0;
@@ -517,7 +509,7 @@ static int check_patching_sections(const struct livepatch_elf *elf)
                                          ELF_LIVEPATCH_PREREVERT_HOOK,
                                          ELF_LIVEPATCH_REVERT_HOOK,
                                          ELF_LIVEPATCH_POSTREVERT_HOOK};
-    DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 };
+    bool found = false;
 
     /*
      * The patching sections are optional, but at least one
@@ -544,16 +536,11 @@ static int check_patching_sections(const struct livepatch_elf *elf)
             return -EINVAL;
         }
 
-        if ( test_and_set_bit(i, found) )
-        {
-            printk(XENLOG_ERR LIVEPATCH "%s: %s was seen more than once\n",
-                   elf->name, names[i]);
-            return -EINVAL;
-        }
+        found = true;
     }
 
     /* Checking if at least one section is present. */
-    if ( bitmap_empty(found, ARRAY_SIZE(names)) )
+    if ( !found )
     {
         printk(XENLOG_ERR LIVEPATCH "%s: Nothing to patch. Aborting...\n",
                elf->name);
-- 
2.46.0

[PATCH v2 2/6] xen/livepatch: zero pointer to temporary load buffer

[Roger Pau Monne]

The livepatch_elf_sec data field points to the temporary load buffer, it's the
load_addr field that points to the stable loaded section data.  Zero the data
field once load_addr is set, as it would otherwise become a dangling pointer
once the load buffer is freed.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/common/livepatch.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index df41dcce970a..87b3db03e26d 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -383,6 +383,9 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
             }
             else
                 memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
+
+            /* Avoid leaking pointers to temporary load buffers. */
+            elf->sec[i].data = NULL;
         }
     }
 
-- 
2.46.0

[PATCH v2 3/6] xen/livepatch: simplify and unify logic in prepare_payload()

[Roger Pau Monne]

The following sections: .note.gnu.build-id, .livepatch.xen_depends and
.livepatch.depends are mandatory and ensured to be present by
check_special_sections() before prepare_payload() is called.

Simplify the logic in prepare_payload() by introducing a generic function to
parse the sections that contain a buildid.  Note the function assumes the
buildid related section to always be present.

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Rename.
 - Change order of assert.
---
 xen/common/livepatch.c | 110 +++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 60 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 87b3db03e26d..8e61083f23a7 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -470,6 +470,31 @@ static int xen_build_id_dep(const struct payload *payload)
     return 0;
 }
 
+/* Parses build-id sections into the given destination. */
+static int parse_buildid(const struct livepatch_elf_sec *sec,
+                         struct livepatch_build_id *id)
+{
+    const Elf_Note *n;
+    int rc;
+
+    /* Presence of the sections is ensured by check_special_sections(). */
+    ASSERT(sec);
+
+    n = sec->load_addr;
+
+    if ( sec->sec->sh_size <= sizeof(*n) )
+        return -EINVAL;
+
+    rc = xen_build_id_check(n, sec->sec->sh_size, &id->p, &id->len);
+    if ( rc )
+        return rc;
+
+    if ( !id->len || !id->p )
+        return -EINVAL;
+
+   return 0;
+}
+
 static int check_special_sections(const struct livepatch_elf *elf)
 {
     unsigned int i;
@@ -641,11 +666,12 @@ static int prepare_payload(struct payload *payload,
                            struct livepatch_elf *elf)
 {
     const struct livepatch_elf_sec *sec;
+    const struct payload *data;
     unsigned int i;
     struct livepatch_func *funcs;
     struct livepatch_func *f;
     struct virtual_region *region;
-    const Elf_Note *n;
+    int rc;
 
     sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_FUNC);
     if ( sec )
@@ -663,8 +689,6 @@ static int prepare_payload(struct payload *payload,
 
         for ( i = 0; i < payload->nfuncs; i++ )
         {
-            int rc;
-
             f = &(funcs[i]);
 
             if ( f->version != LIVEPATCH_PAYLOAD_VERSION )
@@ -707,69 +731,35 @@ static int prepare_payload(struct payload *payload,
     LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.action, ELF_LIVEPATCH_REVERT_HOOK);
     LIVEPATCH_ASSIGN_SINGLE_HOOK(elf, payload->hooks.revert.post, ELF_LIVEPATCH_POSTREVERT_HOOK);
 
-    sec = livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE);
-    if ( sec )
-    {
-        const struct payload *data;
-
-        n = sec->load_addr;
-
-        if ( sec->sec->sh_size <= sizeof(*n) )
-            return -EINVAL;
-
-        if ( xen_build_id_check(n, sec->sec->sh_size,
-                                &payload->id.p, &payload->id.len) )
-            return -EINVAL;
-
-        if ( !payload->id.len || !payload->id.p )
-            return -EINVAL;
+    rc = parse_buildid(livepatch_elf_sec_by_name(elf, ELF_BUILD_ID_NOTE),
+                       &payload->id);
+    if ( rc )
+        return rc;
 
-        /* Make sure it is not a duplicate. */
-        list_for_each_entry ( data, &payload_list, list )
+    /* Make sure it is not a duplicate. */
+    list_for_each_entry ( data, &payload_list, list )
+    {
+        /* No way _this_ payload is on the list. */
+        ASSERT(data != payload);
+        if ( data->id.len == payload->id.len &&
+             !memcmp(data->id.p, payload->id.p, data->id.len) )
         {
-            /* No way _this_ payload is on the list. */
-            ASSERT(data != payload);
-            if ( data->id.len == payload->id.len &&
-                 !memcmp(data->id.p, payload->id.p, data->id.len) )
-            {
-                dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Already loaded as %s!\n",
-                        elf->name, data->name);
-                return -EEXIST;
-            }
+            dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Already loaded as %s!\n",
+                    elf->name, data->name);
+            return -EEXIST;
         }
     }
 
-    sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_DEPENDS);
-    if ( sec )
-    {
-        n = sec->load_addr;
-
-        if ( sec->sec->sh_size <= sizeof(*n) )
-            return -EINVAL;
-
-        if ( xen_build_id_check(n, sec->sec->sh_size,
-                                &payload->dep.p, &payload->dep.len) )
-            return -EINVAL;
-
-        if ( !payload->dep.len || !payload->dep.p )
-            return -EINVAL;
-    }
-
-    sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
-    if ( sec )
-    {
-        n = sec->load_addr;
-
-        if ( sec->sec->sh_size <= sizeof(*n) )
-            return -EINVAL;
-
-        if ( xen_build_id_check(n, sec->sec->sh_size,
-                                &payload->xen_dep.p, &payload->xen_dep.len) )
-            return -EINVAL;
+    rc = parse_buildid(livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_DEPENDS),
+                       &payload->dep);
+    if ( rc )
+        return rc;
 
-        if ( !payload->xen_dep.len || !payload->xen_dep.p )
-            return -EINVAL;
-    }
+    rc = parse_buildid(livepatch_elf_sec_by_name(elf,
+                                                 ELF_LIVEPATCH_XEN_DEPENDS),
+                       &payload->xen_dep);
+    if ( rc )
+        return rc;
 
     /* Setup the virtual region with proper data. */
     region = &payload->region;
-- 
2.46.0

[PATCH v2 4/6] xen/livepatch: do Xen build-id check earlier

[Roger Pau Monne]

The check against the expected Xen build ID should be done ahead of attempting
to apply the alternatives contained in the livepatch.

If the CPUID in the alternatives patching data is out of the scope of the
running Xen featureset the BUG() in _apply_alternatives() will trigger thus
bringing the system down.  Note the layout of struct alt_instr could also
change between versions.  It's also possible for struct exception_table_entry
to have changed format, hence leading to other kind of errors if parsing of the
payload is done ahead of checking if the Xen build-id matches.

Move the Xen build ID check as early as possible.  To do so introduce a new
check_xen_buildid() function that parses and checks the Xen build-id before
moving the payload.  Since the expected Xen build-id is used early to
detect whether the livepatch payload could be loaded, there's no reason to
store it in the payload struct, as a non-matching Xen build-id won't get the
payload populated in the first place.

Note parse_buildid() has to be slightly adjusted to allow fetching the section
data from the 'data' field instead of the 'load_addr' one: with the Xen build
ID moved ahead of move_payload() 'load_addr' is not yet set when the Xen build
ID check is performed.  Also printing the expected Xen build ID has part of
dumping the payload is no longer done, as all loaded payloads would have Xen
build IDs matching the running Xen, otherwise they would have failed to load.

Fixes: 879615f5db1d ('livepatch: Always check hypervisor build ID upon livepatch upload')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Do the Xen build-id check even earlier.
---
 xen/common/livepatch.c              | 66 +++++++++++++++++++----------
 xen/include/xen/livepatch_payload.h |  1 -
 2 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 8e61083f23a7..895c425cd5ea 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -448,24 +448,21 @@ static bool section_ok(const struct livepatch_elf *elf,
     return true;
 }
 
-static int xen_build_id_dep(const struct payload *payload)
+static int xen_build_id_dep(const struct livepatch_build_id *expected)
 {
     const void *id = NULL;
     unsigned int len = 0;
     int rc;
 
-    ASSERT(payload->xen_dep.len);
-    ASSERT(payload->xen_dep.p);
+    ASSERT(expected->len);
+    ASSERT(expected->p);
 
     rc = xen_build_id(&id, &len);
     if ( rc )
         return rc;
 
-    if ( payload->xen_dep.len != len || memcmp(id, payload->xen_dep.p, len) ) {
-        printk(XENLOG_ERR LIVEPATCH "%s: check against hypervisor build-id failed\n",
-               payload->name);
+    if ( expected->len != len || memcmp(id, expected->p, len) )
         return -EINVAL;
-    }
 
     return 0;
 }
@@ -480,7 +477,8 @@ static int parse_buildid(const struct livepatch_elf_sec *sec,
     /* Presence of the sections is ensured by check_special_sections(). */
     ASSERT(sec);
 
-    n = sec->load_addr;
+    /* Possibly use the temporary load buffer if load_addr isn't yet set. */
+    n = sec->load_addr ?: sec->data;
 
     if ( sec->sec->sh_size <= sizeof(*n) )
         return -EINVAL;
@@ -495,11 +493,44 @@ static int parse_buildid(const struct livepatch_elf_sec *sec,
    return 0;
 }
 
+static int check_xen_buildid(const struct livepatch_elf *elf)
+{
+    struct livepatch_build_id id;
+    const struct livepatch_elf_sec *sec =
+        livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
+    int rc;
+
+    if ( !sec )
+    {
+        printk(XENLOG_ERR LIVEPATCH "%s: %s is missing\n",
+               elf->name, ELF_LIVEPATCH_XEN_DEPENDS);
+        return -EINVAL;
+    }
+
+    rc = parse_buildid(sec, &id);
+    if ( rc )
+    {
+        printk(XENLOG_ERR LIVEPATCH "%s: failed to parse build-id at %s: %d\n",
+               elf->name, ELF_LIVEPATCH_XEN_DEPENDS, rc);
+        return -EINVAL;
+    }
+
+    rc = xen_build_id_dep(&id);
+    if ( rc )
+    {
+        printk(XENLOG_ERR LIVEPATCH
+               "%s: check against hypervisor build-id failed: %d\n",
+               elf->name, rc);
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 static int check_special_sections(const struct livepatch_elf *elf)
 {
     unsigned int i;
     static const char *const names[] = { ELF_LIVEPATCH_DEPENDS,
-                                         ELF_LIVEPATCH_XEN_DEPENDS,
                                          ELF_BUILD_ID_NOTE};
 
     for ( i = 0; i < ARRAY_SIZE(names); i++ )
@@ -755,12 +786,6 @@ static int prepare_payload(struct payload *payload,
     if ( rc )
         return rc;
 
-    rc = parse_buildid(livepatch_elf_sec_by_name(elf,
-                                                 ELF_LIVEPATCH_XEN_DEPENDS),
-                       &payload->xen_dep);
-    if ( rc )
-        return rc;
-
     /* Setup the virtual region with proper data. */
     region = &payload->region;
 
@@ -1069,6 +1094,10 @@ static int load_payload_data(struct payload *payload, void *raw, size_t len)
     if ( rc )
         goto out;
 
+    rc = check_xen_buildid(&elf);
+    if ( rc )
+       goto out;
+
     rc = move_payload(payload, &elf);
     if ( rc )
         goto out;
@@ -1093,10 +1122,6 @@ static int load_payload_data(struct payload *payload, void *raw, size_t len)
     if ( rc )
         goto out;
 
-    rc = xen_build_id_dep(payload);
-    if ( rc )
-        goto out;
-
     rc = build_symbol_table(payload, &elf);
     if ( rc )
         goto out;
@@ -2199,9 +2224,6 @@ static void cf_check livepatch_printall(unsigned char key)
 
         if ( data->dep.len )
             printk("depend-on=%*phN\n", data->dep.len, data->dep.p);
-
-        if ( data->xen_dep.len )
-            printk("depend-on-xen=%*phN\n", data->xen_dep.len, data->xen_dep.p);
     }
 
     spin_unlock(&payload_lock);
diff --git a/xen/include/xen/livepatch_payload.h b/xen/include/xen/livepatch_payload.h
index 472d6a4a63c1..c6dc7cb5fa21 100644
--- a/xen/include/xen/livepatch_payload.h
+++ b/xen/include/xen/livepatch_payload.h
@@ -62,7 +62,6 @@ 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). */
-    struct livepatch_build_id xen_dep;   /* ELFNOTE_DESC(.livepatch.xen_depends). */
     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. */
     struct livepatch_hooks hooks;        /* Pre and post hooks for apply and revert */
-- 
2.46.0

[PATCH v2 5/6] x86/alternatives: do not BUG during apply

[Roger Pau Monne]

alternatives is used both at boot time, and when loading livepatch payloads.
While for the former it makes sense to panic, it's not useful for the later, as
for livepatches it's possible to fail to load the livepatch if alternatives
cannot be resolved and continue operating normally.

Relax the BUGs in _apply_alternatives() to instead return an error code.  The
caller will figure out whether the failures are fatal and panic.

Print an error message to provide some user-readable information about what
went wrong.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Unconditionally return from _apply_alternative() and let the caller panic
   if required.
 - Remove test, as next patch imposes restrictions that break the test.
---
 xen/arch/x86/alternative.c             | 46 ++++++++++++++++++++------
 xen/arch/x86/include/asm/alternative.h |  2 +-
 xen/common/livepatch.c                 | 10 +++++-
 3 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
index 7824053c9d33..c8848ba6006e 100644
--- a/xen/arch/x86/alternative.c
+++ b/xen/arch/x86/alternative.c
@@ -175,9 +175,9 @@ extern void *const __initdata_cf_clobber_end[];
  * invocation, such that no CALLs/JMPs to NULL pointers will be left
  * around. See also the further comment below.
  */
-static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
-                                                  struct alt_instr *end,
-                                                  bool force)
+static int init_or_livepatch _apply_alternatives(struct alt_instr *start,
+                                                 struct alt_instr *end,
+                                                 bool force)
 {
     struct alt_instr *a, *base;
 
@@ -198,9 +198,29 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
         uint8_t buf[MAX_PATCH_LEN];
         unsigned int total_len = a->orig_len + a->pad_len;
 
-        BUG_ON(a->repl_len > total_len);
-        BUG_ON(total_len > sizeof(buf));
-        BUG_ON(a->cpuid >= NCAPINTS * 32);
+        if ( a->repl_len > total_len )
+        {
+            printk(XENLOG_ERR
+                   "alt replacement size (%#x) bigger than destination (%#x)\n",
+                   a->repl_len, total_len);
+            return -ENOSPC;
+        }
+
+        if ( total_len > sizeof(buf) )
+        {
+            printk(XENLOG_ERR
+                   "alt destination size (%#x) bigger than buffer (%#zx)\n",
+                   total_len, sizeof(buf));
+            return -ENOSPC;
+        }
+
+        if ( a->cpuid >= NCAPINTS * 32 )
+        {
+             printk(XENLOG_ERR
+                   "alt CPU feature (%#x) outside of featureset range (%#x)\n",
+                   a->cpuid, NCAPINTS * 32);
+            return -ERANGE;
+        }
 
         /*
          * Detect sequences of alt_instr's patching the same origin site, and
@@ -356,12 +376,14 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
 
         printk("altcall: Optimised away %u endbr64 instructions\n", clobbered);
     }
+
+    return 0;
 }
 
 #ifdef CONFIG_LIVEPATCH
-void apply_alternatives(struct alt_instr *start, struct alt_instr *end)
+int apply_alternatives(struct alt_instr *start, struct alt_instr *end)
 {
-    _apply_alternatives(start, end, true);
+    return _apply_alternatives(start, end, true);
 }
 #endif
 
@@ -383,6 +405,8 @@ static int __init cf_check nmi_apply_alternatives(
      */
     if ( !(alt_done & alt_todo) )
     {
+        int rc;
+
         /*
          * Relax perms on .text to be RWX, so we can modify them.
          *
@@ -394,8 +418,10 @@ static int __init cf_check nmi_apply_alternatives(
                                  PAGE_HYPERVISOR_RWX);
         flush_local(FLUSH_TLB_GLOBAL);
 
-        _apply_alternatives(__alt_instructions, __alt_instructions_end,
-                            alt_done);
+        rc = _apply_alternatives(__alt_instructions, __alt_instructions_end,
+                                 alt_done);
+        if ( rc )
+            panic("Unable to apply alternatives: %d\n", rc);
 
         /*
          * Reinstate perms on .text to be RX.  This also cleans out the dirty
diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index a86eadfaecbd..69555d781ef9 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -24,7 +24,7 @@ struct __packed alt_instr {
 
 extern void add_nops(void *insns, unsigned int len);
 /* Similar to alternative_instructions except it can be run with IRQs enabled. */
-extern void apply_alternatives(struct alt_instr *start, struct alt_instr *end);
+extern int apply_alternatives(struct alt_instr *start, struct alt_instr *end);
 extern void alternative_instructions(void);
 extern void alternative_branches(void);
 
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 895c425cd5ea..c777f64d88d4 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -896,7 +896,15 @@ static int prepare_payload(struct payload *payload,
                 return -EINVAL;
             }
         }
-        apply_alternatives(start, end);
+
+        rc = apply_alternatives(start, end);
+        if ( rc )
+        {
+            printk(XENLOG_ERR LIVEPATCH "%s applying alternatives failed: %d\n",
+                   elf->name, rc);
+            return rc;
+        }
+
     alt_done:;
 #else
         printk(XENLOG_ERR LIVEPATCH "%s: We don't support alternative patching\n",
-- 
2.46.0

[PATCH v2 6/6] x86/alternative: build time check feature is in range

[Roger Pau Monne]

Ensure at build time the feature(s) used for the alternative blocks are in
range of the featureset.

No functional change intended, as all current usages are correct.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/include/asm/alternative-asm.h | 3 +++
 xen/arch/x86/include/asm/alternative.h     | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/xen/arch/x86/include/asm/alternative-asm.h b/xen/arch/x86/include/asm/alternative-asm.h
index 4092f5ba70a6..83e8594f0eaf 100644
--- a/xen/arch/x86/include/asm/alternative-asm.h
+++ b/xen/arch/x86/include/asm/alternative-asm.h
@@ -12,6 +12,9 @@
  * instruction. See apply_alternatives().
  */
 .macro altinstruction_entry orig, repl, feature, orig_len, repl_len, pad_len
+    .if \feature >= NCAPINTS * 32
+        .error "alternative feature outside of featureset range"
+    .endif
     .long \orig - .
     .long \repl - .
     .word \feature
diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
index 69555d781ef9..b7f155994b2c 100644
--- a/xen/arch/x86/include/asm/alternative.h
+++ b/xen/arch/x86/include/asm/alternative.h
@@ -7,6 +7,7 @@
 #include <xen/lib.h>
 #include <xen/stringify.h>
 #include <asm/asm-macros.h>
+#include <asm/cpufeatureset.h>
 
 struct __packed alt_instr {
     int32_t  orig_offset;   /* original instruction */
@@ -59,6 +60,9 @@ extern void alternative_branches(void);
                     alt_repl_len(n2)) "-" alt_orig_len)
 
 #define ALTINSTR_ENTRY(feature, num)                                    \
+        " .if " __stringify(feature) " >= " __stringify(NCAPINTS * 32) "\n"\
+        " .error \"alternative feature outside of featureset range\"\n" \
+        " .endif\n"                                                     \
         " .long .LXEN%=_orig_s - .\n"             /* label           */ \
         " .long " alt_repl_s(num)" - .\n"         /* new instruction */ \
         " .word " __stringify(feature) "\n"       /* feature bit     */ \
-- 
2.46.0

Re: [PATCH v2 6/6] x86/alternative: build time check feature is in range

[Andrew Cooper]

On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
> index 69555d781ef9..b7f155994b2c 100644
> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -7,6 +7,7 @@
>  #include <xen/lib.h>
>  #include <xen/stringify.h>
>  #include <asm/asm-macros.h>
> +#include <asm/cpufeatureset.h>
>  
>  struct __packed alt_instr {
>      int32_t  orig_offset;   /* original instruction */
> @@ -59,6 +60,9 @@ extern void alternative_branches(void);
>                      alt_repl_len(n2)) "-" alt_orig_len)
>  
>  #define ALTINSTR_ENTRY(feature, num)                                    \
> +        " .if " __stringify(feature) " >= " __stringify(NCAPINTS * 32) "\n"\

Please use STR() from macros.h.  It's shorter, and we're trying to
retire the use of __stringify().

Happy to fix on commit.  Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

Re: [PATCH v2 1/6] xen/livepatch: remove useless check for duplicated sections

[Jan Beulich]

On 25.09.2024 10:42, Roger Pau Monne wrote:
> The current check for duplicated sections in a payload is not effective.  Such
> check is done inside a loop that iterates over the sections names, it's
> logically impossible for the bitmap to be set more than once.
> 
> The usage of a bitmap in check_patching_sections() has been replaced with a
> boolean, since the function just cares that at least one of the special
> sections is present.
> 
> No functional change intended, as the check was useless.
> 
> Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.')
> Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Just to clarify for my eventual picking up for backporting: Despite
the Fixes: tags there's no actual bug being fixed; it's merely code
simplification. So no need to backport.

Jan

Re: [PATCH v2 5/6] x86/alternatives: do not BUG during apply

[Jan Beulich]

On 25.09.2024 10:42, Roger Pau Monne wrote:
> alternatives is used both at boot time, and when loading livepatch payloads.
> While for the former it makes sense to panic, it's not useful for the later, as
> for livepatches it's possible to fail to load the livepatch if alternatives
> cannot be resolved and continue operating normally.
> 
> Relax the BUGs in _apply_alternatives() to instead return an error code.  The
> caller will figure out whether the failures are fatal and panic.
> 
> Print an error message to provide some user-readable information about what
> went wrong.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Albeit ...

> @@ -394,8 +418,10 @@ static int __init cf_check nmi_apply_alternatives(
>                                   PAGE_HYPERVISOR_RWX);
>          flush_local(FLUSH_TLB_GLOBAL);
>  
> -        _apply_alternatives(__alt_instructions, __alt_instructions_end,
> -                            alt_done);
> +        rc = _apply_alternatives(__alt_instructions, __alt_instructions_end,
> +                                 alt_done);
> +        if ( rc )
> +            panic("Unable to apply alternatives: %d\n", rc);

... I see little value in logging rc here - the other log message will
provide better detail anyway.

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -896,7 +896,15 @@ static int prepare_payload(struct payload *payload,
>                  return -EINVAL;
>              }
>          }
> -        apply_alternatives(start, end);
> +
> +        rc = apply_alternatives(start, end);
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR LIVEPATCH "%s applying alternatives failed: %d\n",
> +                   elf->name, rc);
> +            return rc;
> +        }

Whereas here it may indeed make sense to keep things as you have them, as the
error code is propagated onwards, and matching it with other error messages
coming from elsewhere may help in understanding the situation.

As to possible applying: It looks as if this was independent of the earlier 4
patches?

Jan

Re: [PATCH v2 6/6] x86/alternative: build time check feature is in range

[Jan Beulich]

On 25.09.2024 10:42, Roger Pau Monne wrote:
> Ensure at build time the feature(s) used for the alternative blocks are in
> range of the featureset.
> 
> No functional change intended, as all current usages are correct.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

I'm struggling with what this is meant to guard against. All validly usable
constants are within range. Any unsuitable constant can of course have any
value, yet you'd then refuse only those which are out of bounds.

Jan

Re: [PATCH v2 1/6] xen/livepatch: remove useless check for duplicated sections

[Roger Pau Monné]

On Wed, Sep 25, 2024 at 10:52:06AM +0200, Jan Beulich wrote:
> On 25.09.2024 10:42, Roger Pau Monne wrote:
> > The current check for duplicated sections in a payload is not effective.  Such
> > check is done inside a loop that iterates over the sections names, it's
> > logically impossible for the bitmap to be set more than once.
> > 
> > The usage of a bitmap in check_patching_sections() has been replaced with a
> > boolean, since the function just cares that at least one of the special
> > sections is present.
> > 
> > No functional change intended, as the check was useless.
> > 
> > Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.')
> > Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Just to clarify for my eventual picking up for backporting: Despite
> the Fixes: tags there's no actual bug being fixed; it's merely code
> simplification. So no need to backport.

Indeed, no strict bug, just useless code (unless my analysis is wrong).

Thanks, Roger.

Re: [PATCH v2 6/6] x86/alternative: build time check feature is in range

[Roger Pau Monné]

On Wed, Sep 25, 2024 at 11:04:45AM +0200, Jan Beulich wrote:
> On 25.09.2024 10:42, Roger Pau Monne wrote:
> > Ensure at build time the feature(s) used for the alternative blocks are in
> > range of the featureset.
> > 
> > No functional change intended, as all current usages are correct.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> I'm struggling with what this is meant to guard against. All validly usable
> constants are within range. Any unsuitable constant can of course have any
> value, yet you'd then refuse only those which are out of bounds.

It's IMO better than nothing, and it's a build-time check.

Thanks, Roger.

Re: [PATCH v2 6/6] x86/alternative: build time check feature is in range

[Roger Pau Monné]

On Wed, Sep 25, 2024 at 09:51:36AM +0100, Andrew Cooper wrote:
> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/include/asm/alternative.h b/xen/arch/x86/include/asm/alternative.h
> > index 69555d781ef9..b7f155994b2c 100644
> > --- a/xen/arch/x86/include/asm/alternative.h
> > +++ b/xen/arch/x86/include/asm/alternative.h
> > @@ -7,6 +7,7 @@
> >  #include <xen/lib.h>
> >  #include <xen/stringify.h>
> >  #include <asm/asm-macros.h>
> > +#include <asm/cpufeatureset.h>
> >  
> >  struct __packed alt_instr {
> >      int32_t  orig_offset;   /* original instruction */
> > @@ -59,6 +60,9 @@ extern void alternative_branches(void);
> >                      alt_repl_len(n2)) "-" alt_orig_len)
> >  
> >  #define ALTINSTR_ENTRY(feature, num)                                    \
> > +        " .if " __stringify(feature) " >= " __stringify(NCAPINTS * 32) "\n"\
> 
> Please use STR() from macros.h.  It's shorter, and we're trying to
> retire the use of __stringify().

Oh, yes, indeed.  I just copied from the surrounding context and
forgot about STR().

> Happy to fix on commit.  Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>

Sure, thanks.

Roger.

Re: [PATCH v2 5/6] x86/alternatives: do not BUG during apply

[Roger Pau Monné]

On Wed, Sep 25, 2024 at 11:01:08AM +0200, Jan Beulich wrote:
> On 25.09.2024 10:42, Roger Pau Monne wrote:
> > alternatives is used both at boot time, and when loading livepatch payloads.
> > While for the former it makes sense to panic, it's not useful for the later, as
> > for livepatches it's possible to fail to load the livepatch if alternatives
> > cannot be resolved and continue operating normally.
> > 
> > Relax the BUGs in _apply_alternatives() to instead return an error code.  The
> > caller will figure out whether the failures are fatal and panic.
> > 
> > Print an error message to provide some user-readable information about what
> > went wrong.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Albeit ...
> 
> > @@ -394,8 +418,10 @@ static int __init cf_check nmi_apply_alternatives(
> >                                   PAGE_HYPERVISOR_RWX);
> >          flush_local(FLUSH_TLB_GLOBAL);
> >  
> > -        _apply_alternatives(__alt_instructions, __alt_instructions_end,
> > -                            alt_done);
> > +        rc = _apply_alternatives(__alt_instructions, __alt_instructions_end,
> > +                                 alt_done);
> > +        if ( rc )
> > +            panic("Unable to apply alternatives: %d\n", rc);
> 
> ... I see little value in logging rc here - the other log message will
> provide better detail anyway.

Current log messages do indeed provide better detail, but maybe we end
up adding new return error paths to _apply_alternatives() in the
future.  I see no harm in printing the error code if there's one.

> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -896,7 +896,15 @@ static int prepare_payload(struct payload *payload,
> >                  return -EINVAL;
> >              }
> >          }
> > -        apply_alternatives(start, end);
> > +
> > +        rc = apply_alternatives(start, end);
> > +        if ( rc )
> > +        {
> > +            printk(XENLOG_ERR LIVEPATCH "%s applying alternatives failed: %d\n",
> > +                   elf->name, rc);
> > +            return rc;
> > +        }
> 
> Whereas here it may indeed make sense to keep things as you have them, as the
> error code is propagated onwards, and matching it with other error messages
> coming from elsewhere may help in understanding the situation.
> 
> As to possible applying: It looks as if this was independent of the earlier 4
> patches?

Yes, I think patches 5 and 6 can be applied ahead of the preceding
livepatch fixes.

Thanks, Roger.

Re: [PATCH v2 1/6] xen/livepatch: remove useless check for duplicated sections

[Andrew Cooper]

On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> The current check for duplicated sections in a payload is not effective.  Such
> check is done inside a loop that iterates over the sections names, it's
> logically impossible for the bitmap to be set more than once.
>
> The usage of a bitmap in check_patching_sections() has been replaced with a
> boolean, since the function just cares that at least one of the special
> sections is present.
>
> No functional change intended, as the check was useless.
>
> Fixes: 29f4ab0b0a4f ('xsplice: Implement support for applying/reverting/replacing patches.')
> Fixes: 76b3d4098a92 ('livepatch: Do not enforce ELF_LIVEPATCH_FUNC section presence')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Yes I agree.  This is useless logic.

The only time we could spot such a case is when matching the section
table with the string table.  For this logic, we always only get
whichever answer livepatch_elf_sec_by_name() decides.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> and I'm very
happy to see some atomics disappear.

Re: [PATCH v2 2/6] xen/livepatch: zero pointer to temporary load buffer

[Andrew Cooper]

On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> The livepatch_elf_sec data field points to the temporary load buffer, it's the
> load_addr field that points to the stable loaded section data.  Zero the data
> field once load_addr is set, as it would otherwise become a dangling pointer
> once the load buffer is freed.
>
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v1:
>  - New in this version.
> ---
>  xen/common/livepatch.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index df41dcce970a..87b3db03e26d 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -383,6 +383,9 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
>              }
>              else
>                  memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> +
> +            /* Avoid leaking pointers to temporary load buffers. */
> +            elf->sec[i].data = NULL;
>          }
>      }
>  

Where is the data allocated and freed?

I don't see it being freed in this loop, so how is freed subsequently?

~Andrew

Re: [PATCH v2 3/6] xen/livepatch: simplify and unify logic in prepare_payload()

[Andrew Cooper]

On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 87b3db03e26d..8e61083f23a7 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -470,6 +470,31 @@ static int xen_build_id_dep(const struct payload *payload)
>      return 0;
>  }
>  
> +/* Parses build-id sections into the given destination. */
> +static int parse_buildid(const struct livepatch_elf_sec *sec,
> +                         struct livepatch_build_id *id)
> +{
> +    const Elf_Note *n;
> +    int rc;
> +
> +    /* Presence of the sections is ensured by check_special_sections(). */
> +    ASSERT(sec);
> +
> +    n = sec->load_addr;
> +
> +    if ( sec->sec->sh_size <= sizeof(*n) )
> +        return -EINVAL;
> +
> +    rc = xen_build_id_check(n, sec->sec->sh_size, &id->p, &id->len);

I've just realised what is so confusing.

This function is not a Xen buildid check, it's an ELF buildid note check.

I'll do a followup patch after yours goes in renaming it.

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

Re: [PATCH v2 2/6] xen/livepatch: zero pointer to temporary load buffer

[Roger Pau Monné]

On Wed, Sep 25, 2024 at 10:33:39AM +0100, Andrew Cooper wrote:
> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> > The livepatch_elf_sec data field points to the temporary load buffer, it's the
> > load_addr field that points to the stable loaded section data.  Zero the data
> > field once load_addr is set, as it would otherwise become a dangling pointer
> > once the load buffer is freed.
> >
> > No functional change intended.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v1:
> >  - New in this version.
> > ---
> >  xen/common/livepatch.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index df41dcce970a..87b3db03e26d 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -383,6 +383,9 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >              }
> >              else
> >                  memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> > +
> > +            /* Avoid leaking pointers to temporary load buffers. */
> > +            elf->sec[i].data = NULL;
> >          }
> >      }
> >  
> 
> Where is the data allocated and freed?
> 
> I don't see it being freed in this loop, so how is freed subsequently?

It's allocated and freed by livepatch_upload(), it's the raw_data
buffer that's allocated in the context of that function.

Thanks, Roger.

Re: [PATCH v2 5/6] x86/alternatives: do not BUG during apply

[Jan Beulich]

On 25.09.2024 11:28, Roger Pau Monné wrote:
> On Wed, Sep 25, 2024 at 11:01:08AM +0200, Jan Beulich wrote:
>> On 25.09.2024 10:42, Roger Pau Monne wrote:
>>> alternatives is used both at boot time, and when loading livepatch payloads.
>>> While for the former it makes sense to panic, it's not useful for the later, as
>>> for livepatches it's possible to fail to load the livepatch if alternatives
>>> cannot be resolved and continue operating normally.
>>>
>>> Relax the BUGs in _apply_alternatives() to instead return an error code.  The
>>> caller will figure out whether the failures are fatal and panic.
>>>
>>> Print an error message to provide some user-readable information about what
>>> went wrong.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> Albeit ...
>>
>>> @@ -394,8 +418,10 @@ static int __init cf_check nmi_apply_alternatives(
>>>                                   PAGE_HYPERVISOR_RWX);
>>>          flush_local(FLUSH_TLB_GLOBAL);
>>>  
>>> -        _apply_alternatives(__alt_instructions, __alt_instructions_end,
>>> -                            alt_done);
>>> +        rc = _apply_alternatives(__alt_instructions, __alt_instructions_end,
>>> +                                 alt_done);
>>> +        if ( rc )
>>> +            panic("Unable to apply alternatives: %d\n", rc);
>>
>> ... I see little value in logging rc here - the other log message will
>> provide better detail anyway.
> 
> Current log messages do indeed provide better detail, but maybe we end
> up adding new return error paths to _apply_alternatives() in the
> future.

I'd expect such error paths to then also have dedicated logging.

>  I see no harm in printing the error code if there's one.

Well, it's not much harm indeed, yet imo extra data logged also normally
wants to have a reason for the logging. After if you look at the log,
you'd expect every detail to tell you something (useful; in some certain
cases at least). Anyway - I don't mean to insist on the removal, it just
looked pretty useless to me.

Jan

Re: [PATCH v2 3/6] xen/livepatch: simplify and unify logic in prepare_payload()

[Roger Pau Monné]

On Wed, Sep 25, 2024 at 10:37:56AM +0100, Andrew Cooper wrote:
> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index 87b3db03e26d..8e61083f23a7 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -470,6 +470,31 @@ static int xen_build_id_dep(const struct payload *payload)
> >      return 0;
> >  }
> >  
> > +/* Parses build-id sections into the given destination. */
> > +static int parse_buildid(const struct livepatch_elf_sec *sec,
> > +                         struct livepatch_build_id *id)
> > +{
> > +    const Elf_Note *n;
> > +    int rc;
> > +
> > +    /* Presence of the sections is ensured by check_special_sections(). */
> > +    ASSERT(sec);
> > +
> > +    n = sec->load_addr;
> > +
> > +    if ( sec->sec->sh_size <= sizeof(*n) )
> > +        return -EINVAL;
> > +
> > +    rc = xen_build_id_check(n, sec->sec->sh_size, &id->p, &id->len);
> 
> I've just realised what is so confusing.
> 
> This function is not a Xen buildid check, it's an ELF buildid note check.
> 
> I'll do a followup patch after yours goes in renaming it.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Yeah, the naming of xen_build_id_check is confusing, as it's not just
a check, it also populates livepatch_build_id fields.  Thought about
renaming it, but the series was already long enough...

Thanks, Roger.

Re: [PATCH v2 4/6] xen/livepatch: do Xen build-id check earlier

[Andrew Cooper]

On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> The check against the expected Xen build ID should be done ahead of attempting
> to apply the alternatives contained in the livepatch.
>
> If the CPUID in the alternatives patching data is out of the scope of the
> running Xen featureset the BUG() in _apply_alternatives() will trigger thus
> bringing the system down.  Note the layout of struct alt_instr could also
> change between versions.  It's also possible for struct exception_table_entry
> to have changed format, hence leading to other kind of errors if parsing of the
> payload is done ahead of checking if the Xen build-id matches.
>
> Move the Xen build ID check as early as possible.  To do so introduce a new
> check_xen_buildid() function that parses and checks the Xen build-id before
> moving the payload.  Since the expected Xen build-id is used early to
> detect whether the livepatch payload could be loaded, there's no reason to
> store it in the payload struct, as a non-matching Xen build-id won't get the
> payload populated in the first place.
>
> Note parse_buildid() has to be slightly adjusted to allow fetching the section
> data from the 'data' field instead of the 'load_addr' one: with the Xen build
> ID moved ahead of move_payload() 'load_addr' is not yet set when the Xen build
> ID check is performed.  Also printing the expected Xen build ID has part of
> dumping the payload is no longer done, as all loaded payloads would have Xen
> build IDs matching the running Xen, otherwise they would have failed to load.
>
> Fixes: 879615f5db1d ('livepatch: Always check hypervisor build ID upon livepatch upload')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Much nicer.  A couple of suggestions.

> ---
> Changes since v1:
>  - Do the Xen build-id check even earlier.
> ---
>  xen/common/livepatch.c              | 66 +++++++++++++++++++----------
>  xen/include/xen/livepatch_payload.h |  1 -
>  2 files changed, 44 insertions(+), 23 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 8e61083f23a7..895c425cd5ea 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -448,24 +448,21 @@ static bool section_ok(const struct livepatch_elf *elf,
>      return true;
>  }
>  
> -static int xen_build_id_dep(const struct payload *payload)
> +static int xen_build_id_dep(const struct livepatch_build_id *expected)
>  {
>      const void *id = NULL;
>      unsigned int len = 0;
>      int rc;
>  
> -    ASSERT(payload->xen_dep.len);
> -    ASSERT(payload->xen_dep.p);
> +    ASSERT(expected->len);
> +    ASSERT(expected->p);
>  
>      rc = xen_build_id(&id, &len);
>      if ( rc )
>          return rc;
>  
> -    if ( payload->xen_dep.len != len || memcmp(id, payload->xen_dep.p, len) ) {
> -        printk(XENLOG_ERR LIVEPATCH "%s: check against hypervisor build-id failed\n",
> -               payload->name);
> +    if ( expected->len != len || memcmp(id, expected->p, len) )
>          return -EINVAL;
> -    }

I'd suggest merging this into check_xen_buildid(), which is the single
caller and serves the same singular purpose.

It removes the ASSERT() (expected is now a local variable), and it helps
with some diagnostic improvements.

>  
>      return 0;
>  }
> @@ -495,11 +493,44 @@ static int parse_buildid(const struct livepatch_elf_sec *sec,
>     return 0;
>  }
>  
> +static int check_xen_buildid(const struct livepatch_elf *elf)
> +{
> +    struct livepatch_build_id id;
> +    const struct livepatch_elf_sec *sec =
> +        livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
> +    int rc;
> +
> +    if ( !sec )
> +    {
> +        printk(XENLOG_ERR LIVEPATCH "%s: %s is missing\n",

"%s: Section: '%s' missing\n".

(Maybe no single quotes around the section as we know it's non-empty.)

> +               elf->name, ELF_LIVEPATCH_XEN_DEPENDS);
> +        return -EINVAL;
> +    }
> +
> +    rc = parse_buildid(sec, &id);
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR LIVEPATCH "%s: failed to parse build-id at %s: %d\n",

"%s: Failed to parse section '%s' as build id: %d\n".

> +               elf->name, ELF_LIVEPATCH_XEN_DEPENDS, rc);
> +        return -EINVAL;
> +    }
> +
> +    rc = xen_build_id_dep(&id);
> +    if ( rc )
> +    {
> +        printk(XENLOG_ERR LIVEPATCH
> +               "%s: check against hypervisor build-id failed: %d\n",

"%s: build-id mismatch:\n"
"  livepatch: %*phN\n"
"        xen: %*phN\n"

This needs xen_build_id_dep() inlining in order to have the xen build-id
string, but the end result is much more informative.

I think I'm happy doing all of this on commit, but it might be a better
idea not to.

~Andrew

Re: [PATCH v2 5/6] x86/alternatives: do not BUG during apply

[Andrew Cooper]

On 25/09/2024 11:02 am, Jan Beulich wrote:
> On 25.09.2024 11:28, Roger Pau Monné wrote:
>> On Wed, Sep 25, 2024 at 11:01:08AM +0200, Jan Beulich wrote:
>>> On 25.09.2024 10:42, Roger Pau Monne wrote:
>>>> alternatives is used both at boot time, and when loading livepatch payloads.
>>>> While for the former it makes sense to panic, it's not useful for the later, as
>>>> for livepatches it's possible to fail to load the livepatch if alternatives
>>>> cannot be resolved and continue operating normally.
>>>>
>>>> Relax the BUGs in _apply_alternatives() to instead return an error code.  The
>>>> caller will figure out whether the failures are fatal and panic.
>>>>
>>>> Print an error message to provide some user-readable information about what
>>>> went wrong.
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> Albeit ...
>>>
>>>> @@ -394,8 +418,10 @@ static int __init cf_check nmi_apply_alternatives(
>>>>                                   PAGE_HYPERVISOR_RWX);
>>>>          flush_local(FLUSH_TLB_GLOBAL);
>>>>  
>>>> -        _apply_alternatives(__alt_instructions, __alt_instructions_end,
>>>> -                            alt_done);
>>>> +        rc = _apply_alternatives(__alt_instructions, __alt_instructions_end,
>>>> +                                 alt_done);
>>>> +        if ( rc )
>>>> +            panic("Unable to apply alternatives: %d\n", rc);
>>> ... I see little value in logging rc here - the other log message will
>>> provide better detail anyway.
>> Current log messages do indeed provide better detail, but maybe we end
>> up adding new return error paths to _apply_alternatives() in the
>> future.
> I'd expect such error paths to then also have dedicated logging.
>
>>  I see no harm in printing the error code if there's one.
> Well, it's not much harm indeed, yet imo extra data logged also normally
> wants to have a reason for the logging. After if you look at the log,
> you'd expect every detail to tell you something (useful; in some certain
> cases at least). Anyway - I don't mean to insist on the removal, it just
> looked pretty useless to me.

People reading the logs can ignore bits they don't think are useful.

What they cannot do is read data which should have been there but wasn't.

~Andrew

Re: [PATCH v2 5/6] x86/alternatives: do not BUG during apply

[Andrew Cooper]

On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> alternatives is used both at boot time, and when loading livepatch payloads.
> While for the former it makes sense to panic, it's not useful for the later, as
> for livepatches it's possible to fail to load the livepatch if alternatives
> cannot be resolved and continue operating normally.
>
> Relax the BUGs in _apply_alternatives() to instead return an error code.  The
> caller will figure out whether the failures are fatal and panic.
>
> Print an error message to provide some user-readable information about what
> went wrong.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Much nicer.  A few more diagnostic comments.

> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> index 7824053c9d33..c8848ba6006e 100644
> --- a/xen/arch/x86/alternative.c
> +++ b/xen/arch/x86/alternative.c
> @@ -198,9 +198,29 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
>          uint8_t buf[MAX_PATCH_LEN];
>          unsigned int total_len = a->orig_len + a->pad_len;
>  
> -        BUG_ON(a->repl_len > total_len);
> -        BUG_ON(total_len > sizeof(buf));
> -        BUG_ON(a->cpuid >= NCAPINTS * 32);
> +        if ( a->repl_len > total_len )
> +        {
> +            printk(XENLOG_ERR
> +                   "alt replacement size (%#x) bigger than destination (%#x)\n",

These all say "some alternative went wrong", without identifying which. 
For x86_decode_lite(), debugging was far easier when using:

"Alternative for %ps ...", ALT_ORIG_PTR(a)

If we get the order of loading correct, then %ps will even work for a
livepatch, but that's secondary - even the raw number is slightly useful
given the livepatch load address.

I could be talked down to just "Alt for %ps" as you've got it here.  I
think it's clear enough in context.  So, I'd recommend:

"Alt for %ps, replacement size %#x larger than origin %#x\n".

Here, I think origin is better than destination, when discussing
alternatives.

I can adjust on commit.  Everything else is fine.

~Andrew

Re: [PATCH v2 4/6] xen/livepatch: do Xen build-id check earlier

[Roger Pau Monné]

On Wed, Sep 25, 2024 at 11:21:01AM +0100, Andrew Cooper wrote:
> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> > The check against the expected Xen build ID should be done ahead of attempting
> > to apply the alternatives contained in the livepatch.
> >
> > If the CPUID in the alternatives patching data is out of the scope of the
> > running Xen featureset the BUG() in _apply_alternatives() will trigger thus
> > bringing the system down.  Note the layout of struct alt_instr could also
> > change between versions.  It's also possible for struct exception_table_entry
> > to have changed format, hence leading to other kind of errors if parsing of the
> > payload is done ahead of checking if the Xen build-id matches.
> >
> > Move the Xen build ID check as early as possible.  To do so introduce a new
> > check_xen_buildid() function that parses and checks the Xen build-id before
> > moving the payload.  Since the expected Xen build-id is used early to
> > detect whether the livepatch payload could be loaded, there's no reason to
> > store it in the payload struct, as a non-matching Xen build-id won't get the
> > payload populated in the first place.
> >
> > Note parse_buildid() has to be slightly adjusted to allow fetching the section
> > data from the 'data' field instead of the 'load_addr' one: with the Xen build
> > ID moved ahead of move_payload() 'load_addr' is not yet set when the Xen build
> > ID check is performed.  Also printing the expected Xen build ID has part of
> > dumping the payload is no longer done, as all loaded payloads would have Xen
> > build IDs matching the running Xen, otherwise they would have failed to load.
> >
> > Fixes: 879615f5db1d ('livepatch: Always check hypervisor build ID upon livepatch upload')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Much nicer.  A couple of suggestions.
> 
> > ---
> > Changes since v1:
> >  - Do the Xen build-id check even earlier.
> > ---
> >  xen/common/livepatch.c              | 66 +++++++++++++++++++----------
> >  xen/include/xen/livepatch_payload.h |  1 -
> >  2 files changed, 44 insertions(+), 23 deletions(-)
> >
> > diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> > index 8e61083f23a7..895c425cd5ea 100644
> > --- a/xen/common/livepatch.c
> > +++ b/xen/common/livepatch.c
> > @@ -448,24 +448,21 @@ static bool section_ok(const struct livepatch_elf *elf,
> >      return true;
> >  }
> >  
> > -static int xen_build_id_dep(const struct payload *payload)
> > +static int xen_build_id_dep(const struct livepatch_build_id *expected)
> >  {
> >      const void *id = NULL;
> >      unsigned int len = 0;
> >      int rc;
> >  
> > -    ASSERT(payload->xen_dep.len);
> > -    ASSERT(payload->xen_dep.p);
> > +    ASSERT(expected->len);
> > +    ASSERT(expected->p);
> >  
> >      rc = xen_build_id(&id, &len);
> >      if ( rc )
> >          return rc;
> >  
> > -    if ( payload->xen_dep.len != len || memcmp(id, payload->xen_dep.p, len) ) {
> > -        printk(XENLOG_ERR LIVEPATCH "%s: check against hypervisor build-id failed\n",
> > -               payload->name);
> > +    if ( expected->len != len || memcmp(id, expected->p, len) )
> >          return -EINVAL;
> > -    }
> 
> I'd suggest merging this into check_xen_buildid(), which is the single
> caller and serves the same singular purpose.
> 
> It removes the ASSERT() (expected is now a local variable), and it helps
> with some diagnostic improvements.

Sure.

> >  
> >      return 0;
> >  }
> > @@ -495,11 +493,44 @@ static int parse_buildid(const struct livepatch_elf_sec *sec,
> >     return 0;
> >  }
> >  
> > +static int check_xen_buildid(const struct livepatch_elf *elf)
> > +{
> > +    struct livepatch_build_id id;
> > +    const struct livepatch_elf_sec *sec =
> > +        livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
> > +    int rc;
> > +
> > +    if ( !sec )
> > +    {
> > +        printk(XENLOG_ERR LIVEPATCH "%s: %s is missing\n",
> 
> "%s: Section: '%s' missing\n".
> 
> (Maybe no single quotes around the section as we know it's non-empty.)
> 
> > +               elf->name, ELF_LIVEPATCH_XEN_DEPENDS);
> > +        return -EINVAL;
> > +    }
> > +
> > +    rc = parse_buildid(sec, &id);
> > +    if ( rc )
> > +    {
> > +        printk(XENLOG_ERR LIVEPATCH "%s: failed to parse build-id at %s: %d\n",
> 
> "%s: Failed to parse section '%s' as build id: %d\n".
> 
> > +               elf->name, ELF_LIVEPATCH_XEN_DEPENDS, rc);
> > +        return -EINVAL;
> > +    }
> > +
> > +    rc = xen_build_id_dep(&id);
> > +    if ( rc )
> > +    {
> > +        printk(XENLOG_ERR LIVEPATCH
> > +               "%s: check against hypervisor build-id failed: %d\n",
> 
> "%s: build-id mismatch:\n"
> "  livepatch: %*phN\n"
> "        xen: %*phN\n"
> 
> This needs xen_build_id_dep() inlining in order to have the xen build-id
> string, but the end result is much more informative.
> 
> I think I'm happy doing all of this on commit, but it might be a better
> idea not to.

No problem, I can send a v3.

Thanks, Roger.

Re: [PATCH v2 5/6] x86/alternatives: do not BUG during apply

[Roger Pau Monné]

On Wed, Sep 25, 2024 at 11:53:30AM +0100, Andrew Cooper wrote:
> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> > alternatives is used both at boot time, and when loading livepatch payloads.
> > While for the former it makes sense to panic, it's not useful for the later, as
> > for livepatches it's possible to fail to load the livepatch if alternatives
> > cannot be resolved and continue operating normally.
> >
> > Relax the BUGs in _apply_alternatives() to instead return an error code.  The
> > caller will figure out whether the failures are fatal and panic.
> >
> > Print an error message to provide some user-readable information about what
> > went wrong.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Much nicer.  A few more diagnostic comments.
> 
> > diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> > index 7824053c9d33..c8848ba6006e 100644
> > --- a/xen/arch/x86/alternative.c
> > +++ b/xen/arch/x86/alternative.c
> > @@ -198,9 +198,29 @@ static void init_or_livepatch _apply_alternatives(struct alt_instr *start,
> >          uint8_t buf[MAX_PATCH_LEN];
> >          unsigned int total_len = a->orig_len + a->pad_len;
> >  
> > -        BUG_ON(a->repl_len > total_len);
> > -        BUG_ON(total_len > sizeof(buf));
> > -        BUG_ON(a->cpuid >= NCAPINTS * 32);
> > +        if ( a->repl_len > total_len )
> > +        {
> > +            printk(XENLOG_ERR
> > +                   "alt replacement size (%#x) bigger than destination (%#x)\n",
> 
> These all say "some alternative went wrong", without identifying which. 
> For x86_decode_lite(), debugging was far easier when using:
> 
> "Alternative for %ps ...", ALT_ORIG_PTR(a)
> 
> If we get the order of loading correct, then %ps will even work for a
> livepatch, but that's secondary - even the raw number is slightly useful
> given the livepatch load address.

I don't think this will work as-is for livepatches.  The call to
register the virtual region is currently done in livepatch_upload(),
after load_payload_data() has completed.

We could see about registering the virtual region earlier (no
volunteering to do that work right now).

> I could be talked down to just "Alt for %ps" as you've got it here.  I
> think it's clear enough in context.  So, I'd recommend:
> 
> "Alt for %ps, replacement size %#x larger than origin %#x\n".
> 
> Here, I think origin is better than destination, when discussing
> alternatives.

Sure.

> I can adjust on commit.  Everything else is fine.

If you are comfortable with doing the adjustments on commit, please go
ahead.

Thanks, Roger.

Re: [PATCH v2 2/6] xen/livepatch: zero pointer to temporary load buffer

[Andrew Cooper]

On 25/09/2024 11:00 am, Roger Pau Monné wrote:
> On Wed, Sep 25, 2024 at 10:33:39AM +0100, Andrew Cooper wrote:
>> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
>>> The livepatch_elf_sec data field points to the temporary load buffer, it's the
>>> load_addr field that points to the stable loaded section data.  Zero the data
>>> field once load_addr is set, as it would otherwise become a dangling pointer
>>> once the load buffer is freed.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> Changes since v1:
>>>  - New in this version.
>>> ---
>>>  xen/common/livepatch.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
>>> index df41dcce970a..87b3db03e26d 100644
>>> --- a/xen/common/livepatch.c
>>> +++ b/xen/common/livepatch.c
>>> @@ -383,6 +383,9 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
>>>              }
>>>              else
>>>                  memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
>>> +
>>> +            /* Avoid leaking pointers to temporary load buffers. */
>>> +            elf->sec[i].data = NULL;
>>>          }
>>>      }
>>>  
>> Where is the data allocated and freed?
>>
>> I don't see it being freed in this loop, so how is freed subsequently?
> It's allocated and freed by livepatch_upload(), it's the raw_data
> buffer that's allocated in the context of that function.

Well, this is a mess isn't it.

Ok, so livepatch_upload() makes a temporary buffer to copy everything into.

In elf_resolve_sections(), we set up sec[i].data pointing into this
temporary buffer.

And here, we copy the data from the temporary buffer, into the final
destination in the Xen .text/data/rodata region.

So yes, this does end up being a dangling pointer, and clobbering it is
good.


But, seeing the `n = sec->load_addr ?: sec->data` in patch 4, wouldn't
it be better to drop this second pointer and just have move_payload()
update it here?

I can't see anything good which can come from having two addresses, nor
a reason why we'd need both.

Then again, if this is too hard to arrange, it probably can be left as
an exercise to a future developer.

~Andrew

Re: [PATCH v2 2/6] xen/livepatch: zero pointer to temporary load buffer

[Roger Pau Monné]

On Wed, Sep 25, 2024 at 07:28:42PM +0100, Andrew Cooper wrote:
> On 25/09/2024 11:00 am, Roger Pau Monné wrote:
> > On Wed, Sep 25, 2024 at 10:33:39AM +0100, Andrew Cooper wrote:
> >> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> >>> The livepatch_elf_sec data field points to the temporary load buffer, it's the
> >>> load_addr field that points to the stable loaded section data.  Zero the data
> >>> field once load_addr is set, as it would otherwise become a dangling pointer
> >>> once the load buffer is freed.
> >>>
> >>> No functional change intended.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>> Changes since v1:
> >>>  - New in this version.
> >>> ---
> >>>  xen/common/livepatch.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> >>> index df41dcce970a..87b3db03e26d 100644
> >>> --- a/xen/common/livepatch.c
> >>> +++ b/xen/common/livepatch.c
> >>> @@ -383,6 +383,9 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >>>              }
> >>>              else
> >>>                  memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> >>> +
> >>> +            /* Avoid leaking pointers to temporary load buffers. */
> >>> +            elf->sec[i].data = NULL;
> >>>          }
> >>>      }
> >>>  
> >> Where is the data allocated and freed?
> >>
> >> I don't see it being freed in this loop, so how is freed subsequently?
> > It's allocated and freed by livepatch_upload(), it's the raw_data
> > buffer that's allocated in the context of that function.
> 
> Well, this is a mess isn't it.
> 
> Ok, so livepatch_upload() makes a temporary buffer to copy everything into.
> 
> In elf_resolve_sections(), we set up sec[i].data pointing into this
> temporary buffer.
> 
> And here, we copy the data from the temporary buffer, into the final
> destination in the Xen .text/data/rodata region.
> 
> So yes, this does end up being a dangling pointer, and clobbering it is
> good.
> 
> 
> But, seeing the `n = sec->load_addr ?: sec->data` in patch 4, wouldn't
> it be better to drop this second pointer and just have move_payload()
> update it here?

I didn't specially like the usage of either load_addr or data in patch
4.

I see, so move_payload() would replace ->data with the relocated
pointer.  One slightly nice thing about the current arrangement is
that data is const, with that change it should become non-const, as we
do modify the contents of load_addr in some case (like sorting the
exception table).  I don't think the constness of data was that
useful, as it's just the temporary buffer.

> I can't see anything good which can come from having two addresses, nor
> a reason why we'd need both.
> 
> Then again, if this is too hard to arrange, it probably can be left as
> an exercise to a future developer.

I can see if it's mostly a trivial change.

Thanks, Roger.

Re: [PATCH v2 2/6] xen/livepatch: zero pointer to temporary load buffer

[Roger Pau Monné]

On Wed, Sep 25, 2024 at 07:28:42PM +0100, Andrew Cooper wrote:
> On 25/09/2024 11:00 am, Roger Pau Monné wrote:
> > On Wed, Sep 25, 2024 at 10:33:39AM +0100, Andrew Cooper wrote:
> >> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> >>> The livepatch_elf_sec data field points to the temporary load buffer, it's the
> >>> load_addr field that points to the stable loaded section data.  Zero the data
> >>> field once load_addr is set, as it would otherwise become a dangling pointer
> >>> once the load buffer is freed.
> >>>
> >>> No functional change intended.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>> Changes since v1:
> >>>  - New in this version.
> >>> ---
> >>>  xen/common/livepatch.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> >>> index df41dcce970a..87b3db03e26d 100644
> >>> --- a/xen/common/livepatch.c
> >>> +++ b/xen/common/livepatch.c
> >>> @@ -383,6 +383,9 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >>>              }
> >>>              else
> >>>                  memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> >>> +
> >>> +            /* Avoid leaking pointers to temporary load buffers. */
> >>> +            elf->sec[i].data = NULL;
> >>>          }
> >>>      }
> >>>  
> >> Where is the data allocated and freed?
> >>
> >> I don't see it being freed in this loop, so how is freed subsequently?
> > It's allocated and freed by livepatch_upload(), it's the raw_data
> > buffer that's allocated in the context of that function.
> 
> Well, this is a mess isn't it.
> 
> Ok, so livepatch_upload() makes a temporary buffer to copy everything into.
> 
> In elf_resolve_sections(), we set up sec[i].data pointing into this
> temporary buffer.
> 
> And here, we copy the data from the temporary buffer, into the final
> destination in the Xen .text/data/rodata region.
> 
> So yes, this does end up being a dangling pointer, and clobbering it is
> good.
> 
> 
> But, seeing the `n = sec->load_addr ?: sec->data` in patch 4, wouldn't
> it be better to drop this second pointer and just have move_payload()
> update it here?
> 
> I can't see anything good which can come from having two addresses, nor
> a reason why we'd need both.
> 
> Then again, if this is too hard to arrange, it probably can be left as
> an exercise to a future developer.

So this is how it ends up looking,  there's a bit of churn due to
having to drop const on some function parameters.

diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c
index d50066564666..d1a89dde6ea3 100644
--- a/xen/arch/arm/arm32/livepatch.c
+++ b/xen/arch/arm/arm32/livepatch.c
@@ -243,7 +243,7 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
 
             symndx = ELF32_R_SYM(r_a->r_info);
             type = ELF32_R_TYPE(r_a->r_info);
-            dest = base->load_addr + r_a->r_offset; /* P */
+            dest = base->data + r_a->r_offset; /* P */
             addend = r_a->r_addend;
         }
         else
@@ -252,7 +252,7 @@ int arch_livepatch_perform(struct livepatch_elf *elf,
 
             symndx = ELF32_R_SYM(r->r_info);
             type = ELF32_R_TYPE(r->r_info);
-            dest = base->load_addr + r->r_offset; /* P */
+            dest = base->data + r->r_offset; /* P */
             addend = get_addend(type, dest);
         }
 
diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c
index dfb72be44fb8..46a390da42a3 100644
--- a/xen/arch/arm/arm64/livepatch.c
+++ b/xen/arch/arm/arm64/livepatch.c
@@ -248,7 +248,7 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
     {
         const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
         unsigned int symndx = ELF64_R_SYM(r->r_info);
-        void *dest = base->load_addr + r->r_offset; /* P */
+        void *dest = base->data + r->r_offset; /* P */
         bool overflow_check = true;
         int ovf = 0;
         uint64_t val;
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index 4f76127e1f77..89a62b33cd9c 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -260,7 +260,7 @@ int arch_livepatch_perform_rela(struct livepatch_elf *elf,
     {
         const Elf_RelA *r = rela->data + i * rela->sec->sh_entsize;
         unsigned int symndx = ELF64_R_SYM(r->r_info);
-        uint8_t *dest = base->load_addr + r->r_offset;
+        uint8_t *dest = base->data + r->r_offset;
         uint64_t val;
 
         if ( symndx == STN_UNDEF )
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 87b3db03e26d..a74f2ffce683 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -371,21 +371,21 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
 
             ASSERT(offset[i] != UINT_MAX);
 
-            elf->sec[i].load_addr = buf + offset[i];
+            buf += offset[i];
 
             /* Don't copy NOBITS - such as BSS. */
             if ( elf->sec[i].sec->sh_type != SHT_NOBITS )
             {
-                memcpy(elf->sec[i].load_addr, elf->sec[i].data,
+                memcpy(buf, elf->sec[i].data,
                        elf->sec[i].sec->sh_size);
                 dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Loaded %s at %p\n",
-                        elf->name, elf->sec[i].name, elf->sec[i].load_addr);
+                        elf->name, elf->sec[i].name, buf);
             }
             else
-                memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
+                memset(buf, 0, elf->sec[i].sec->sh_size);
 
-            /* Avoid leaking pointers to temporary load buffers. */
-            elf->sec[i].data = NULL;
+            /* Replace the temporary buffer with the relocated one. */
+            elf->sec[i].data = buf;
         }
     }
 
@@ -619,7 +619,7 @@ static inline int livepatch_check_expectations(const struct payload *payload)
         break;                                                                            \
     if ( !section_ok(elf, __sec, sizeof(*hook)) || __sec->sec->sh_size != sizeof(*hook) ) \
         return -EINVAL;                                                                   \
-    hook = __sec->load_addr;                                                              \
+    hook = __sec->data;                                                              \
 } while (0)
 
 /*
@@ -633,7 +633,7 @@ static inline int livepatch_check_expectations(const struct payload *payload)
         break;                                                                            \
     if ( !section_ok(elf, __sec, sizeof(*hook)) )                                         \
         return -EINVAL;                                                                   \
-    hook = __sec->load_addr;                                                              \
+    hook = __sec->data;                                                              \
     nhooks = __sec->sec->sh_size / sizeof(*hook);                                         \
 } while (0)
 
@@ -653,7 +653,7 @@ static int prepare_payload(struct payload *payload,
         if ( !section_ok(elf, sec, sizeof(*payload->funcs)) )
             return -EINVAL;
 
-        payload->funcs = funcs = sec->load_addr;
+        payload->funcs = funcs = sec->data;
         payload->nfuncs = sec->sec->sh_size / sizeof(*payload->funcs);
 
         payload->fstate = xzalloc_array(typeof(*payload->fstate),
@@ -712,7 +712,7 @@ static int prepare_payload(struct payload *payload,
     {
         const struct payload *data;
 
-        n = sec->load_addr;
+        n = sec->data;
 
         if ( sec->sec->sh_size <= sizeof(*n) )
             return -EINVAL;
@@ -742,7 +742,7 @@ static int prepare_payload(struct payload *payload,
     sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_DEPENDS);
     if ( sec )
     {
-        n = sec->load_addr;
+        n = sec->data;
 
         if ( sec->sec->sh_size <= sizeof(*n) )
             return -EINVAL;
@@ -758,7 +758,7 @@ static int prepare_payload(struct payload *payload,
     sec = livepatch_elf_sec_by_name(elf, ELF_LIVEPATCH_XEN_DEPENDS);
     if ( sec )
     {
-        n = sec->load_addr;
+        n = sec->data;
 
         if ( sec->sec->sh_size <= sizeof(*n) )
             return -EINVAL;
@@ -797,8 +797,8 @@ static int prepare_payload(struct payload *payload,
         if ( !section_ok(elf, sec, sizeof(*region->frame[i].start)) )
             return -EINVAL;
 
-        region->frame[i].start = sec->load_addr;
-        region->frame[i].stop  = sec->load_addr + sec->sec->sh_size;
+        region->frame[i].start = sec->data;
+        region->frame[i].stop  = sec->data + sec->sec->sh_size;
     }
 
     sec = livepatch_elf_sec_by_name(elf, ".altinstructions");
@@ -846,8 +846,8 @@ static int prepare_payload(struct payload *payload,
             return -EINVAL;
         }
 
-        start = sec->load_addr;
-        end = sec->load_addr + sec->sec->sh_size;
+        start = sec->data;
+        end = sec->data + sec->sec->sh_size;
 
         for ( a = start; a < end; a++ )
         {
@@ -870,14 +870,14 @@ static int prepare_payload(struct payload *payload,
              * repl must be fully within .altinstr_replacement, even if the
              * replacement and the section happen to both have zero length.
              */
-            if ( repl               < repl_sec->load_addr ||
+            if ( repl               < repl_sec->data ||
                  a->repl_len        > repl_sec->sec->sh_size ||
-                 repl + a->repl_len > repl_sec->load_addr + repl_sec->sec->sh_size )
+                 repl + a->repl_len > repl_sec->data + repl_sec->sec->sh_size )
             {
                 printk(XENLOG_ERR LIVEPATCH
                        "%s Alternative repl %p+%#x outside .altinstr_replacement %p+%#"PRIxElfWord"\n",
                        elf->name, repl, a->repl_len,
-                       repl_sec->load_addr, repl_sec->sec->sh_size);
+                       repl_sec->data, repl_sec->sec->sh_size);
                 return -EINVAL;
             }
         }
@@ -899,8 +899,8 @@ static int prepare_payload(struct payload *payload,
         if ( !section_ok(elf, sec, sizeof(*region->ex)) )
             return -EINVAL;
 
-        s = sec->load_addr;
-        e = sec->load_addr + sec->sec->sh_size;
+        s = sec->data;
+        e = sec->data + sec->sec->sh_size;
 
         sort_exception_table(s ,e);
 
@@ -919,7 +919,7 @@ static int prepare_payload(struct payload *payload,
         if ( !section_ok(elf, sec, sizeof(*payload->metadata.data)) )
             return -EINVAL;
 
-        payload->metadata.data = sec->load_addr;
+        payload->metadata.data = sec->data;
         payload->metadata.len = sec->sec->sh_size;
 
         /* The metadata is required to consists of null terminated strings. */
diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
index 45d73912a3cd..8b0076eb4f87 100644
--- a/xen/common/livepatch_elf.c
+++ b/xen/common/livepatch_elf.c
@@ -44,7 +44,7 @@ static int elf_verify_strtab(const struct livepatch_elf_sec *sec)
     return 0;
 }
 
-static int elf_resolve_sections(struct livepatch_elf *elf, const void *data)
+static int elf_resolve_sections(struct livepatch_elf *elf, void *data)
 {
     struct livepatch_elf_sec *sec;
     unsigned int i;
@@ -342,7 +342,7 @@ int livepatch_elf_resolve_symbols(struct livepatch_elf *elf)
                 break;
             }
 
-            st_value += (unsigned long)elf->sec[idx].load_addr;
+            st_value += (unsigned long)elf->sec[idx].data;
             if ( elf->sym[i].name )
                 dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Symbol resolved: %s => %#"PRIxElfAddr" (%s)\n",
                        elf->name, elf->sym[i].name,
@@ -503,7 +503,7 @@ static int livepatch_header_check(const struct livepatch_elf *elf)
     return 0;
 }
 
-int livepatch_elf_load(struct livepatch_elf *elf, const void *data)
+int livepatch_elf_load(struct livepatch_elf *elf, void *data)
 {
     int rc;
 
diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
index 7116deaddc28..dd0a8c65d79d 100644
--- a/xen/include/xen/livepatch_elf.h
+++ b/xen/include/xen/livepatch_elf.h
@@ -13,10 +13,11 @@ struct livepatch_elf_sec {
     const Elf_Shdr *sec;                 /* Hooked up in elf_resolve_sections.*/
     const char *name;                    /* Human readable name hooked in
                                             elf_resolve_section_names. */
-    const void *data;                    /* Pointer to the section (done by
-                                            elf_resolve_sections). */
-    void *load_addr;                     /* A pointer to the allocated destination.
-                                            Done by load_payload_data. */
+    void *data;                          /*
+                                          * Pointer to the section.  Note this
+                                          * can either be the temporary buffer
+                                          * or the relocated data.
+                                          */
 };
 
 struct livepatch_elf_sym {
@@ -41,7 +42,7 @@ struct livepatch_elf {
 const struct livepatch_elf_sec *
 livepatch_elf_sec_by_name(const struct livepatch_elf *elf,
                           const char *name);
-int livepatch_elf_load(struct livepatch_elf *elf, const void *data);
+int livepatch_elf_load(struct livepatch_elf *elf, void *data);
 void livepatch_elf_free(struct livepatch_elf *elf);
 
 int livepatch_elf_resolve_symbols(struct livepatch_elf *elf);

Re: [PATCH v2 2/6] xen/livepatch: zero pointer to temporary load buffer

[Andrew Cooper]

On 26/09/2024 9:22 am, Roger Pau Monné wrote:
> On Wed, Sep 25, 2024 at 07:28:42PM +0100, Andrew Cooper wrote:
>> On 25/09/2024 11:00 am, Roger Pau Monné wrote:
>>> On Wed, Sep 25, 2024 at 10:33:39AM +0100, Andrew Cooper wrote:
>>>> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
>>>>> The livepatch_elf_sec data field points to the temporary load buffer, it's the
>>>>> load_addr field that points to the stable loaded section data.  Zero the data
>>>>> field once load_addr is set, as it would otherwise become a dangling pointer
>>>>> once the load buffer is freed.
>>>>>
>>>>> No functional change intended.
>>>>>
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>> ---
>>>>> Changes since v1:
>>>>>  - New in this version.
>>>>> ---
>>>>>  xen/common/livepatch.c | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
>>>>> index df41dcce970a..87b3db03e26d 100644
>>>>> --- a/xen/common/livepatch.c
>>>>> +++ b/xen/common/livepatch.c
>>>>> @@ -383,6 +383,9 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
>>>>>              }
>>>>>              else
>>>>>                  memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
>>>>> +
>>>>> +            /* Avoid leaking pointers to temporary load buffers. */
>>>>> +            elf->sec[i].data = NULL;
>>>>>          }
>>>>>      }
>>>>>  
>>>> Where is the data allocated and freed?
>>>>
>>>> I don't see it being freed in this loop, so how is freed subsequently?
>>> It's allocated and freed by livepatch_upload(), it's the raw_data
>>> buffer that's allocated in the context of that function.
>> Well, this is a mess isn't it.
>>
>> Ok, so livepatch_upload() makes a temporary buffer to copy everything into.
>>
>> In elf_resolve_sections(), we set up sec[i].data pointing into this
>> temporary buffer.
>>
>> And here, we copy the data from the temporary buffer, into the final
>> destination in the Xen .text/data/rodata region.
>>
>> So yes, this does end up being a dangling pointer, and clobbering it is
>> good.
>>
>>
>> But, seeing the `n = sec->load_addr ?: sec->data` in patch 4, wouldn't
>> it be better to drop this second pointer and just have move_payload()
>> update it here?
>>
>> I can't see anything good which can come from having two addresses, nor
>> a reason why we'd need both.
>>
>> Then again, if this is too hard to arrange, it probably can be left as
>> an exercise to a future developer.
> So this is how it ends up looking,  there's a bit of churn due to
> having to drop const on some function parameters.

Looks fine to me.

I'd be tempted to name the final field 'addr' because for most of its
life it is the load address.

For the comment on the field, I'd say "this is first a temporary buffer,
then later the load address".

~Andrew

Re: [PATCH v2 2/6] xen/livepatch: zero pointer to temporary load buffer

[Roger Pau Monné]

On Thu, Sep 26, 2024 at 09:24:56AM +0100, Andrew Cooper wrote:
> On 26/09/2024 9:22 am, Roger Pau Monné wrote:
> > On Wed, Sep 25, 2024 at 07:28:42PM +0100, Andrew Cooper wrote:
> >> On 25/09/2024 11:00 am, Roger Pau Monné wrote:
> >>> On Wed, Sep 25, 2024 at 10:33:39AM +0100, Andrew Cooper wrote:
> >>>> On 25/09/2024 9:42 am, Roger Pau Monne wrote:
> >>>>> The livepatch_elf_sec data field points to the temporary load buffer, it's the
> >>>>> load_addr field that points to the stable loaded section data.  Zero the data
> >>>>> field once load_addr is set, as it would otherwise become a dangling pointer
> >>>>> once the load buffer is freed.
> >>>>>
> >>>>> No functional change intended.
> >>>>>
> >>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>>>> ---
> >>>>> Changes since v1:
> >>>>>  - New in this version.
> >>>>> ---
> >>>>>  xen/common/livepatch.c | 3 +++
> >>>>>  1 file changed, 3 insertions(+)
> >>>>>
> >>>>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> >>>>> index df41dcce970a..87b3db03e26d 100644
> >>>>> --- a/xen/common/livepatch.c
> >>>>> +++ b/xen/common/livepatch.c
> >>>>> @@ -383,6 +383,9 @@ static int move_payload(struct payload *payload, struct livepatch_elf *elf)
> >>>>>              }
> >>>>>              else
> >>>>>                  memset(elf->sec[i].load_addr, 0, elf->sec[i].sec->sh_size);
> >>>>> +
> >>>>> +            /* Avoid leaking pointers to temporary load buffers. */
> >>>>> +            elf->sec[i].data = NULL;
> >>>>>          }
> >>>>>      }
> >>>>>  
> >>>> Where is the data allocated and freed?
> >>>>
> >>>> I don't see it being freed in this loop, so how is freed subsequently?
> >>> It's allocated and freed by livepatch_upload(), it's the raw_data
> >>> buffer that's allocated in the context of that function.
> >> Well, this is a mess isn't it.
> >>
> >> Ok, so livepatch_upload() makes a temporary buffer to copy everything into.
> >>
> >> In elf_resolve_sections(), we set up sec[i].data pointing into this
> >> temporary buffer.
> >>
> >> And here, we copy the data from the temporary buffer, into the final
> >> destination in the Xen .text/data/rodata region.
> >>
> >> So yes, this does end up being a dangling pointer, and clobbering it is
> >> good.
> >>
> >>
> >> But, seeing the `n = sec->load_addr ?: sec->data` in patch 4, wouldn't
> >> it be better to drop this second pointer and just have move_payload()
> >> update it here?
> >>
> >> I can't see anything good which can come from having two addresses, nor
> >> a reason why we'd need both.
> >>
> >> Then again, if this is too hard to arrange, it probably can be left as
> >> an exercise to a future developer.
> > So this is how it ends up looking,  there's a bit of churn due to
> > having to drop const on some function parameters.
> 
> Looks fine to me.
> 
> I'd be tempted to name the final field 'addr' because for most of its
> life it is the load address.

I've changed to `addr`.  I however feel it's kind of redundant to name
a pointer field `addr`, as by the type itself (being a pointer) it's
clear it's an address.

> For the comment on the field, I'd say "this is first a temporary buffer,
> then later the load address".

Adjusted.

Thanks, Roger.