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

[Martin Pohlack <mpohlack@amazon.com>]

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.

>      }
>  
>      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.

>  #define XEN_BANNER		\
> diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h
> index 81a3c7d..02f9585 100644
> --- a/xen/include/xen/version.h
> +++ b/xen/include/xen/version.h
> @@ -12,5 +12,6 @@ unsigned int xen_minor_version(void);
>  const char *xen_extra_version(void);
>  const char *xen_changeset(void);
>  const char *xen_banner(void);
> +const char *xen_build_id(void);
>  
>  #endif /* __XEN_VERSION_H__ */
> 

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