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 printing the expected Xen build ID has part of dumping the payload
information is no longer done: 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 v2:
 - Move contents of xen_build_id_dep() into check_xen_buildid().

Changes since v1:
 - Do the Xen build-id check even earlier.
---
 xen/common/livepatch.c              | 86 +++++++++++++++++------------
 xen/include/xen/livepatch_payload.h |  1 -
 2 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 50e2268e19a3..f7db4be96e66 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -448,28 +448,6 @@ static bool section_ok(const struct livepatch_elf *elf,
     return true;
 }
 
-static int xen_build_id_dep(const struct payload *payload)
-{
-    const void *id = NULL;
-    unsigned int len = 0;
-    int rc;
-
-    ASSERT(payload->xen_dep.len);
-    ASSERT(payload->xen_dep.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);
-        return -EINVAL;
-    }
-
-    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)
@@ -495,11 +473,56 @@ static int parse_buildid(const struct livepatch_elf_sec *sec,
    return 0;
 }
 
+static int check_xen_buildid(const struct livepatch_elf *elf)
+{
+    const void *id;
+    unsigned int len;
+    struct livepatch_build_id lp_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: section %s is missing\n",
+               elf->name, ELF_LIVEPATCH_XEN_DEPENDS);
+        return -EINVAL;
+    }
+
+    rc = parse_buildid(sec, &lp_id);
+    if ( rc )
+    {
+        printk(XENLOG_ERR LIVEPATCH
+               "%s: failed to parse section %s as build-id: %d\n",
+               elf->name, ELF_LIVEPATCH_XEN_DEPENDS, rc);
+        return -EINVAL;
+    }
+
+    rc = xen_build_id(&id, &len);
+    if ( rc )
+    {
+        printk(XENLOG_ERR LIVEPATCH
+               "%s: unable to get running Xen build-id: %d\n",
+               elf->name, rc);
+        return rc;
+    }
+
+    if ( lp_id.len != len || memcmp(id, lp_id.p, len) )
+    {
+        printk(XENLOG_ERR LIVEPATCH "%s: build-id mismatch:\n"
+                                    "  livepatch: %*phN\n"
+                                    "        xen: %*phN\n",
+               elf->name, lp_id.len, lp_id.p, len, id);
+        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 +778,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 +1086,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 +1114,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 +2216,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

On Thu, Sep 26, 2024 at 11:21 AM Roger Pau Monne <roger.pau@citrix.com> 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 printing the expected Xen build ID has part of dumping the payload
> information is no longer done: 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>

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

Should the ELF_LIVEPATCH_XEN_DEPENDS check also be dropped from
check_special_sections() since it is no longer necessary?

Ross

On Thu, Sep 26, 2024 at 05:11:19PM +0100, Ross Lagerwall wrote:
> On Thu, Sep 26, 2024 at 11:21 AM Roger Pau Monne <roger.pau@citrix.com> 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 printing the expected Xen build ID has part of dumping the payload
> > information is no longer done: 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>
> 
> Reviewed-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Should the ELF_LIVEPATCH_XEN_DEPENDS check also be dropped from
> check_special_sections() since it is no longer necessary?

It's dropped from check_special_sections() in this patch, just not
mentioned in the commit message I'm afraid.

Thanks, Roger.

On 26/09/2024 11:14 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 printing the expected Xen build ID has part of dumping the payload
> information is no longer done: 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>

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