[PATCH v2] Add build-id to XENVER hypercall.

[PATCH v2] Add build-id to XENVER hypercall.

[Konrad Rzeszutek Wilk]

Since v1 (http://lists.xen.org/archives/html/xen-devel/2015-10/msg01090.html)
 - Made it work on EFI
 - Compiles on ARM
 - Redid it per comments.

Attached are the three patches that will add XENVER_build_id and
add the proper bits in libxl/libxc.

However they also change the behavior of the existing hypercall
for XENVER_[compile_info|changeset|commandline|extraversion] - that
is they return the string '<denied>'.

This is with XSM enabled or disabled.

The new sub-ops - XENVER_build_id on the other hand will return
-EPERM (XSM or not) for !priv guests.

Please take a look and provide your feedback at your leisure.

 tools/flask/policy/policy/modules/xen/xen.te |  9 +++++
 tools/libxc/xc_private.c                     |  7 ++++
 tools/libxc/xc_private.h                     | 10 ++++++
 tools/libxl/libxl.c                          | 24 +++++++++++++
 tools/libxl/libxl.h                          |  5 +++
 tools/libxl/libxl_types.idl                  |  1 +
 tools/libxl/xl_cmdimpl.c                     |  1 +
 xen/arch/x86/Makefile                        | 14 +++++---
 xen/arch/x86/xen.lds.S                       |  6 ++++
 xen/common/kernel.c                          | 54 +++++++++++++++++++++++-----
 xen/common/version.c                         | 41 +++++++++++++++++++++
 xen/include/public/version.h                 | 15 +++++++-
 xen/include/xen/version.h                    |  2 ++
 xen/include/xsm/dummy.h                      | 18 ++++++++++
 xen/include/xsm/xsm.h                        |  6 ++++
 xen/xsm/dummy.c                              |  1 +
 xen/xsm/flask/hooks.c                        | 22 ++++++++++++
 xen/xsm/flask/policy/access_vectors          |  4 +++
 18 files changed, 226 insertions(+), 14 deletions(-)


Konrad Rzeszutek Wilk (2):
      xsm/xen_version: Add XSM for the xen_version hypercall.
      libxl: info: Display build_id of the hypervisor.

Martin Pohlack (1):
      XENVER_build_id: Provide ld-embedded build-ids

[PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Konrad Rzeszutek Wilk]

All of XENVER_* have now an XSM check.

The subops for XENVER_[compile_info|changeset|commandline|
extraversion] are now priviliged operations. To not break
guests we still return an string - but it is just '<denied>'.

The rest: XENVER_[version|capabilities|
parameters|get_features|page_size|guest_handle] behave
as before - allowed by default for all guests.

This is with the XSM default policy and with the dummy ones.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: Do XSM check for all the XENVER_ ops.
v3: Add empty data conditions.
v4: Return <denied> for priv subops.
---
 tools/flask/policy/policy/modules/xen/xen.te |  9 +++++++++
 xen/common/kernel.c                          | 25 +++++++++++++++++--------
 xen/common/version.c                         |  5 +++++
 xen/include/xen/version.h                    |  1 +
 xen/include/xsm/dummy.h                      | 18 ++++++++++++++++++
 xen/include/xsm/xsm.h                        |  6 ++++++
 xen/xsm/dummy.c                              |  1 +
 xen/xsm/flask/hooks.c                        | 22 ++++++++++++++++++++++
 xen/xsm/flask/policy/access_vectors          |  4 ++++
 9 files changed, 83 insertions(+), 8 deletions(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
index d35ae22..1ca0e65 100644
--- a/tools/flask/policy/policy/modules/xen/xen.te
+++ b/tools/flask/policy/policy/modules/xen/xen.te
@@ -73,6 +73,12 @@ allow dom0_t xen_t:xen2 {
     pmu_ctrl
     get_symbol
 };
+
+# Allow dom0 to use XENVER_compile_info|changeset|commandline]extraversion
+allow dom0_t xen_t:xen2 {
+    version_priv
+};
+
 allow dom0_t xen_t:mmu memorymap;
 
 # Allow dom0 to use these domctls on itself. For domctls acting on other
@@ -137,6 +143,9 @@ if (guest_writeconsole) {
 # pmu_ctrl is for)
 allow domain_type xen_t:xen2 pmu_use;
 
+# All all domains to use (unprivileged parts of) the XENVER_* hypercall
+allow domain_type xen_t:domain2 version_use;
+
 ###############################################################################
 #
 # Domain creation
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 6a3196a..d51a3ed 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -13,6 +13,7 @@
 #include <xen/nmi.h>
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
+#include <xsm/xsm.h>
 #include <asm/current.h>
 #include <public/nmi.h>
 #include <public/version.h>
@@ -226,9 +227,10 @@ void __init do_initcalls(void)
 /*
  * Simple hypercalls.
  */
-
 DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
+    bool_t deny = !!xsm_version_op(XSM_HOOK, cmd);
+
     switch ( cmd )
     {
     case XENVER_version:
@@ -239,7 +241,7 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         xen_extraversion_t extraversion;
 
         memset(extraversion, 0, sizeof(extraversion));
-        safe_strcpy(extraversion, xen_extra_version());
+        safe_strcpy(extraversion, deny ? xen_deny() : xen_extra_version());
         if ( copy_to_guest(arg, extraversion, ARRAY_SIZE(extraversion)) )
             return -EFAULT;
         return 0;
@@ -250,10 +252,10 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         xen_compile_info_t info;
 
         memset(&info, 0, sizeof(info));
-        safe_strcpy(info.compiler,       xen_compiler());
-        safe_strcpy(info.compile_by,     xen_compile_by());
-        safe_strcpy(info.compile_domain, xen_compile_domain());
-        safe_strcpy(info.compile_date,   xen_compile_date());
+        safe_strcpy(info.compiler,       deny ? xen_deny() : xen_compiler());
+        safe_strcpy(info.compile_by,     deny ? xen_deny() : xen_compile_by());
+        safe_strcpy(info.compile_domain, deny ? xen_deny() : xen_compile_domain());
+        safe_strcpy(info.compile_date,   deny ? xen_deny() : xen_compile_date());
         if ( copy_to_guest(arg, &info, 1) )
             return -EFAULT;
         return 0;
@@ -288,7 +290,7 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         xen_changeset_info_t chgset;
 
         memset(chgset, 0, sizeof(chgset));
-        safe_strcpy(chgset, xen_changeset());
+        safe_strcpy(chgset, deny ? xen_deny() : xen_changeset());
         if ( copy_to_guest(arg, chgset, ARRAY_SIZE(chgset)) )
             return -EFAULT;
         return 0;
@@ -354,10 +356,17 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         return 0;
 
     case XENVER_commandline:
-        if ( copy_to_guest(arg, saved_cmdline, ARRAY_SIZE(saved_cmdline)) )
+    {
+        size_t len = ARRAY_SIZE(saved_cmdline);
+
+        if ( deny )
+            len = strlen(xen_deny());
+
+        if ( copy_to_guest(arg, deny ? xen_deny() : saved_cmdline, len) )
             return -EFAULT;
         return 0;
     }
+    }
 
     return -ENOSYS;
 }
diff --git a/xen/common/version.c b/xen/common/version.c
index b152e27..786be4e 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_deny(void)
+{
+    return "<denied>";
+}
diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h
index 81a3c7d..2015c0b 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_deny(void);
 
 #endif /* __XEN_VERSION_H__ */
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 9fe372c..5088462 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -721,3 +721,21 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG struct domain *d, unsigned int
 }
 
 #endif /* CONFIG_X86 */
+
+#include <public/version.h>
+static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op)
+{
+    XSM_ASSERT_ACTION(XSM_HOOK);
+    switch ( op )
+    {
+    case XENVER_version:
+    case XENVER_capabilities:
+    case XENVER_platform_parameters:
+    case XENVER_get_features:
+    case XENVER_pagesize:
+    case XENVER_guest_handle:
+        return xsm_default_action(XSM_HOOK, current->domain, NULL);
+    default:
+        return xsm_default_action(XSM_PRIV, current->domain, NULL);
+    }
+}
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index ba3caed..e038cbd 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -191,6 +191,7 @@ struct xsm_operations {
     int (*ioport_mapping) (struct domain *d, uint32_t s, uint32_t e, uint8_t allow);
     int (*pmu_op) (struct domain *d, unsigned int op);
 #endif
+    int (*version_op) (uint32_t cmd);
 };
 
 #ifdef XSM_ENABLE
@@ -729,6 +730,11 @@ static inline int xsm_pmu_op (xsm_default_t def, struct domain *d, unsigned int
 
 #endif /* CONFIG_X86 */
 
+static inline int xsm_version_op (xsm_default_t def, uint32_t op)
+{
+    return xsm_ops->version_op(op);
+}
+
 #endif /* XSM_NO_WRAPPERS */
 
 #ifdef CONFIG_MULTIBOOT
diff --git a/xen/xsm/dummy.c b/xen/xsm/dummy.c
index 72eba40..6105c07 100644
--- a/xen/xsm/dummy.c
+++ b/xen/xsm/dummy.c
@@ -162,4 +162,5 @@ void xsm_fixup_ops (struct xsm_operations *ops)
     set_to_dummy_if_null(ops, ioport_mapping);
     set_to_dummy_if_null(ops, pmu_op);
 #endif
+    set_to_dummy_if_null(ops, version_op);
 }
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 4180f3b..d2b915e 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -26,6 +26,7 @@
 #include <public/xen.h>
 #include <public/physdev.h>
 #include <public/platform.h>
+#include <public/version.h>
 
 #include <public/xsm/flask_op.h>
 
@@ -1621,6 +1622,26 @@ static int flask_pmu_op (struct domain *d, unsigned int op)
 }
 #endif /* CONFIG_X86 */
 
+static int flask_version_op (uint32_t op)
+{
+    u32 dsid = domain_sid(current->domain);
+
+    switch ( op )
+    {
+    case XENVER_version:
+    case XENVER_capabilities:
+    case XENVER_platform_parameters:
+    case XENVER_get_features:
+    case XENVER_pagesize:
+    case XENVER_guest_handle:
+        return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_DOMAIN2,
+                            DOMAIN2__VERSION_USE, NULL);
+    default:
+        return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_XEN2,
+                            XEN2__VERSION_PRIV, NULL);
+    }
+}
+
 long do_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op);
 int compat_flask_op(XEN_GUEST_HANDLE_PARAM(xsm_op_t) u_flask_op);
 
@@ -1759,6 +1780,7 @@ static struct xsm_operations flask_ops = {
     .ioport_mapping = flask_ioport_mapping,
     .pmu_op = flask_pmu_op,
 #endif
+    .version_op = flask_version_op,
 };
 
 static __init void flask_init(void)
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index effb59f..273459f 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -93,6 +93,8 @@ class xen2
     pmu_ctrl
 # PMU use (domains, including unprivileged ones, will be using this operation)
     pmu_use
+# XENVER_[compile_info|changeset|commandline|extraversion] usage.
+   version_priv
 }
 
 # Classes domain and domain2 consist of operations that a domain performs on
@@ -242,6 +244,8 @@ class domain2
     mem_sharing
 # XEN_DOMCTL_psr_cat_op
     psr_cat_op
+# XENVER_[version|capabilities|parameters|get_features|page_size|guest_handle].
+    version_use
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
2.1.0

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Jan Beulich]

>>> On 06.11.15 at 20:36, <konrad.wilk@oracle.com> wrote:
> All of XENVER_* have now an XSM check.
> 
> The subops for XENVER_[compile_info|changeset|commandline|
> extraversion] are now priviliged operations. To not break
> guests we still return an string - but it is just '<denied>'.

And I continue to question at least the extraversion part.

> The rest: XENVER_[version|capabilities|
> parameters|get_features|page_size|guest_handle] behave
> as before - allowed by default for all guests.
> 
> This is with the XSM default policy and with the dummy ones.

And with a non-default policy you now ignore one of the latter
ops to also get denied.

> @@ -354,10 +356,17 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          return 0;
>  
>      case XENVER_commandline:
> -        if ( copy_to_guest(arg, saved_cmdline, ARRAY_SIZE(saved_cmdline)) )
> +    {
> +        size_t len = ARRAY_SIZE(saved_cmdline);
> +
> +        if ( deny )
> +            len = strlen(xen_deny());

+1 (or else you fail to nul-terminate the output).

Jan

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Konrad Rzeszutek Wilk]

On Tue, Nov 10, 2015 at 05:29:35AM -0700, Jan Beulich wrote:
> >>> On 06.11.15 at 20:36, <konrad.wilk@oracle.com> wrote:
> > All of XENVER_* have now an XSM check.
> > 
> > The subops for XENVER_[compile_info|changeset|commandline|
> > extraversion] are now priviliged operations. To not break
> > guests we still return an string - but it is just '<denied>'.
> 
> And I continue to question at least the extraversion part.

How about I remove the extraversion part from this patch and we can
discuss putting 'extraversion' in the blacklist around another patch.

> 
> > The rest: XENVER_[version|capabilities|
> > parameters|get_features|page_size|guest_handle] behave
> > as before - allowed by default for all guests.
> > 
> > This is with the XSM default policy and with the dummy ones.
> 
> And with a non-default policy you now ignore one of the latter
> ops to also get denied.

No, but that is due to the 'deny' being only checked for certain subops.

I think what you are saying is that for XENVER_[version|capabilities|
parameters|get_features|page_size|guest_handle] we should not have any
XSM checks as they serve no purpose (which is what I had in the earlier
versions of this patch). However Andrew mentioned that he would
like _ALL_ of the sub-ops to be checked for.

It feels like I am back to square one :-)

My feeling is that those subops I mentioned should have no XSM checks
at all so nobody can mess that up (thought it was fun messing that up).

However Andrew did not like that, but you seem to imply you like that.

Could you two hash out which way you would like this?

> 
> > @@ -354,10 +356,17 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >          return 0;
> >  
> >      case XENVER_commandline:
> > -        if ( copy_to_guest(arg, saved_cmdline, ARRAY_SIZE(saved_cmdline)) )
> > +    {
> > +        size_t len = ARRAY_SIZE(saved_cmdline);
> > +
> > +        if ( deny )
> > +            len = strlen(xen_deny());
> 
> +1 (or else you fail to nul-terminate the output).

Nice spotting!
Perhaps modifying xen_deny() to be:

const char *xen_deny(void)
{
    return "<denied>\0";
}

Would be better?

As this 'len' +1 would lead me to copy one byte extra from the .rodata
section where "<denied>" resides leaking to some information leak.

> Jan
> 

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Jan Beulich]

>>> On 06.01.16 at 18:41, <konrad.wilk@oracle.com> wrote:
> On Tue, Nov 10, 2015 at 05:29:35AM -0700, Jan Beulich wrote:
>> >>> On 06.11.15 at 20:36, <konrad.wilk@oracle.com> wrote:
>> > All of XENVER_* have now an XSM check.
>> > 
>> > The subops for XENVER_[compile_info|changeset|commandline|
>> > extraversion] are now priviliged operations. To not break
>> > guests we still return an string - but it is just '<denied>'.
>> 
>> And I continue to question at least the extraversion part.
> 
> How about I remove the extraversion part from this patch and we can
> discuss putting 'extraversion' in the blacklist around another patch.

Yes, that'd be a step towards me agreeing with the change. I'm
not necessarily saying that's going to be enough, since I said "at
least", i.e. I continue to wonder how relevant it really is to hide
changeset and compile info (fwiw I agree with hiding the command
line).

>> > The rest: XENVER_[version|capabilities|
>> > parameters|get_features|page_size|guest_handle] behave
>> > as before - allowed by default for all guests.
>> > 
>> > This is with the XSM default policy and with the dummy ones.
>> 
>> And with a non-default policy you now ignore one of the latter
>> ops to also get denied.
> 
> No, but that is due to the 'deny' being only checked for certain subops.

To me this reply seems contradictory in itself: The "no" doesn't
seem to match up with the rest.

> I think what you are saying is that for XENVER_[version|capabilities|
> parameters|get_features|page_size|guest_handle] we should not have any
> XSM checks as they serve no purpose (which is what I had in the earlier
> versions of this patch). However Andrew mentioned that he would
> like _ALL_ of the sub-ops to be checked for.

And I agree with Andrew, hence my earlier comment above (with
the reply I can't really make sense of).

>> > @@ -354,10 +356,17 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> >          return 0;
>> >  
>> >      case XENVER_commandline:
>> > -        if ( copy_to_guest(arg, saved_cmdline, ARRAY_SIZE(saved_cmdline)) )
>> > +    {
>> > +        size_t len = ARRAY_SIZE(saved_cmdline);
>> > +
>> > +        if ( deny )
>> > +            len = strlen(xen_deny());
>> 
>> +1 (or else you fail to nul-terminate the output).
> 
> Nice spotting!
> Perhaps modifying xen_deny() to be:
> 
> const char *xen_deny(void)
> {
>     return "<denied>\0";
> }
> 
> Would be better?

This would just add a second NUL at the end, without altering what
strlen() returns on that string.

Jan

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Konrad Rzeszutek Wilk]

> >> > The subops for XENVER_[compile_info|changeset|commandline|
> >> > extraversion] are now priviliged operations. To not break
> >> > guests we still return an string - but it is just '<denied>'.
> >> 
> >> And I continue to question at least the extraversion part.
> > 
> > How about I remove the extraversion part from this patch and we can
> > discuss putting 'extraversion' in the blacklist around another patch.
> 
> Yes, that'd be a step towards me agreeing with the change. I'm
> not necessarily saying that's going to be enough, since I said "at
> least", i.e. I continue to wonder how relevant it really is to hide
> changeset and compile info (fwiw I agree with hiding the command
> line).

I made it now only return '<denied>' for the XENVER_commandline.

> 
> >> > The rest: XENVER_[version|capabilities|
> >> > parameters|get_features|page_size|guest_handle] behave
> >> > as before - allowed by default for all guests.
> >> > 
> >> > This is with the XSM default policy and with the dummy ones.
> >> 
> >> And with a non-default policy you now ignore one of the latter
> >> ops to also get denied.
> > 
> > No, but that is due to the 'deny' being only checked for certain subops.
> 
> To me this reply seems contradictory in itself: The "no" doesn't
> seem to match up with the rest.
> 
> > I think what you are saying is that for XENVER_[version|capabilities|
> > parameters|get_features|page_size|guest_handle] we should not have any
> > XSM checks as they serve no purpose (which is what I had in the earlier
> > versions of this patch). However Andrew mentioned that he would
> > like _ALL_ of the sub-ops to be checked for.
> 
> And I agree with Andrew, hence my earlier comment above (with
> the reply I can't really make sense of).

I am all confused now.

There are two parts here:
 a) The XSM checks - which allow the XENVER_version..XENVER_guest_handle
   without any hinderance. For XENVER_commandline and XENVER_buildid
   they are evaluated.

 b) Acting on the XSM check. For most of them we cannot actually return
   -EFAULT and MUST return either an valid value or some form of a string.
   
   The ones for which we could return '<denied>' were changeset, compile_info,
   commandline, extraversion. To make it simpler we only do it for
   commandline.

In essence we have an XSM check which is ignored by all XENVER_ subops
except commandline (and build_id in later patch).

I think both of you are OK with that?

> 
> >> > @@ -354,10 +356,17 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >> >          return 0;
> >> >  
> >> >      case XENVER_commandline:
> >> > -        if ( copy_to_guest(arg, saved_cmdline, ARRAY_SIZE(saved_cmdline)) )
> >> > +    {
> >> > +        size_t len = ARRAY_SIZE(saved_cmdline);
> >> > +
> >> > +        if ( deny )
> >> > +            len = strlen(xen_deny());
> >> 
> >> +1 (or else you fail to nul-terminate the output).
> > 
> > Nice spotting!
> > Perhaps modifying xen_deny() to be:
> > 
> > const char *xen_deny(void)
> > {
> >     return "<denied>\0";
> > }
> > 
> > Would be better?
> 
> This would just add a second NUL at the end, without altering what

Right. Sorry about that - the patch I had sent still includes the \0.

> strlen() returns on that string
> 
> Jan
> 

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Jan Beulich]

>>> On 08.01.16 at 18:31, <konrad.wilk@oracle.com> wrote:
>> >> > The rest: XENVER_[version|capabilities|
>> >> > parameters|get_features|page_size|guest_handle] behave
>> >> > as before - allowed by default for all guests.
>> >> > 
>> >> > This is with the XSM default policy and with the dummy ones.
>> >> 
>> >> And with a non-default policy you now ignore one of the latter
>> >> ops to also get denied.
>> > 
>> > No, but that is due to the 'deny' being only checked for certain subops.
>> 
>> To me this reply seems contradictory in itself: The "no" doesn't
>> seem to match up with the rest.
>> 
>> > I think what you are saying is that for XENVER_[version|capabilities|
>> > parameters|get_features|page_size|guest_handle] we should not have any
>> > XSM checks as they serve no purpose (which is what I had in the earlier
>> > versions of this patch). However Andrew mentioned that he would
>> > like _ALL_ of the sub-ops to be checked for.
>> 
>> And I agree with Andrew, hence my earlier comment above (with
>> the reply I can't really make sense of).
> 
> I am all confused now.
> 
> There are two parts here:
>  a) The XSM checks - which allow the XENVER_version..XENVER_guest_handle
>    without any hinderance. For XENVER_commandline and XENVER_buildid
>    they are evaluated.
> 
>  b) Acting on the XSM check. For most of them we cannot actually return
>    -EFAULT and MUST return either an valid value or some form of a string.
>    
>    The ones for which we could return '<denied>' were changeset, compile_info,
>    commandline, extraversion. To make it simpler we only do it for
>    commandline.
> 
> In essence we have an XSM check which is ignored by all XENVER_ subops
> except commandline (and build_id in later patch).
> 
> I think both of you are OK with that?

Iirc Andrew's request was to honor XSM denies on any sub-op
when a non-default policy is in place. Whereas in default mode
only command line and build id are the ones clearly needing
restricting. Which won't be possible if you ignore the return
value of the XSM check in some of the cases.

Jan

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Konrad Rzeszutek Wilk]

On Mon, Jan 11, 2016 at 02:02:54AM -0700, Jan Beulich wrote:
> >>> On 08.01.16 at 18:31, <konrad.wilk@oracle.com> wrote:
> >> >> > The rest: XENVER_[version|capabilities|
> >> >> > parameters|get_features|page_size|guest_handle] behave
> >> >> > as before - allowed by default for all guests.
> >> >> > 
> >> >> > This is with the XSM default policy and with the dummy ones.
> >> >> 
> >> >> And with a non-default policy you now ignore one of the latter
> >> >> ops to also get denied.
> >> > 
> >> > No, but that is due to the 'deny' being only checked for certain subops.
> >> 
> >> To me this reply seems contradictory in itself: The "no" doesn't
> >> seem to match up with the rest.
> >> 
> >> > I think what you are saying is that for XENVER_[version|capabilities|
> >> > parameters|get_features|page_size|guest_handle] we should not have any
> >> > XSM checks as they serve no purpose (which is what I had in the earlier
> >> > versions of this patch). However Andrew mentioned that he would
> >> > like _ALL_ of the sub-ops to be checked for.
> >> 
> >> And I agree with Andrew, hence my earlier comment above (with
> >> the reply I can't really make sense of).
> > 
> > I am all confused now.
> > 
> > There are two parts here:
> >  a) The XSM checks - which allow the XENVER_version..XENVER_guest_handle
> >    without any hinderance. For XENVER_commandline and XENVER_buildid
> >    they are evaluated.
> > 
> >  b) Acting on the XSM check. For most of them we cannot actually return
> >    -EFAULT and MUST return either an valid value or some form of a string.
> >    
> >    The ones for which we could return '<denied>' were changeset, compile_info,
> >    commandline, extraversion. To make it simpler we only do it for
> >    commandline.
> > 
> > In essence we have an XSM check which is ignored by all XENVER_ subops
> > except commandline (and build_id in later patch).
> > 
> > I think both of you are OK with that?
> 
> Iirc Andrew's request was to honor XSM denies on any sub-op
> when a non-default policy is in place. Whereas in default mode
> only command line and build id are the ones clearly needing
> restricting. Which won't be possible if you ignore the return
> value of the XSM check in some of the cases.

That means we need two (as earlier patches had it) version labels.
One for the command_line and build_id (version_priv) and one for
the rest (version_use). By default version_use would be available
to every guest. If a non-default policy wants to mess with it - that is OK.

Now comes the big question - for the XENVER_[version|capabilities|
parameters|get_features|page_size|guest_handle] - if it is denied
(so non-default version_use policy) - what should we return?

I can return '<denied>' for the strings, but what should we do
for the page_size, capabilities and guest_handle ? -EPERM?

Or leave those out of the version_use check? (so do not act on
XSM check on those?)
> 
> Jan
> 

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Jan Beulich]

>>> On 11.01.16 at 17:01, <konrad.wilk@oracle.com> wrote:
> On Mon, Jan 11, 2016 at 02:02:54AM -0700, Jan Beulich wrote:
>> >>> On 08.01.16 at 18:31, <konrad.wilk@oracle.com> wrote:
>> >> >> > The rest: XENVER_[version|capabilities|
>> >> >> > parameters|get_features|page_size|guest_handle] behave
>> >> >> > as before - allowed by default for all guests.
>> >> >> > 
>> >> >> > This is with the XSM default policy and with the dummy ones.
>> >> >> 
>> >> >> And with a non-default policy you now ignore one of the latter
>> >> >> ops to also get denied.
>> >> > 
>> >> > No, but that is due to the 'deny' being only checked for certain subops.
>> >> 
>> >> To me this reply seems contradictory in itself: The "no" doesn't
>> >> seem to match up with the rest.
>> >> 
>> >> > I think what you are saying is that for XENVER_[version|capabilities|
>> >> > parameters|get_features|page_size|guest_handle] we should not have any
>> >> > XSM checks as they serve no purpose (which is what I had in the earlier
>> >> > versions of this patch). However Andrew mentioned that he would
>> >> > like _ALL_ of the sub-ops to be checked for.
>> >> 
>> >> And I agree with Andrew, hence my earlier comment above (with
>> >> the reply I can't really make sense of).
>> > 
>> > I am all confused now.
>> > 
>> > There are two parts here:
>> >  a) The XSM checks - which allow the XENVER_version..XENVER_guest_handle
>> >    without any hinderance. For XENVER_commandline and XENVER_buildid
>> >    they are evaluated.
>> > 
>> >  b) Acting on the XSM check. For most of them we cannot actually return
>> >    -EFAULT and MUST return either an valid value or some form of a string.
>> >    
>> >    The ones for which we could return '<denied>' were changeset, compile_info,
>> >    commandline, extraversion. To make it simpler we only do it for
>> >    commandline.
>> > 
>> > In essence we have an XSM check which is ignored by all XENVER_ subops
>> > except commandline (and build_id in later patch).
>> > 
>> > I think both of you are OK with that?
>> 
>> Iirc Andrew's request was to honor XSM denies on any sub-op
>> when a non-default policy is in place. Whereas in default mode
>> only command line and build id are the ones clearly needing
>> restricting. Which won't be possible if you ignore the return
>> value of the XSM check in some of the cases.
> 
> That means we need two (as earlier patches had it) version labels.
> One for the command_line and build_id (version_priv) and one for
> the rest (version_use). By default version_use would be available
> to every guest. If a non-default policy wants to mess with it - that is OK.

That would seem a little too coarse grained. Why can't we keep it
at the sub-op level, just that the default is "OK" for everything
except the two?

> Now comes the big question - for the XENVER_[version|capabilities|
> parameters|get_features|page_size|guest_handle] - if it is denied
> (so non-default version_use policy) - what should we return?

"<denied>" just like for the others that return strings; page_size
and other numeric ones may need to return zero.

> I can return '<denied>' for the strings, but what should we do
> for the page_size, capabilities and guest_handle ? -EPERM?

guest_handle is particularly interesting: It seems to make very
little sense to deny a guest access to its own handle.

Jan

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Konrad Rzeszutek Wilk]

On Mon, Jan 11, 2016 at 09:17:57AM -0700, Jan Beulich wrote:
> >>> On 11.01.16 at 17:01, <konrad.wilk@oracle.com> wrote:
> > On Mon, Jan 11, 2016 at 02:02:54AM -0700, Jan Beulich wrote:
> >> >>> On 08.01.16 at 18:31, <konrad.wilk@oracle.com> wrote:
> >> >> >> > The rest: XENVER_[version|capabilities|
> >> >> >> > parameters|get_features|page_size|guest_handle] behave
> >> >> >> > as before - allowed by default for all guests.
> >> >> >> > 
> >> >> >> > This is with the XSM default policy and with the dummy ones.
> >> >> >> 
> >> >> >> And with a non-default policy you now ignore one of the latter
> >> >> >> ops to also get denied.
> >> >> > 
> >> >> > No, but that is due to the 'deny' being only checked for certain subops.
> >> >> 
> >> >> To me this reply seems contradictory in itself: The "no" doesn't
> >> >> seem to match up with the rest.
> >> >> 
> >> >> > I think what you are saying is that for XENVER_[version|capabilities|
> >> >> > parameters|get_features|page_size|guest_handle] we should not have any
> >> >> > XSM checks as they serve no purpose (which is what I had in the earlier
> >> >> > versions of this patch). However Andrew mentioned that he would
> >> >> > like _ALL_ of the sub-ops to be checked for.
> >> >> 
> >> >> And I agree with Andrew, hence my earlier comment above (with
> >> >> the reply I can't really make sense of).
> >> > 
> >> > I am all confused now.
> >> > 
> >> > There are two parts here:
> >> >  a) The XSM checks - which allow the XENVER_version..XENVER_guest_handle
> >> >    without any hinderance. For XENVER_commandline and XENVER_buildid
> >> >    they are evaluated.
> >> > 
> >> >  b) Acting on the XSM check. For most of them we cannot actually return
> >> >    -EFAULT and MUST return either an valid value or some form of a string.
> >> >    
> >> >    The ones for which we could return '<denied>' were changeset, compile_info,
> >> >    commandline, extraversion. To make it simpler we only do it for
> >> >    commandline.
> >> > 
> >> > In essence we have an XSM check which is ignored by all XENVER_ subops
> >> > except commandline (and build_id in later patch).
> >> > 
> >> > I think both of you are OK with that?
> >> 
> >> Iirc Andrew's request was to honor XSM denies on any sub-op
> >> when a non-default policy is in place. Whereas in default mode
> >> only command line and build id are the ones clearly needing
> >> restricting. Which won't be possible if you ignore the return
> >> value of the XSM check in some of the cases.
> > 
> > That means we need two (as earlier patches had it) version labels.
> > One for the command_line and build_id (version_priv) and one for
> > the rest (version_use). By default version_use would be available
> > to every guest. If a non-default policy wants to mess with it - that is OK.
> 
> That would seem a little too coarse grained. Why can't we keep it
> at the sub-op level, just that the default is "OK" for everything
> except the two?

So you thinking have a whole new XSM 'class' for this hypercall?

As in (not compile tested of course):

diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
index 17f304e..fca5809 100644
--- a/tools/flask/policy/policy/modules/xen/xen.te
+++ b/tools/flask/policy/policy/modules/xen/xen.te
@@ -141,6 +141,13 @@ if (guest_writeconsole) {
 # pmu_ctrl is for)
 allow domain_type xen_t:xen2 pmu_use;
 
+allow dom0_t version:domain {
+    version parameters ... commandline build_id
+};
+allow domain_type version:domain {
+    version parameters ...
+}
+
 ###############################################################################
 #
 # Domain creation
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index c123256..4b20d08 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -246,6 +246,18 @@ class domain2
     psr_cat_op
 }
 
+# defined by XENVER_hypercall
+class xenver
+{
+# XENVER_version
+    version
+# XENVER_parameters
+    parameters
+# ... snip..
+    commandline
+    build_id
+}
+
 # Similar to class domain, but primarily contains domctls related to HVM domains
 class hvm
 {

> 
> > Now comes the big question - for the XENVER_[version|capabilities|
> > parameters|get_features|page_size|guest_handle] - if it is denied
> > (so non-default version_use policy) - what should we return?
> 
> "<denied>" just like for the others that return strings; page_size
> and other numeric ones may need to return zero.
> 
> > I can return '<denied>' for the strings, but what should we do
> > for the page_size, capabilities and guest_handle ? -EPERM?
> 
> guest_handle is particularly interesting: It seems to make very
> little sense to deny a guest access to its own handle.
> 
> Jan
> 

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Jan Beulich]

>>> On 12.01.16 at 17:37, <konrad.wilk@oracle.com> wrote:
> On Mon, Jan 11, 2016 at 09:17:57AM -0700, Jan Beulich wrote:
>> >>> On 11.01.16 at 17:01, <konrad.wilk@oracle.com> wrote:
>> > On Mon, Jan 11, 2016 at 02:02:54AM -0700, Jan Beulich wrote:
>> >> >>> On 08.01.16 at 18:31, <konrad.wilk@oracle.com> wrote:
>> >> >> >> > The rest: XENVER_[version|capabilities|
>> >> >> >> > parameters|get_features|page_size|guest_handle] behave
>> >> >> >> > as before - allowed by default for all guests.
>> >> >> >> > 
>> >> >> >> > This is with the XSM default policy and with the dummy ones.
>> >> >> >> 
>> >> >> >> And with a non-default policy you now ignore one of the latter
>> >> >> >> ops to also get denied.
>> >> >> > 
>> >> >> > No, but that is due to the 'deny' being only checked for certain subops.
>> >> >> 
>> >> >> To me this reply seems contradictory in itself: The "no" doesn't
>> >> >> seem to match up with the rest.
>> >> >> 
>> >> >> > I think what you are saying is that for XENVER_[version|capabilities|
>> >> >> > parameters|get_features|page_size|guest_handle] we should not have any
>> >> >> > XSM checks as they serve no purpose (which is what I had in the earlier
>> >> >> > versions of this patch). However Andrew mentioned that he would
>> >> >> > like _ALL_ of the sub-ops to be checked for.
>> >> >> 
>> >> >> And I agree with Andrew, hence my earlier comment above (with
>> >> >> the reply I can't really make sense of).
>> >> > 
>> >> > I am all confused now.
>> >> > 
>> >> > There are two parts here:
>> >> >  a) The XSM checks - which allow the XENVER_version..XENVER_guest_handle
>> >> >    without any hinderance. For XENVER_commandline and XENVER_buildid
>> >> >    they are evaluated.
>> >> > 
>> >> >  b) Acting on the XSM check. For most of them we cannot actually return
>> >> >    -EFAULT and MUST return either an valid value or some form of a string.
>> >> >    
>> >> >    The ones for which we could return '<denied>' were changeset, 
> compile_info,
>> >> >    commandline, extraversion. To make it simpler we only do it for
>> >> >    commandline.
>> >> > 
>> >> > In essence we have an XSM check which is ignored by all XENVER_ subops
>> >> > except commandline (and build_id in later patch).
>> >> > 
>> >> > I think both of you are OK with that?
>> >> 
>> >> Iirc Andrew's request was to honor XSM denies on any sub-op
>> >> when a non-default policy is in place. Whereas in default mode
>> >> only command line and build id are the ones clearly needing
>> >> restricting. Which won't be possible if you ignore the return
>> >> value of the XSM check in some of the cases.
>> > 
>> > That means we need two (as earlier patches had it) version labels.
>> > One for the command_line and build_id (version_priv) and one for
>> > the rest (version_use). By default version_use would be available
>> > to every guest. If a non-default policy wants to mess with it - that is OK.
>> 
>> That would seem a little too coarse grained. Why can't we keep it
>> at the sub-op level, just that the default is "OK" for everything
>> except the two?
> 
> So you thinking have a whole new XSM 'class' for this hypercall?

Yes, subject to Daniel's input it seems to me that's the only way
providing the necessary granularity.

Jan

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Daniel De Graaf]

On 06/11/15 14:36, Konrad Rzeszutek Wilk wrote:
> All of XENVER_* have now an XSM check.
>
> The subops for XENVER_[compile_info|changeset|commandline|
> extraversion] are now priviliged operations. To not break
> guests we still return an string - but it is just '<denied>'.
>
> The rest: XENVER_[version|capabilities|
> parameters|get_features|page_size|guest_handle] behave
> as before - allowed by default for all guests.
>
> This is with the XSM default policy and with the dummy ones.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Comments below, inline.

[...]
> diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
> index d35ae22..1ca0e65 100644
> --- a/tools/flask/policy/policy/modules/xen/xen.te
> +++ b/tools/flask/policy/policy/modules/xen/xen.te
> @@ -73,6 +73,12 @@ allow dom0_t xen_t:xen2 {
>       pmu_ctrl
>       get_symbol
>   };
> +
> +# Allow dom0 to use XENVER_compile_info|changeset|commandline]extraversion
> +allow dom0_t xen_t:xen2 {
> +    version_priv
> +};
> +
>   allow dom0_t xen_t:mmu memorymap;
>
>   # Allow dom0 to use these domctls on itself. For domctls acting on other

Minor tweak: if you don't want to add the new to the block a few lines above,
the one-line permission syntax without braces (as seen below) looks better.

[...]
>   DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>   {
> +    bool_t deny = !!xsm_version_op(XSM_HOOK, cmd);
> +

Since this call produces denials in the default policy, it should be marked
as XSM_OTHER.

> diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
> index effb59f..273459f 100644
> --- a/xen/xsm/flask/policy/access_vectors
> +++ b/xen/xsm/flask/policy/access_vectors
> @@ -93,6 +93,8 @@ class xen2
>       pmu_ctrl
>   # PMU use (domains, including unprivileged ones, will be using this operation)
>       pmu_use
> +# XENVER_[compile_info|changeset|commandline|extraversion] usage.
> +   version_priv
>   }
>
>   # Classes domain and domain2 consist of operations that a domain performs on
> @@ -242,6 +244,8 @@ class domain2
>       mem_sharing
>   # XEN_DOMCTL_psr_cat_op
>       psr_cat_op
> +# XENVER_[version|capabilities|parameters|get_features|page_size|guest_handle].
> +    version_use
>   }
>
>   # Similar to class domain, but primarily contains domctls related to HVM domains
>

I think that both version_priv and version_use belong in the same access
vector (xen2) rather than placing version_use in domain2.

-- 
Daniel De Graaf
National Security Agency

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Konrad Rzeszutek Wilk]

On Tue, Nov 10, 2015 at 02:51:35PM -0500, Daniel De Graaf wrote:
> On 06/11/15 14:36, Konrad Rzeszutek Wilk wrote:
> >All of XENVER_* have now an XSM check.
> >
> >The subops for XENVER_[compile_info|changeset|commandline|
> >extraversion] are now priviliged operations. To not break
> >guests we still return an string - but it is just '<denied>'.
> >
> >The rest: XENVER_[version|capabilities|
> >parameters|get_features|page_size|guest_handle] behave
> >as before - allowed by default for all guests.
> >
> >This is with the XSM default policy and with the dummy ones.
> >
> >Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Comments below, inline.
> 
> [...]
> >diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
> >index d35ae22..1ca0e65 100644
> >--- a/tools/flask/policy/policy/modules/xen/xen.te
> >+++ b/tools/flask/policy/policy/modules/xen/xen.te
> >@@ -73,6 +73,12 @@ allow dom0_t xen_t:xen2 {
> >      pmu_ctrl
> >      get_symbol
> >  };
> >+
> >+# Allow dom0 to use XENVER_compile_info|changeset|commandline]extraversion
> >+allow dom0_t xen_t:xen2 {
> >+    version_priv
> >+};
> >+
> >  allow dom0_t xen_t:mmu memorymap;
> >
> >  # Allow dom0 to use these domctls on itself. For domctls acting on other
> 
> Minor tweak: if you don't want to add the new to the block a few lines above,
> the one-line permission syntax without braces (as seen below) looks better.

OK.
> 
> [...]
> >  DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >  {
> >+    bool_t deny = !!xsm_version_op(XSM_HOOK, cmd);
> >+
> 
> Since this call produces denials in the default policy, it should be marked
> as XSM_OTHER.

<nods>
> 
> >diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
> >index effb59f..273459f 100644
> >--- a/xen/xsm/flask/policy/access_vectors
> >+++ b/xen/xsm/flask/policy/access_vectors
> >@@ -93,6 +93,8 @@ class xen2
> >      pmu_ctrl
> >  # PMU use (domains, including unprivileged ones, will be using this operation)
> >      pmu_use
> >+# XENVER_[compile_info|changeset|commandline|extraversion] usage.
> >+   version_priv
> >  }
> >
> >  # Classes domain and domain2 consist of operations that a domain performs on
> >@@ -242,6 +244,8 @@ class domain2
> >      mem_sharing
> >  # XEN_DOMCTL_psr_cat_op
> >      psr_cat_op
> >+# XENVER_[version|capabilities|parameters|get_features|page_size|guest_handle].
> >+    version_use
> >  }
> >
> >  # Similar to class domain, but primarily contains domctls related to HVM domains
> >
> 
> I think that both version_priv and version_use belong in the same access
> vector (xen2) rather than placing version_use in domain2.

OK, it just that the 'xen2' says: "Class xen and xen2 consists of dom0-only operations
dealing with the  hypervisor itself." (from access_vectors) - hence the split in using
domain2 and xen2.
> 
> -- 
> Daniel De Graaf
> National Security Agency

Re: [PATCH v2 1/3] xsm/xen_version: Add XSM for the xen_version hypercall.

[Konrad Rzeszutek Wilk]

On Tue, Nov 10, 2015 at 02:51:35PM -0500, Daniel De Graaf wrote:
> On 06/11/15 14:36, Konrad Rzeszutek Wilk wrote:
> >All of XENVER_* have now an XSM check.
> >
> >The subops for XENVER_[compile_info|changeset|commandline|
> >extraversion] are now priviliged operations. To not break
> >guests we still return an string - but it is just '<denied>'.
> >
> >The rest: XENVER_[version|capabilities|
> >parameters|get_features|page_size|guest_handle] behave
> >as before - allowed by default for all guests.
> >
> >This is with the XSM default policy and with the dummy ones.
> >
> >Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Comments below, inline.
> 
> [...]
> >diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
> >index d35ae22..1ca0e65 100644
> >--- a/tools/flask/policy/policy/modules/xen/xen.te
> >+++ b/tools/flask/policy/policy/modules/xen/xen.te
> >@@ -73,6 +73,12 @@ allow dom0_t xen_t:xen2 {
> >      pmu_ctrl
> >      get_symbol
> >  };
> >+
> >+# Allow dom0 to use XENVER_compile_info|changeset|commandline]extraversion
> >+allow dom0_t xen_t:xen2 {
> >+    version_priv
> >+};
> >+
> >  allow dom0_t xen_t:mmu memorymap;
> >
> >  # Allow dom0 to use these domctls on itself. For domctls acting on other
> 
> Minor tweak: if you don't want to add the new to the block a few lines above,
> the one-line permission syntax without braces (as seen below) looks better.

<nods>
> 
> [...]
> >  DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >  {
> >+    bool_t deny = !!xsm_version_op(XSM_HOOK, cmd);
> >+
> 
> Since this call produces denials in the default policy, it should be marked
> as XSM_OTHER.

<nods>
> 
> >diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
> >index effb59f..273459f 100644
> >--- a/xen/xsm/flask/policy/access_vectors
> >+++ b/xen/xsm/flask/policy/access_vectors
> >@@ -93,6 +93,8 @@ class xen2
> >      pmu_ctrl
> >  # PMU use (domains, including unprivileged ones, will be using this operation)
> >      pmu_use
> >+# XENVER_[compile_info|changeset|commandline|extraversion] usage.
> >+   version_priv
> >  }
> >
> >  # Classes domain and domain2 consist of operations that a domain performs on
> >@@ -242,6 +244,8 @@ class domain2
> >      mem_sharing
> >  # XEN_DOMCTL_psr_cat_op
> >      psr_cat_op
> >+# XENVER_[version|capabilities|parameters|get_features|page_size|guest_handle].
> >+    version_use
> >  }
> >
> >  # Similar to class domain, but primarily contains domctls related to HVM domains
> >
> 
> I think that both version_priv and version_use belong in the same access
> vector (xen2) rather than placing version_use in domain2.

Ok, modified.
> 
> -- 
> Daniel De Graaf
> National Security Agency

[PATCH v2 2/3] XENVER_build_id: Provide ld-embedded build-ids

[Konrad Rzeszutek Wilk]

From: Martin Pohlack <mpohlack@amazon.de>

The mechanism to get this is via the XENVER_build_id and
we add a new subsequent sub-command to retrieve the
binary build-id. The hypercall allows an arbitrary
size (the buffer and len is provided to the hypervisor).

Note that one can also retrieve the value by 'readelf -h xen-syms'.

For EFI builds we re-use the same build-id that the xen-syms
was built with.

Note that there are no changes to the XSM files (dummy.c
and hooks.c) as the priviliged subops fall in the default case.

Since the new sub-ops provides the len parameter in the
arguments to the hypercall we have to modify libxc to allow
copying the arguments before the hypercall. We provide a
new macro that modifies the bounce structure to change
the direction.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Martin Pohlack <mpohlack@amazon.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v1: Rebase it on Martin's initial patch
v2: Move it to XENVER hypercall
v3: Fix EFI building (Ross's fix)
v4: Don't use the third argument for length.
v5: Use new structure for XENVER_build_id with variable buf.
---
 tools/libxc/xc_private.c            |  7 +++++++
 tools/libxc/xc_private.h            | 10 ++++++++++
 xen/arch/x86/Makefile               | 14 +++++++++-----
 xen/arch/x86/xen.lds.S              |  6 ++++++
 xen/common/kernel.c                 | 29 +++++++++++++++++++++++++++++
 xen/common/version.c                | 36 ++++++++++++++++++++++++++++++++++++
 xen/include/public/version.h        | 15 ++++++++++++++-
 xen/include/xen/version.h           |  1 +
 xen/xsm/flask/policy/access_vectors |  2 +-
 9 files changed, 113 insertions(+), 7 deletions(-)

diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 7c39897..9cd676e 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -712,6 +712,13 @@ int xc_version(xc_interface *xch, int cmd, void *arg)
     case XENVER_commandline:
         sz = sizeof(xen_commandline_t);
         break;
+    case XENVER_build_id:
+        {
+            xen_build_id_t *build_id = (xen_build_id_t *)arg;
+            sz = sizeof(*build_id) + build_id->len;
+            HYPERCALL_BOUNCE_SET_DIR(arg, XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+            break;
+        }
     default:
         ERROR("xc_version: unknown command %d\n", cmd);
         return -EINVAL;
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 2df1d59..aa0cad7 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -202,6 +202,16 @@ enum {
 #define DECLARE_HYPERCALL_BOUNCE(_ubuf, _sz, _dir) DECLARE_NAMED_HYPERCALL_BOUNCE(_ubuf, _ubuf, _sz, _dir)
 
 /*
+ * Change the direction.
+ *
+ * Can only be used if the bounce_pre/bounce_post commands have
+ * not been used.
+ */
+#define HYPERCALL_BOUNCE_SET_DIR(_buf, _dir) do { if ((HYPERCALL_BUFFER(_buf))->hbuf)         \
+                                                        assert(1);                            \
+                                                   (HYPERCALL_BUFFER(_buf))->dir = _dir;      \
+                                                } while (0)
+/*
  * Set the size of data to bounce. Useful when the size is not known
  * when the bounce buffer is declared.
  */
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index d4e507a..17a4830 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -109,15 +109,19 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
 	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
 		| $(BASEDIR)/tools/symbols --sysv --sort >$(@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) -pa --format=sysv $(@D)/.$(@F).1 \
 		| $(BASEDIR)/tools/symbols --sysv --sort --warn-dup >$(@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]*
 
+build_id.o: $(TARGET)-syms
+	$(OBJCOPY) --only-section=.note.gnu.build-id $< build_id.o
+
+
 EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(LDFLAGS)) --subsystem=10
 EFI_LDFLAGS += --image-base=$(1) --stack=0,0 --heap=0,0 --strip-debug
 EFI_LDFLAGS += --section-alignment=0x200000 --file-alignment=0x20
@@ -130,7 +134,7 @@ $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIR
 $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p')
 # Don't use $(wildcard ...) here - at least make 3.80 expands this too early!
 $(TARGET).efi: guard = $(if $(shell echo efi/dis* | grep disabled),:)
-$(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc
+$(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbols-dummy.o efi/mkreloc build_id.o
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \
 	                $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).$(base).0 &&) :
@@ -145,8 +149,8 @@ $(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbol
 	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
 		| $(guard) $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1s.S
 	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o
-	$(guard) $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T efi.lds -N $< \
-	                $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o -o $@
+	$(guard) $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T efi.lds --build-id=sha1 -N $< \
+	                $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o build_id.o -o $@
 	if $(guard) false; then rm -f $@; echo 'EFI support disabled'; fi
 	rm -f $(@D)/.$(@F).[0-9]*
 
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 6553cff..c4aabc3 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -67,6 +67,12 @@ SECTIONS
        *(.rodata.*)
   } :text
 
+  .note.gnu.build-id : {
+      __note_gnu_build_id_start = .;
+      *(.note.gnu.build-id)
+      __note_gnu_build_id_end = .;
+  } :text
+
   . = ALIGN(SMP_CACHE_BYTES);
   .data.read_mostly : {
        /* Exception table */
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index d51a3ed..1574c63 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -366,6 +366,35 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             return -EFAULT;
         return 0;
     }
+    case XENVER_build_id:
+    {
+        xen_build_id_t build_id;
+        unsigned int sz;
+        int rc = 0;
+        char *p = NULL;
+
+        if ( deny )
+            return -EPERM;
+
+        if ( copy_from_guest(&build_id, arg, 1) )
+            return -EFAULT;
+
+        if ( build_id.len == 0 )
+            return -EINVAL;
+
+        rc = xen_build_id(&p, &sz);
+        if ( rc )
+            return rc;
+
+        if ( sz > build_id.len )
+            return -ENOMEM;
+
+        if ( copy_to_guest_offset(arg, offsetof(xen_build_id_t, buf),
+                                  deny ? xen_deny() : p, sz) )
+            return -EFAULT;
+
+        return sz;
+    }
     }
 
     return -ENOSYS;
diff --git a/xen/common/version.c b/xen/common/version.c
index 786be4e..d556c35 100644
--- a/xen/common/version.c
+++ b/xen/common/version.c
@@ -1,5 +1,9 @@
 #include <xen/compile.h>
 #include <xen/version.h>
+#include <xen/types.h>
+#include <xen/string.h>
+#include <xen/elf.h>
+#include <xen/errno.h>
 
 const char *xen_compile_date(void)
 {
@@ -60,3 +64,35 @@ const char *xen_deny(void)
 {
     return "<denied>";
 }
+#ifdef CONFIG_ARM
+int xen_build_id(char **p, unsigned int *len)
+{
+    return -ENODATA;
+}
+#else
+#define NT_GNU_BUILD_ID 3
+
+extern const Elf_Note __note_gnu_build_id_start;  /* Defined in linker script. */
+extern const char __note_gnu_build_id_end[];
+int xen_build_id(char **p, unsigned int *len)
+{
+    const Elf_Note *n = &__note_gnu_build_id_start;
+
+    /* Something is wrong. */
+    if ( __note_gnu_build_id_end <= (char *)&__note_gnu_build_id_start )
+        return -ENODATA;
+
+    /* Check if we really have a build-id. */
+    if ( NT_GNU_BUILD_ID != n->type )
+        return -ENODATA;
+
+    /* Sanity check, name should be "GNU" for ld-generated build-id. */
+    if ( strncmp(ELFNOTE_NAME(n), "GNU", n->namesz) != 0 )
+        return -ENODATA;
+
+    *len = n->descsz;
+    *p = ELFNOTE_DESC(n);
+
+    return 0;
+}
+#endif
diff --git a/xen/include/public/version.h b/xen/include/public/version.h
index 44f26b0..adae298 100644
--- a/xen/include/public/version.h
+++ b/xen/include/public/version.h
@@ -30,7 +30,8 @@
 
 #include "xen.h"
 
-/* NB. All ops return zero on success, except XENVER_{version,pagesize} */
+/* NB. All ops return zero on success, except
+ * XENVER_{version,pagesize, build_id} */
 
 /* arg == NULL; returns major:minor (16:16). */
 #define XENVER_version      0
@@ -83,6 +84,18 @@ typedef struct xen_feature_info xen_feature_info_t;
 #define XENVER_commandline 9
 typedef char xen_commandline_t[1024];
 
+/* arg = xen_build_id_t. Returns size in bytes or XEN_Exx. */
+#define XENVER_build_id 10
+struct xen_build_id {
+        uint32_t        len; /* IN: size of buf[]. */
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L
+        unsigned char   buf[];
+#elif defined(__GNUC__)
+        unsigned char   buf[1]; /* OUT: Variable length buffer with build_id. */
+#endif
+};
+typedef struct xen_build_id xen_build_id_t;
+
 #endif /* __XEN_PUBLIC_VERSION_H__ */
 
 /*
diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h
index 2015c0b..466c977 100644
--- a/xen/include/xen/version.h
+++ b/xen/include/xen/version.h
@@ -13,5 +13,6 @@ const char *xen_extra_version(void);
 const char *xen_changeset(void);
 const char *xen_banner(void);
 const char *xen_deny(void);
+int xen_build_id(char **p, unsigned int *len);
 
 #endif /* __XEN_VERSION_H__ */
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 273459f..35bdb42 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -93,7 +93,7 @@ class xen2
     pmu_ctrl
 # PMU use (domains, including unprivileged ones, will be using this operation)
     pmu_use
-# XENVER_[compile_info|changeset|commandline|extraversion] usage.
+# XENVER_[compile_info|changeset|commandline|extraversion|buildid] usage.
    version_priv
 }
 
-- 
2.1.0

Re: [PATCH v2 2/3] XENVER_build_id: Provide ld-embedded build-ids

[Ross Lagerwall]

On 11/06/2015 07:36 PM, Konrad Rzeszutek Wilk wrote:
> From: Martin Pohlack <mpohlack@amazon.de>
>
> The mechanism to get this is via the XENVER_build_id and
> we add a new subsequent sub-command to retrieve the
> binary build-id. The hypercall allows an arbitrary
> size (the buffer and len is provided to the hypervisor).
>
> Note that one can also retrieve the value by 'readelf -h xen-syms'.
>
> For EFI builds we re-use the same build-id that the xen-syms
> was built with.
>
> Note that there are no changes to the XSM files (dummy.c
> and hooks.c) as the priviliged subops fall in the default case.
>
> Since the new sub-ops provides the len parameter in the
> arguments to the hypercall we have to modify libxc to allow
> copying the arguments before the hypercall. We provide a
> new macro that modifies the bounce structure to change
> the direction.
>
snip
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index d4e507a..17a4830 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -109,15 +109,19 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o

The above invocation of ld also needs to pass "--build-id=sha1".
Otherwise, the first invocation of tools/symbols has 
__note_gnu_build_id_end == __note_gnu_build_id_start, which may cause 
the size of the symbol table to change later when there is actually a 
build id (presumably due to the way it is compressed?) ultimately 
resulting in incorrect values in the symbol table.

>   	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
>   		| $(BASEDIR)/tools/symbols --sysv --sort >$(@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) -pa --format=sysv $(@D)/.$(@F).1 \
>   		| $(BASEDIR)/tools/symbols --sysv --sort --warn-dup >$(@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]*
>

-- 
Ross Lagerwall

Re: [PATCH v2 2/3] XENVER_build_id: Provide ld-embedded build-ids

[Jan Beulich]

>>> On 06.11.15 at 20:36, <konrad.wilk@oracle.com> wrote:
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -109,15 +109,19 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
>  	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
>  		| $(BASEDIR)/tools/symbols --sysv --sort >$(@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 \

You continue to use option this unconditionally.

Jan

Re: [PATCH v2 2/3] XENVER_build_id: Provide ld-embedded build-ids

[Konrad Rzeszutek Wilk]

On Tue, Nov 10, 2015 at 09:49:00AM -0700, Jan Beulich wrote:
> >>> On 06.11.15 at 20:36, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -109,15 +109,19 @@ $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
> >  	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
> >  		| $(BASEDIR)/tools/symbols --sysv --sort >$(@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 \
> 
> You continue to use option this unconditionally.

I presume you are suggesting that I should check that the $(LD) can use this right?

I've been building this using an ancient tool-chain:
GNU ld version 2.20.51.0.2-20.fc13 20091009

And even there it is OK.

Or are you thinking of a different LD altogether and checking for that?

> 
> Jan
> 

Re: [PATCH v2 2/3] XENVER_build_id: Provide ld-embedded build-ids

[Jan Beulich]

>>> On 06.01.16 at 18:27, <konrad.wilk@oracle.com> wrote:
> On Tue, Nov 10, 2015 at 09:49:00AM -0700, Jan Beulich wrote:
>> >>> On 06.11.15 at 20:36, <konrad.wilk@oracle.com> wrote:
>> > --- a/xen/arch/x86/Makefile
>> > +++ b/xen/arch/x86/Makefile
>> > @@ -109,15 +109,19 @@ $(TARGET)-syms: prelink.o xen.lds 
> $(BASEDIR)/common/symbols-dummy.o
>> >  	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
>> >  		| $(BASEDIR)/tools/symbols --sysv --sort >$(@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 \
>> 
>> You continue to use option this unconditionally.
> 
> I presume you are suggesting that I should check that the $(LD) can use this 
> right?
> 
> I've been building this using an ancient tool-chain:
> GNU ld version 2.20.51.0.2-20.fc13 20091009
> 
> And even there it is OK.

With us supporting gcc back to 4.1, binutils should be supported
back to what was available at about that time, i.e. at least 2.17,
whereas --build-id only got introduced in 2.18.

Jan

[PATCH v2 3/3] libxl: info: Display build_id of the hypervisor.

[Konrad Rzeszutek Wilk]

If the hypervisor is built with we will display it.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: Include HAVE_*, use libxl_zalloc, s/rc/ret/
---
 tools/libxl/libxl.c         | 24 ++++++++++++++++++++++++
 tools/libxl/libxl.h         |  5 +++++
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    |  1 +
 4 files changed, 31 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 854e957..f30f8c4 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5251,6 +5251,7 @@ libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr)
 
 const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
 {
+    GC_INIT(ctx);
     union {
         xen_extraversion_t xen_extra;
         xen_compile_info_t xen_cc;
@@ -5258,8 +5259,10 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
         xen_capabilities_info_t xen_caps;
         xen_platform_parameters_t p_parms;
         xen_commandline_t xen_commandline;
+        xen_build_id_t build_id;
     } u;
     long xen_version;
+    int ret;
     libxl_version_info *info = &ctx->version_info;
 
     if (info->xen_version_extra != NULL)
@@ -5292,6 +5295,27 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
     xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
     info->commandline = strdup(u.xen_commandline);
 
+    u.build_id.len = sizeof(u) - sizeof(u.build_id);
+    ret = xc_version(ctx->xch, XENVER_build_id, &u.build_id);
+    switch ( ret ) {
+    case -EPERM:
+    case -ENODATA:
+    case 0:
+        info->build_id = strdup("");
+        break;
+    default:
+        if (ret > 0) {
+            unsigned int i;
+
+            info->build_id = libxl__zalloc(NOGC, (ret * 2) + 1);
+
+            for (i = 0; i < ret ; i++)
+                snprintf(&info->build_id[i * 2], 3, "%02hhx", u.build_id.buf[i]);
+        } else
+            LOGEV(ERROR, ret, "getting build_id");
+        break;
+    }
+    GC_FREE;
     return info;
 }
 
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 168fedd..3e2ebd1 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -212,6 +212,11 @@
 #define LIBXL_HAVE_SOFT_RESET 1
 
 /*
+ * LIBXL_HAVE_BUILD_ID means that libxl_version_info has the extra
+ * field for the hypervisor build_id.
+ */
+#define LIBXL_HAVE_BUILD_ID 1
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 4d78f86..5b57824 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -353,6 +353,7 @@ libxl_version_info = Struct("version_info", [
     ("virt_start",        uint64),
     ("pagesize",          integer),
     ("commandline",       string),
+    ("build_id",          string),
     ], dir=DIR_OUT)
 
 libxl_domain_create_info = Struct("domain_create_info",[
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 03442e1..287a356 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5544,6 +5544,7 @@ static void output_xeninfo(void)
     printf("cc_compile_by          : %s\n", info->compile_by);
     printf("cc_compile_domain      : %s\n", info->compile_domain);
     printf("cc_compile_date        : %s\n", info->compile_date);
+    printf("build_id               : %s\n", info->build_id);
 
     return;
 }
-- 
2.1.0