Re: [RFC PATCH v3.1 2/2] xsplice: Add hook for build_id

[Martin Pohlack <mpohlack@amazon.com>]

On 05.08.2015 10:58, Andrew Cooper wrote:
> On 05/08/15 09:50, Martin Pohlack wrote:
>> On 27.07.2015 21:20, Konrad Rzeszutek Wilk wrote:
>>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>>> ---
>>>  tools/libxc/xc_private.c     |  3 +++
>>>  tools/misc/xen-xsplice.c     | 25 +++++++++++++++++++++++++
>>>  xen/common/kernel.c          | 11 +++++++++++
>>>  xen/common/version.c         |  5 +++++
>>>  xen/include/public/version.h |  4 ++++
>>>  xen/include/xen/compile.h.in |  1 +
>>>  xen/include/xen/version.h    |  1 +
>>>  7 files changed, 50 insertions(+)
>>>
>>> diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
>>> index 2ffebd9..7c039ca 100644
>>> --- a/tools/libxc/xc_private.c
>>> +++ b/tools/libxc/xc_private.c
>>> @@ -713,6 +713,9 @@ int xc_version(xc_interface *xch, int cmd, void *arg)
>>>      case XENVER_commandline:
>>>          sz = sizeof(xen_commandline_t);
>>>          break;
>>> +    case XENVER_build_id:
>>> +        sz = sizeof(xen_build_id_t);
>>> +        break;
>>>      default:
>>>          ERROR("xc_version: unknown command %d\n", cmd);
>>>          return -EINVAL;
>>> diff --git a/tools/misc/xen-xsplice.c b/tools/misc/xen-xsplice.c
>>> index 7cf9879..dd8266c 100644
>>> --- a/tools/misc/xen-xsplice.c
>>> +++ b/tools/misc/xen-xsplice.c
>>> @@ -17,6 +17,7 @@ void show_help(void)
>>>              " <id> An unique name of payload. Up to 40 characters.\n"
>>>              "Commands:\n"
>>>              "  help                 display this help\n"
>>> +            "  build-id             display build-id of hypervisor.\n"
>>>              "  upload <id> <file>   upload file <cpuid> with <id> name\n"
>>>              "  list                 list payloads uploaded.\n"
>>>              "  apply <id>           apply <id> patch.\n"
>>> @@ -306,12 +307,36 @@ int action_func(int argc, char *argv[], unsigned int idx)
>>>  
>>>      return rc;
>>>  }
>>> +
>>> +static int build_id_func(int argc, char *argv[])
>>> +{
>>> +    xen_build_id_t build_id;
>>> +
>>> +    if ( argc )
>>> +    {
>>> +        show_help();
>>> +        return -1;
>>> +    }
>>> +
>>> +    memset(build_id, 0, sizeof(*build_id));
>>> +
>>> +    if ( xc_version(xch, XENVER_build_id, &build_id) < 0 )
>>> +    {
>>> +        printf("Failed to get build_id: %d(%s)\n", errno, strerror(errno));
>>> +        return -1;
>>> +    }
>>> +
>>> +    printf("%s\n", build_id);
>>> +    return 0;
>>> +}
>>> +
>>>  struct {
>>>      const char *name;
>>>      int (*function)(int argc, char *argv[]);
>>>  } main_options[] = {
>>>      { "help", help_func },
>>>      { "list", list_func },
>>> +    { "build-id", build_id_func },
>>>      { "upload", upload_func },
>>>  };
>>>  
>>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
>>> index 6a3196a..e9d41b6 100644
>>> --- a/xen/common/kernel.c
>>> +++ b/xen/common/kernel.c
>>> @@ -357,6 +357,17 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>          if ( copy_to_guest(arg, saved_cmdline, ARRAY_SIZE(saved_cmdline)) )
>>>              return -EFAULT;
>>>          return 0;
>>> +
>>> +    case XENVER_build_id:
>>> +    {
>>> +        xen_build_id_t build_id;
>>> +
>>> +        memset(build_id, 0, sizeof(build_id));
>>> +        safe_strcpy(build_id, xen_build_id());
>> You seem to want to store and transfer the build_id as a string.  Any
>> reason why we don't directly expose the build_id embedded by the linker
>> in binary format?
>>
>>> +        if ( copy_to_guest(arg, build_id, ARRAY_SIZE(build_id)) )
>>> +            return -EFAULT;
>>> +        return 0;
>>> +    }
>> We should not expose the build_id to normal guests, but only to Dom0.
>>
>> A build_id uniquely identifies a specific build and I don't see how that
>> information would be required from DomU.  It might actually help an
>> attacker to build his return-oriented programming exploit against a
>> specific build.
>>
>> The normal version numbers should be enough to know about capabilities
>> and API.
> 
> It will need its own XSM hook, but need not be strictly limited to just
> dom0.
> 
>>
>>>      }
>>>  
>>>      return -ENOSYS;
>>> diff --git a/xen/common/version.c b/xen/common/version.c
>>> index b152e27..5c3dbb0 100644
>>> --- a/xen/common/version.c
>>> +++ b/xen/common/version.c
>>> @@ -55,3 +55,8 @@ const char *xen_banner(void)
>>>  {
>>>      return XEN_BANNER;
>>>  }
>>> +
>>> +const char *xen_build_id(void)
>>> +{
>>> +    return XEN_BUILD_ID;
>>> +}
>>> diff --git a/xen/include/public/version.h b/xen/include/public/version.h
>>> index 44f26b0..c863393 100644
>>> --- a/xen/include/public/version.h
>>> +++ b/xen/include/public/version.h
>>> @@ -83,6 +83,10 @@ typedef struct xen_feature_info xen_feature_info_t;
>>>  #define XENVER_commandline 9
>>>  typedef char xen_commandline_t[1024];
>>>  
>>> +#define XENVER_build_id 10
>>> +typedef char xen_build_id_t[1024];
>>> +#define XEN_BUILD_ID_LEN (sizeof(xen_build_id_t))
>>> +
>>>  #endif /* __XEN_PUBLIC_VERSION_H__ */
>>>  
>>>  /*
>>> diff --git a/xen/include/xen/compile.h.in b/xen/include/xen/compile.h.in
>>> index 440ecb2..939685e 100644
>>> --- a/xen/include/xen/compile.h.in
>>> +++ b/xen/include/xen/compile.h.in
>>> @@ -10,4 +10,5 @@
>>>  #define XEN_EXTRAVERSION	"@@extraversion@@"
>>>  
>>>  #define XEN_CHANGESET		"@@changeset@@"
>>> +#define XEN_BUILD_ID        "@@changeset@@"
>> That leads to a chicken and egg problem when embedding a real build_id.
>>  Some linker script magic seems to be required.  I will try to refine
>> the patch.
> 
> So funnily enough, I tried experimenting with this and it is fairly easy
> to get the basics done.
> 
> Further TODO which I havn't done yet is make the --build-id optional on
> finding a compatible `ld`, and some symbol magic to directly locate
> .note.gnu.build-id
> 
> However, this in addition to some of Konrad's original patch is a good
> start.
> 
> ~Andrew
> 
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index 5f24951..10938b2 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -112,7 +112,7 @@ $(TARGET)-syms: prelink.o xen.lds
> $(BASEDIR)/common/symbols-dummy.o
>             $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
>         $(NM) -n $(@D)/.$(@F).1 | $(BASEDIR)/tools/symbols >$(@D)/.$(@F).1.S
>         $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
> -       $(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
> +       $(LD) $(LDFLAGS) -T xen.lds -N prelink.o --build-id \
>             $(@D)/.$(@F).1.o -o $@
>         rm -f $(@D)/.$(@F).[0-9]*
>  
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 6553cff..46e6546 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -68,6 +68,13 @@ SECTIONS
>    } :text
>  
>    . = ALIGN(SMP_CACHE_BYTES);
> +  .notes : {
> +       __start_notes = .;
> +       *(.note.*)
> +       __end_notes = .;
> +  } :text
> +
> +  . = ALIGN(SMP_CACHE_BYTES);
>    .data.read_mostly : {
>         /* Exception table */
>         __start___ex_table = .;

And here is my version, also on-top of Konrad's series:

----------------------------------------------------------------------

[PATCH] xsplice: Use ld-embedded build-ids

Todo:
  * Should be moved to sysctl to only allow Dom0 access
  * Maybe convert to binary transport to userland instead of printable form

* use ld to actually embed the build ID
* convert to textual representation in hypervisor and report in
  printable form

Signed-off-by: Martin Pohlack <mpohlack@amazon.de>
---
 xen/arch/x86/Makefile        |  4 ++--
 xen/arch/x86/xen.lds.S       |  5 +++++
 xen/common/kernel.c          | 33 +++++++++++++++++++++++++++++----
 xen/common/version.c         |  5 -----
 xen/include/xen/compile.h.in |  1 -
 5 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 5f24951..f724bd8 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -108,11 +108,11 @@ $(TARGET)-syms: prelink.o xen.lds
$(BASEDIR)/common/symbols-dummy.o
 	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
 	$(NM) -n $(@D)/.$(@F).0 | $(BASEDIR)/tools/symbols >$(@D)/.$(@F).0.S
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0.o
-	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
+	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o --build-id=sha1 \
 	    $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
 	$(NM) -n $(@D)/.$(@F).1 | $(BASEDIR)/tools/symbols >$(@D)/.$(@F).1.S
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
-	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
+	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o --build-id=sha1 \
 	    $(@D)/.$(@F).1.o -o $@
 	rm -f $(@D)/.$(@F).[0-9]*

diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 6553cff..2176782 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -67,6 +67,11 @@ SECTIONS
        *(.rodata.*)
   } :text

+  .note.gnu.build-id : {
+      __note_gnu_build_id_start = .;
+      *(.note.gnu.build-id)
+  } :text
+
   . = ALIGN(SMP_CACHE_BYTES);
   .data.read_mostly : {
        /* Exception table */
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index e9d41b6..9814585 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -6,9 +6,11 @@

 #include <xen/init.h>
 #include <xen/lib.h>
+#include <xen/elf.h>
 #include <xen/errno.h>
 #include <xen/version.h>
 #include <xen/sched.h>
+#include <xen/types.h>
 #include <xen/paging.h>
 #include <xen/nmi.h>
 #include <xen/guest_access.h>
@@ -227,6 +229,10 @@ void __init do_initcalls(void)
  * Simple hypercalls.
  */

+#define NT_GNU_BUILD_ID 3
+
+extern char * __note_gnu_build_id_start;  /* defined in linker script */
+
 DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     switch ( cmd )
@@ -360,11 +366,30 @@ DO(xen_version)(int cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)

     case XENVER_build_id:
     {
-        xen_build_id_t build_id;
+        xen_build_id_t ascii_id;
+        Elf_Note * n = (Elf_Note *)&__note_gnu_build_id_start;
+        char * binary_id;
+        int i;
+
+        memset(ascii_id, 0, sizeof(ascii_id));
+
+        /* check if we really have a build-id */
+        if ( NT_GNU_BUILD_ID != n->type )
+            return 0;
+
+        /* sanity check, name should be "GNU" for ld-generated build-id */
+        if ( 0 != strncmp(ELFNOTE_NAME(n), "GNU", n->namesz))
+            return 0;
+
+        binary_id = (char *)ELFNOTE_DESC(n);
+
+        /* convert to printable format */
+        for (i = 0; i < n->descsz && (i + 1) * 2 <
sizeof(xen_build_id_t); i++)
+        {
+            snprintf(&ascii_id[i * 2], 3, "%02hhx", binary_id[i]);
+        }

-        memset(build_id, 0, sizeof(build_id));
-        safe_strcpy(build_id, xen_build_id());
-        if ( copy_to_guest(arg, build_id, ARRAY_SIZE(build_id)) )
+        if ( copy_to_guest(arg, ascii_id, ARRAY_SIZE(ascii_id)) )
             return -EFAULT;
         return 0;
     }
diff --git a/xen/common/version.c b/xen/common/version.c
index 5c3dbb0..b152e27 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -55,8 +55,3 @@ const char *xen_banner(void)
 {
     return XEN_BANNER;
 }
-
-const char *xen_build_id(void)
-{
-    return XEN_BUILD_ID;
-}
diff --git a/xen/include/xen/compile.h.in b/xen/include/xen/compile.h.in
index 939685e..440ecb2 100644
--- a/xen/include/xen/compile.h.in
+++ b/xen/include/xen/compile.h.in
@@ -10,5 +10,4 @@
 #define XEN_EXTRAVERSION	"@@extraversion@@"

 #define XEN_CHANGESET		"@@changeset@@"
-#define XEN_BUILD_ID        "@@changeset@@"
 #define XEN_BANNER		\
-- 
2.5.0

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B