diff mbox series

[livepatch:,independ.,modules,1/3] livepatch: Always check hypervisor build ID upon hotpatch upload

Message ID 20190416125832.32881-1-wipawel@amazon.de (mailing list archive)
State New, archived
Headers show
Series [livepatch:,independ.,modules,1/3] livepatch: Always check hypervisor build ID upon hotpatch upload | expand

Commit Message

Wieczorkiewicz, Pawel April 16, 2019, 12:58 p.m. UTC 
This change is part of a independant stacked hotpatch modules
feature. This feature allows to bypass dependencies between modules
upon loading, but still verifies Xen build ID matching.

In order to prevent (up)loading any hotpatches built for different
hypervisor version as indicated by the Xen Build ID, add checking for
the payload's vs Xen's build id match.

To achieve that embed into every hotpatch another section with a
dedicated hypervisor build id in it. After the payload is loaded and
the .livepatch.xen_depends section becomes available, perform the
check and reject the payload if there is no match.

Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
Reviewed-by: Bjoern Doebel <doebel@amazon.de>
Reviewed-by: Eslam Elnikety <elnikety@amazon.de>
Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
---
 xen/common/livepatch.c      | 47 +++++++++++++++++++++++++++++++++++++++++++++
 xen/include/xen/livepatch.h |  7 ++++---
 2 files changed, 51 insertions(+), 3 deletions(-)

Comments

Konrad Rzeszutek Wilk April 23, 2019, 3:47 p.m. UTC | #1 
On Tue, Apr 16, 2019 at 12:58:30PM +0000, Pawel Wieczorkiewicz wrote:
> This change is part of a independant stacked hotpatch modules
> feature. This feature allows to bypass dependencies between modules
> upon loading, but still verifies Xen build ID matching.

OK, so build_id_dep would not be called for those?
I see patch #2 doing this. Cool.

> 
> In order to prevent (up)loading any hotpatches built for different
> hypervisor version as indicated by the Xen Build ID, add checking for
> the payload's vs Xen's build id match.
> 
> To achieve that embed into every hotpatch another section with a
> dedicated hypervisor build id in it. After the payload is loaded and
> the .livepatch.xen_depends section becomes available, perform the
> check and reject the payload if there is no match.
> 
> Signed-off-by: Pawel Wieczorkiewicz <wipawel@amazon.de>
> Reviewed-by: Andra-Irina Paraschiv <andraprs@amazon.com>
> Reviewed-by: Bjoern Doebel <doebel@amazon.de>
> Reviewed-by: Eslam Elnikety <elnikety@amazon.de>
> Reviewed-by: Martin Pohlack <mpohlack@amazon.de>
> ---
>  xen/common/livepatch.c      | 47 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/include/xen/livepatch.h |  7 ++++---

Can you please include:

- Changes to the docs/misc/livepatch.markdown describing this change.
- Include a test-case exercising this.
- Fix the test-cases as I think they will now all fail? (As they don't have this section?)

Thank you.

>  2 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index d6eaae6d3b..6a4af6ce57 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -74,6 +74,7 @@ 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. */
>      unsigned int n_load_funcs;           /* Nr of the funcs to load and execute. */
> @@ -476,11 +477,34 @@ static bool section_ok(const struct livepatch_elf *elf,
>      return true;
>  }
>  
> +static int check_xen_build_id(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) ) {
> +        dprintk(XENLOG_ERR, "%s%s: check against hypervisor build-id failed!\n",
> +                LIVEPATCH, payload->name);
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
>  static int check_special_sections(const struct livepatch_elf *elf)
>  {
>      unsigned int i;
>      static const char *const names[] = { ELF_LIVEPATCH_FUNC,
>                                           ELF_LIVEPATCH_DEPENDS,
> +                                         ELF_LIVEPATCH_XEN_DEPENDS,
>                                           ELF_BUILD_ID_NOTE};
>      DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 };
>  
> @@ -632,6 +656,22 @@ static int prepare_payload(struct payload *payload,
>              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;
> +
> +        if ( !payload->xen_dep.len || !payload->xen_dep.p )
> +            return -EINVAL;
> +    }
> +
>      /* Setup the virtual region with proper data. */
>      region = &payload->region;
>  
> @@ -882,6 +922,10 @@ static int load_payload_data(struct payload *payload, void *raw, size_t len)
>      if ( rc )
>          goto out;
>  
> +    rc = check_xen_build_id(payload);
> +    if ( rc )
> +        goto out;
> +
>      rc = build_symbol_table(payload, &elf);
>      if ( rc )
>          goto out;
> @@ -1655,6 +1699,9 @@ static void 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.h b/xen/include/xen/livepatch.h
> index 1b1817ca0d..ed997aa4cc 100644
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -29,9 +29,10 @@ struct xen_sysctl_livepatch_op;
>  /* Convenience define for printk. */
>  #define LIVEPATCH             "livepatch: "
>  /* ELF payload special section names. */
> -#define ELF_LIVEPATCH_FUNC    ".livepatch.funcs"
> -#define ELF_LIVEPATCH_DEPENDS ".livepatch.depends"
> -#define ELF_BUILD_ID_NOTE      ".note.gnu.build-id"
> +#define ELF_LIVEPATCH_FUNC        ".livepatch.funcs"
> +#define ELF_LIVEPATCH_DEPENDS     ".livepatch.depends"
> +#define ELF_LIVEPATCH_XEN_DEPENDS ".livepatch.xen_depends"
> +#define ELF_BUILD_ID_NOTE         ".note.gnu.build-id"
>  /* Arbitrary limit for payload size and .bss section size. */
>  #define LIVEPATCH_MAX_SIZE     MB(2)
>  
> -- 
> 2.16.5
> 
> 
> 
> 
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
> Ust-ID: DE 289 237 879
> Eingetragen am Amtsgericht Charlottenburg HRB 149173 B
> 
>
diff mbox series

Patch

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index d6eaae6d3b..6a4af6ce57 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -74,6 +74,7 @@  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. */
     unsigned int n_load_funcs;           /* Nr of the funcs to load and execute. */
@@ -476,11 +477,34 @@  static bool section_ok(const struct livepatch_elf *elf,
     return true;
 }
 
+static int check_xen_build_id(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) ) {
+        dprintk(XENLOG_ERR, "%s%s: check against hypervisor build-id failed!\n",
+                LIVEPATCH, payload->name);
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
 static int check_special_sections(const struct livepatch_elf *elf)
 {
     unsigned int i;
     static const char *const names[] = { ELF_LIVEPATCH_FUNC,
                                          ELF_LIVEPATCH_DEPENDS,
+                                         ELF_LIVEPATCH_XEN_DEPENDS,
                                          ELF_BUILD_ID_NOTE};
     DECLARE_BITMAP(found, ARRAY_SIZE(names)) = { 0 };
 
@@ -632,6 +656,22 @@  static int prepare_payload(struct payload *payload,
             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;
+
+        if ( !payload->xen_dep.len || !payload->xen_dep.p )
+            return -EINVAL;
+    }
+
     /* Setup the virtual region with proper data. */
     region = &payload->region;
 
@@ -882,6 +922,10 @@  static int load_payload_data(struct payload *payload, void *raw, size_t len)
     if ( rc )
         goto out;
 
+    rc = check_xen_build_id(payload);
+    if ( rc )
+        goto out;
+
     rc = build_symbol_table(payload, &elf);
     if ( rc )
         goto out;
@@ -1655,6 +1699,9 @@  static void 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.h b/xen/include/xen/livepatch.h
index 1b1817ca0d..ed997aa4cc 100644
--- a/xen/include/xen/livepatch.h
+++ b/xen/include/xen/livepatch.h
@@ -29,9 +29,10 @@  struct xen_sysctl_livepatch_op;
 /* Convenience define for printk. */
 #define LIVEPATCH             "livepatch: "
 /* ELF payload special section names. */
-#define ELF_LIVEPATCH_FUNC    ".livepatch.funcs"
-#define ELF_LIVEPATCH_DEPENDS ".livepatch.depends"
-#define ELF_BUILD_ID_NOTE      ".note.gnu.build-id"
+#define ELF_LIVEPATCH_FUNC        ".livepatch.funcs"
+#define ELF_LIVEPATCH_DEPENDS     ".livepatch.depends"
+#define ELF_LIVEPATCH_XEN_DEPENDS ".livepatch.xen_depends"
+#define ELF_BUILD_ID_NOTE         ".note.gnu.build-id"
 /* Arbitrary limit for payload size and .bss section size. */
 #define LIVEPATCH_MAX_SIZE     MB(2)