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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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 = .;