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

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

[Konrad Rzeszutek Wilk]

Hey,

Attached are the four 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] and make them
dom0 accessible. This is if XSM is built in or not (though with
XSM one can expose it to a guest if desired).

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

Note:
 * Hadn't tried compiling it on ARM cross compiler lately. In
   the past I had to #ifdef CONFIG_ARM as the ARM code did not
   use any ELF code so none of the ELF parts made any sense.
 * The EFI build works kindof. It is missing an build_id.o stanza.


 tools/flask/policy/policy/modules/xen/xen.te |  3 +-
 tools/libxc/include/xenctrl.h                |  2 ++
 tools/libxc/xc_private.c                     | 37 ++++++++++++-----------
 tools/libxc/xc_private.h                     |  4 ++-
 tools/libxl/libxl.c                          | 20 ++++++++++++
 tools/libxl/libxl_types.idl                  |  1 +
 tools/libxl/xl_cmdimpl.c                     |  1 +
 xen/arch/arm/traps.c                         |  2 +-
 xen/arch/x86/Makefile                        | 11 +++----
 xen/arch/x86/xen.lds.S                       |  6 ++++
 xen/common/kernel.c                          | 44 ++++++++++++++++++++++++++--
 xen/common/version.c                         | 37 +++++++++++++++++++++++
 xen/include/public/version.h                 |  9 +++++-
 xen/include/xen/hypercall.h                  |  6 ++--
 xen/include/xen/version.h                    |  1 +
 xen/include/xsm/dummy.h                      | 25 ++++++++++++++++
 xen/include/xsm/xsm.h                        |  6 ++++
 xen/xsm/dummy.c                              |  1 +
 xen/xsm/flask/hooks.c                        | 28 ++++++++++++++++++
 xen/xsm/flask/policy/access_vectors          |  4 +++
 20 files changed, 217 insertions(+), 30 deletions(-)

Konrad Rzeszutek Wilk (3):
      xsm/libxl/xen_version: Add XSM for some of the xen_version commands.
      xen-version: Add third parameter (len) to the do_version hypercall.
      libxl: info: Display build_id of the hypervisor.

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

[PATCH v1 1/4] xsm/libxl/xen_version: Add XSM for some of the xen_version commands.

[Konrad Rzeszutek Wilk]

The XENVER_[compile_info|changeset|commandline] are now
guarded by an XSM check.

The rest: XENVER_[version|extraversion|capabilities|
parameters|get_features|page_size|guest_handle] behave
as before (no XSM check).

We allow the initial domain to see these while the other
guests are not permitted.

As such we also modify the toolstack such that if we fail
to get any data instead of printing (null) we just print "".

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 tools/flask/policy/policy/modules/xen/xen.te |  2 +-
 tools/libxl/libxl.c                          |  3 +++
 xen/common/kernel.c                          | 11 ++++++++++-
 xen/include/xsm/dummy.h                      | 24 ++++++++++++++++++++++++
 xen/include/xsm/xsm.h                        |  6 ++++++
 xen/xsm/dummy.c                              |  1 +
 xen/xsm/flask/hooks.c                        | 25 +++++++++++++++++++++++++
 xen/xsm/flask/policy/access_vectors          |  2 ++
 8 files changed, 72 insertions(+), 2 deletions(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
index d35ae22..d0ad758 100644
--- a/tools/flask/policy/policy/modules/xen/xen.te
+++ b/tools/flask/policy/policy/modules/xen/xen.te
@@ -86,7 +86,7 @@ allow dom0_t dom0_t:domain {
 };
 allow dom0_t dom0_t:domain2 {
 	set_cpuid gettsc settsc setscheduler set_max_evtchn set_vnumainfo
-	get_vnumainfo psr_cmt_op psr_cat_op
+	get_vnumainfo psr_cmt_op psr_cat_op version_use
 };
 allow dom0_t dom0_t:resource { add remove };
 
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 22bbc29..efa6462 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5272,6 +5272,7 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
     xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
     info->xen_version_extra = strdup(u.xen_extra);
 
+    memset(&u.xen_cc, 0, sizeof(xen_compile_info_t));
     xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
     info->compiler = strdup(u.xen_cc.compiler);
     info->compile_by = strdup(u.xen_cc.compile_by);
@@ -5281,6 +5282,7 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
     xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
     info->capabilities = strdup(u.xen_caps);
 
+    memset(&u.xen_cc, 0, sizeof(xen_changeset_info_t));
     xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
     info->changeset = strdup(u.xen_chgset);
 
@@ -5289,6 +5291,7 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
 
     info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
 
+    memset(&u.xen_commandline, 0, sizeof(xen_commandline_t));
     xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
     info->commandline = strdup(u.xen_commandline);
 
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 6a3196a..210ec99 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,17 @@ void __init do_initcalls(void)
 /*
  * Simple hypercalls.
  */
-
+#define XENVER_CMD_XSM_CHECK    ( (1U << XENVER_compile_info) | \
+                                  (1U << XENVER_changeset) | \
+                                  (1U << XENVER_commandline) )
 DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
+    if ( ( 1 << cmd ) & XENVER_CMD_XSM_CHECK )
+    {
+        int rc = xsm_version_op(XSM_PRIV, cmd);
+        if ( rc )
+            return rc;
+    }
     switch ( cmd )
     {
     case XENVER_version:
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 9fe372c..2ee102e 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -720,4 +720,28 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG struct domain *d, unsigned int
     }
 }
 
+#include <public/version.h>
+static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op)
+{
+    XSM_ASSERT_ACTION(XSM_PRIV);
+    switch ( op )
+    {
+    case XENVER_compile_info:
+    case XENVER_changeset:
+    case XENVER_commandline:
+        return xsm_default_action(XSM_PRIV, current->domain, NULL);
+    case XENVER_version:
+    case XENVER_extraversion:
+    case XENVER_capabilities:
+    case XENVER_platform_parameters:
+    case XENVER_get_features:
+    case XENVER_pagesize:
+    case XENVER_guest_handle:
+        /* The should _NEVER_ get here, but just in case. */
+        return xsm_default_action(XSM_HOOK, current->domain, NULL);
+    default:
+        return -EPERM;
+    }
+}
+
 #endif /* CONFIG_X86 */
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index ba3caed..cd3e9b8 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
@@ -727,6 +728,11 @@ static inline int xsm_pmu_op (xsm_default_t def, struct domain *d, unsigned int
     return xsm_ops->pmu_op(d, op);
 }
 
+static inline int xsm_version_op (xsm_default_t def, uint32_t op)
+{
+    return xsm_ops->version_op(op);
+}
+
 #endif /* CONFIG_X86 */
 
 #endif /* XSM_NO_WRAPPERS */
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..4f1269d 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,29 @@ static int flask_pmu_op (struct domain *d, unsigned int op)
 }
 #endif /* CONFIG_X86 */
 
+static int flask_version_op (uint32_t op)
+{
+    switch ( op )
+    {
+    case XENVER_compile_info:
+    case XENVER_changeset:
+    case XENVER_commandline:
+        return current_has_perm(current->domain, SECCLASS_DOMAIN2,
+                                DOMAIN2__VERSION_USE);
+    case XENVER_version:
+    case XENVER_extraversion:
+    case XENVER_capabilities:
+    case XENVER_platform_parameters:
+    case XENVER_get_features:
+    case XENVER_pagesize:
+    case XENVER_guest_handle:
+        /* The XSM check is only for a subset of ops. The rest are allowed. */
+        return 0;
+    default:
+        return -EPERM;
+    }
+}
+
 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 +1783,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..6e2e5d1 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -242,6 +242,8 @@ class domain2
     mem_sharing
 # XEN_DOMCTL_psr_cat_op
     psr_cat_op
+# XENVER_[compile_info|changeset|commandline] usage. The rest is not checked.
+   version_use
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
2.1.0

Re: [PATCH v1 1/4] xsm/libxl/xen_version: Add XSM for some of the xen_version commands.

[Ian Campbell]

On Thu, 2015-10-08 at 22:56 -0400, Konrad Rzeszutek Wilk wrote:
> The XENVER_[compile_info|changeset|commandline] are now
> guarded by an XSM check.

I can guess, but please explain/justify why this is the case for these
here.

> The rest: XENVER_[version|extraversion|capabilities|
> parameters|get_features|page_size|guest_handle] behave
> as before (no XSM check).

and correspondingly why these ones to not warrant such a change.

> As such we also modify the toolstack such that if we fail
> to get any data instead of printing (null) we just print "".

Perhaps the hypervisor should instead return "<denied>" or some suitable
string indicating why (<denied-xsm>)?

> @@ -720,4 +720,28 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG
> struct domain *d, unsigned int
>      }
>  }
>  
> +#include <public/version.h>
> +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op)
> +{
> +    XSM_ASSERT_ACTION(XSM_PRIV);
> +    switch ( op )
> +    {
> +    case XENVER_compile_info:
> +    case XENVER_changeset:
> +    case XENVER_commandline:
> +        return xsm_default_action(XSM_PRIV, current->domain, NULL);
> +    case XENVER_version:
> +    case XENVER_extraversion:
> +    case XENVER_capabilities:
> +    case XENVER_platform_parameters:
> +    case XENVER_get_features:
> +    case XENVER_pagesize:
> +    case XENVER_guest_handle:
> +        /* The should _NEVER_ get here, but just in case. */

BUG_ON?

IMHO such a comment should have a "because ..." in it.

Actually, thinking about it, instead of splitting access control between
do_xen_version and here it would be more normal to have this function DTRT
and for it to be called unconditionally.

Ian.

Re: [PATCH v1 1/4] xsm/libxl/xen_version: Add XSM for some of the xen_version commands.

[Martin Pohlack]

On 09.10.2015 11:31, Ian Campbell wrote:
> On Thu, 2015-10-08 at 22:56 -0400, Konrad Rzeszutek Wilk wrote:
>> The XENVER_[compile_info|changeset|commandline] are now
>> guarded by an XSM check.
> 
> I can guess, but please explain/justify why this is the case for these
> here.
> 
>> The rest: XENVER_[version|extraversion|capabilities|
>> parameters|get_features|page_size|guest_handle] behave
>> as before (no XSM check).
> 
> and correspondingly why these ones to not warrant such a change.
>
>> As such we also modify the toolstack such that if we fail
>> to get any data instead of printing (null) we just print "".
> 
> Perhaps the hypervisor should instead return "<denied>" or some suitable
> string indicating why (<denied-xsm>)?

Yes, that makes it more obvious to userland what is going on and when
debugging and might limit userland breakage where at least some string
is required.

Martin
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

Re: [PATCH v1 1/4] xsm/libxl/xen_version: Add XSM for some of the xen_version commands.

[Andrew Cooper]

On 09/10/15 03:56, Konrad Rzeszutek Wilk wrote:
> The XENVER_[compile_info|changeset|commandline] are now
> guarded by an XSM check.
>
> The rest: XENVER_[version|extraversion|capabilities|
> parameters|get_features|page_size|guest_handle] behave
> as before (no XSM check).
>
> We allow the initial domain to see these while the other
> guests are not permitted.
>
> As such we also modify the toolstack such that if we fail
> to get any data instead of printing (null) we just print "".
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/flask/policy/policy/modules/xen/xen.te |  2 +-
>  tools/libxl/libxl.c                          |  3 +++
>  xen/common/kernel.c                          | 11 ++++++++++-
>  xen/include/xsm/dummy.h                      | 24 ++++++++++++++++++++++++
>  xen/include/xsm/xsm.h                        |  6 ++++++
>  xen/xsm/dummy.c                              |  1 +
>  xen/xsm/flask/hooks.c                        | 25 +++++++++++++++++++++++++
>  xen/xsm/flask/policy/access_vectors          |  2 ++
>  8 files changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
> index d35ae22..d0ad758 100644
> --- a/tools/flask/policy/policy/modules/xen/xen.te
> +++ b/tools/flask/policy/policy/modules/xen/xen.te
> @@ -86,7 +86,7 @@ allow dom0_t dom0_t:domain {
>  };
>  allow dom0_t dom0_t:domain2 {
>  	set_cpuid gettsc settsc setscheduler set_max_evtchn set_vnumainfo
> -	get_vnumainfo psr_cmt_op psr_cat_op
> +	get_vnumainfo psr_cmt_op psr_cat_op version_use
>  };
>  allow dom0_t dom0_t:resource { add remove };
>  
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 22bbc29..efa6462 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5272,6 +5272,7 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
>      xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
>      info->xen_version_extra = strdup(u.xen_extra);
>  
> +    memset(&u.xen_cc, 0, sizeof(xen_compile_info_t));

FILLZERO()

>      xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
>      info->compiler = strdup(u.xen_cc.compiler);
>      info->compile_by = strdup(u.xen_cc.compile_by);
> @@ -5281,6 +5282,7 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
>      xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
>      info->capabilities = strdup(u.xen_caps);
>  
> +    memset(&u.xen_cc, 0, sizeof(xen_changeset_info_t));
>      xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
>      info->changeset = strdup(u.xen_chgset);
>  
> @@ -5289,6 +5291,7 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
>  
>      info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
>  
> +    memset(&u.xen_commandline, 0, sizeof(xen_commandline_t));
>      xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
>      info->commandline = strdup(u.xen_commandline);
>  
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 6a3196a..210ec99 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,17 @@ void __init do_initcalls(void)
>  /*
>   * Simple hypercalls.
>   */
> -
> +#define XENVER_CMD_XSM_CHECK    ( (1U << XENVER_compile_info) | \
> +                                  (1U << XENVER_changeset) | \
> +                                  (1U << XENVER_commandline) )
>  DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> +    if ( ( 1 << cmd ) & XENVER_CMD_XSM_CHECK )
> +    {
> +        int rc = xsm_version_op(XSM_PRIV, cmd);
> +        if ( rc )
> +            return rc;

This call should be unconditional.  It is not going to make a
measureable difference on XENVER_version latency.

~Andrew

Re: [PATCH v1 1/4] xsm/libxl/xen_version: Add XSM for some of the xen_version commands.

[Martin Pohlack]

On 09.10.2015 04:56, Konrad Rzeszutek Wilk wrote:
> The XENVER_[compile_info|changeset|commandline] are now
> guarded by an XSM check.
> 
> The rest: XENVER_[version|extraversion|capabilities|
> parameters|get_features|page_size|guest_handle] behave
> as before (no XSM check).

I think extraversion should also be guarded, it helps too much deducing
specific builds.

> We allow the initial domain to see these while the other
> guests are not permitted.
> 
> As such we also modify the toolstack such that if we fail
> to get any data instead of printing (null) we just print "".
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/flask/policy/policy/modules/xen/xen.te |  2 +-
>  tools/libxl/libxl.c                          |  3 +++
>  xen/common/kernel.c                          | 11 ++++++++++-
>  xen/include/xsm/dummy.h                      | 24 ++++++++++++++++++++++++
>  xen/include/xsm/xsm.h                        |  6 ++++++
>  xen/xsm/dummy.c                              |  1 +
>  xen/xsm/flask/hooks.c                        | 25 +++++++++++++++++++++++++
>  xen/xsm/flask/policy/access_vectors          |  2 ++
>  8 files changed, 72 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
> index d35ae22..d0ad758 100644
> --- a/tools/flask/policy/policy/modules/xen/xen.te
> +++ b/tools/flask/policy/policy/modules/xen/xen.te
> @@ -86,7 +86,7 @@ allow dom0_t dom0_t:domain {
>  };
>  allow dom0_t dom0_t:domain2 {
>  	set_cpuid gettsc settsc setscheduler set_max_evtchn set_vnumainfo
> -	get_vnumainfo psr_cmt_op psr_cat_op
> +	get_vnumainfo psr_cmt_op psr_cat_op version_use
>  };
>  allow dom0_t dom0_t:resource { add remove };
>  
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 22bbc29..efa6462 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5272,6 +5272,7 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
>      xc_version(ctx->xch, XENVER_extraversion, &u.xen_extra);
>      info->xen_version_extra = strdup(u.xen_extra);
>  
> +    memset(&u.xen_cc, 0, sizeof(xen_compile_info_t));
>      xc_version(ctx->xch, XENVER_compile_info, &u.xen_cc);
>      info->compiler = strdup(u.xen_cc.compiler);
>      info->compile_by = strdup(u.xen_cc.compile_by);
> @@ -5281,6 +5282,7 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
>      xc_version(ctx->xch, XENVER_capabilities, &u.xen_caps);
>      info->capabilities = strdup(u.xen_caps);
>  
> +    memset(&u.xen_cc, 0, sizeof(xen_changeset_info_t));
>      xc_version(ctx->xch, XENVER_changeset, &u.xen_chgset);
>      info->changeset = strdup(u.xen_chgset);
>  
> @@ -5289,6 +5291,7 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
>  
>      info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
>  
> +    memset(&u.xen_commandline, 0, sizeof(xen_commandline_t));
>      xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
>      info->commandline = strdup(u.xen_commandline);
>  
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 6a3196a..210ec99 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,17 @@ void __init do_initcalls(void)
>  /*
>   * Simple hypercalls.
>   */
> -
> +#define XENVER_CMD_XSM_CHECK    ( (1U << XENVER_compile_info) | \
> +                                  (1U << XENVER_changeset) | \
> +                                  (1U << XENVER_commandline) )
>  DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> +    if ( ( 1 << cmd ) & XENVER_CMD_XSM_CHECK )

"1" -> "1U" as 3 lines above.

> +    {
> +        int rc = xsm_version_op(XSM_PRIV, cmd);
> +        if ( rc )
> +            return rc;
> +    }
>      switch ( cmd )
>      {
>      case XENVER_version:
> diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
> index 9fe372c..2ee102e 100644
> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -720,4 +720,28 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG struct domain *d, unsigned int
>      }
>  }
>  
> +#include <public/version.h>
> +static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op)
> +{
> +    XSM_ASSERT_ACTION(XSM_PRIV);
> +    switch ( op )
> +    {
> +    case XENVER_compile_info:
> +    case XENVER_changeset:
> +    case XENVER_commandline:
> +        return xsm_default_action(XSM_PRIV, current->domain, NULL);
> +    case XENVER_version:
> +    case XENVER_extraversion:

See above.

> +    case XENVER_capabilities:
> +    case XENVER_platform_parameters:
> +    case XENVER_get_features:
> +    case XENVER_pagesize:
> +    case XENVER_guest_handle:
> +        /* The should _NEVER_ get here, but just in case. */
> +        return xsm_default_action(XSM_HOOK, current->domain, NULL);
> +    default:
> +        return -EPERM;
> +    }
> +}
> +
>  #endif /* CONFIG_X86 */
> diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
> index ba3caed..cd3e9b8 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
> @@ -727,6 +728,11 @@ static inline int xsm_pmu_op (xsm_default_t def, struct domain *d, unsigned int
>      return xsm_ops->pmu_op(d, op);
>  }
>  
> +static inline int xsm_version_op (xsm_default_t def, uint32_t op)
> +{
> +    return xsm_ops->version_op(op);
> +}
> +
>  #endif /* CONFIG_X86 */
>  
>  #endif /* XSM_NO_WRAPPERS */
> 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..4f1269d 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,29 @@ static int flask_pmu_op (struct domain *d, unsigned int op)
>  }
>  #endif /* CONFIG_X86 */
>  
> +static int flask_version_op (uint32_t op)
> +{
> +    switch ( op )
> +    {
> +    case XENVER_compile_info:
> +    case XENVER_changeset:
> +    case XENVER_commandline:
> +        return current_has_perm(current->domain, SECCLASS_DOMAIN2,
> +                                DOMAIN2__VERSION_USE);
> +    case XENVER_version:
> +    case XENVER_extraversion:

See above.

> +    case XENVER_capabilities:
> +    case XENVER_platform_parameters:
> +    case XENVER_get_features:
> +    case XENVER_pagesize:
> +    case XENVER_guest_handle:
> +        /* The XSM check is only for a subset of ops. The rest are allowed. */
> +        return 0;
> +    default:
> +        return -EPERM;
> +    }
> +}
> +
>  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 +1783,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..6e2e5d1 100644
> --- a/xen/xsm/flask/policy/access_vectors
> +++ b/xen/xsm/flask/policy/access_vectors
> @@ -242,6 +242,8 @@ class domain2
>      mem_sharing
>  # XEN_DOMCTL_psr_cat_op
>      psr_cat_op
> +# XENVER_[compile_info|changeset|commandline] usage. The rest is not checked.
> +   version_use

See above (+XENVER_extraversion).

>  }
>  
>  # Similar to class domain, but primarily contains domctls related to HVM domains
> 

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

[PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Konrad Rzeszutek Wilk]

All existing commands ignore the parameter so this does
not break the ABI. This paves the way for expanding the XENVER_
hypercall with variable size structures, such as
"XENVER_build_id: Provide ld-embedded build-ids"

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 xen/arch/arm/traps.c        | 2 +-
 xen/common/kernel.c         | 3 ++-
 xen/include/xen/hypercall.h | 6 ++++--
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9d2bd6a..9d687c4 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1223,7 +1223,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(sched_op, 2),
     HYPERCALL_DEPRECATED(sched_op_compat, 2),
     HYPERCALL(console_io, 3),
-    HYPERCALL(xen_version, 2),
+    HYPERCALL(xen_version, 3),
     HYPERCALL(xsm_op, 1),
     HYPERCALL(event_channel_op, 2),
     HYPERCALL_DEPRECATED(event_channel_op_compat, 1),
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 210ec99..7bab8de 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -230,7 +230,8 @@ void __init do_initcalls(void)
 #define XENVER_CMD_XSM_CHECK    ( (1U << XENVER_compile_info) | \
                                   (1U << XENVER_changeset) | \
                                   (1U << XENVER_commandline) )
-DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
+DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg,
+                unsigned int len)
 {
     if ( ( 1 << cmd ) & XENVER_CMD_XSM_CHECK )
     {
diff --git a/xen/include/xen/hypercall.h b/xen/include/xen/hypercall.h
index 26cb615..59b63ee 100644
--- a/xen/include/xen/hypercall.h
+++ b/xen/include/xen/hypercall.h
@@ -83,7 +83,8 @@ do_event_channel_op(
 extern long
 do_xen_version(
     int cmd,
-    XEN_GUEST_HANDLE_PARAM(void) arg);
+    XEN_GUEST_HANDLE_PARAM(void) arg,
+    unsigned int len);
 
 extern long
 do_console_io(
@@ -168,7 +169,8 @@ compat_xenoprof_op(int op, XEN_GUEST_HANDLE_PARAM(void) arg);
 extern int
 compat_xen_version(
     int cmd,
-    XEN_GUEST_HANDLE_PARAM(void) arg);
+    XEN_GUEST_HANDLE_PARAM(void) arg,
+    unsigned int len);
 
 extern int
 compat_sched_op(
-- 
2.1.0

Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Jan Beulich]

>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> All existing commands ignore the parameter so this does
> not break the ABI.

Does it not? What about the debug mode clobbering of hypercall
argument registers? I think such length indicators need to be part
of the newly added sub-structures instead.

> This paves the way for expanding the XENVER_
> hypercall with variable size structures, such as
> "XENVER_build_id: Provide ld-embedded build-ids"
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  xen/arch/arm/traps.c        | 2 +-

xen/arch/x86/x86_64/entry.S
xen/arch/x86/x86_64/compat/entry.S

(but that's moot with the comment above)

Jan

Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Andrew Cooper]

On 09/10/15 09:25, Jan Beulich wrote:
>>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
>> All existing commands ignore the parameter so this does
>> not break the ABI.
> Does it not? What about the debug mode clobbering of hypercall
> argument registers?

That is an implementation detail of the hypervisor.  It is irrelevant to
guests whether Xen chooses to clobber the spare registers or not.

> I think such length indicators need to be part
> of the newly added sub-structures instead.

I disagree. Having this as a hypercall parameter is ABI compatible, and
avoids unnecessary copy_from_guest()

~Andrew

>
>> This paves the way for expanding the XENVER_
>> hypercall with variable size structures, such as
>> "XENVER_build_id: Provide ld-embedded build-ids"
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> ---
>>  xen/arch/arm/traps.c        | 2 +-
> xen/arch/x86/x86_64/entry.S
> xen/arch/x86/x86_64/compat/entry.S
>
> (but that's moot with the comment above)
>
> Jan
>

Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Ian Campbell]

On Fri, 2015-10-09 at 13:29 +0100, Andrew Cooper wrote:
> On 09/10/15 09:25, Jan Beulich wrote:
> > > > > On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> > > All existing commands ignore the parameter so this does
> > > not break the ABI.
> > Does it not? What about the debug mode clobbering of hypercall
> > argument registers?
> 
> That is an implementation detail of the hypervisor.  It is irrelevant to
> guests whether Xen chooses to clobber the spare registers or not.

Or in other words the effect here is to clobber one _less_ register, and
the guest cannot have been relying on a register getting so clobbered (if
nothing else it doesn't happen in debug=n builds).

The flip side is that we are now no longer clobbering that register even
for existing sub-ops which do not use it (since the clobbering doesn't go
down to the subop level). So there is a risk that a guest may come to
depend on that register not being clobbered and then fail older debug=y
hypervisors.

This second scenario doesn't seem especially likely to me.

Do we not already have one or two hypercalls where subops consume different
numbers of parameters anyway? HYPERVISOR_sched_op I think has this property
and we've not been too concerned.

Ian.

Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Andrew Cooper]

On 09/10/15 13:46, Ian Campbell wrote:
> On Fri, 2015-10-09 at 13:29 +0100, Andrew Cooper wrote:
>> On 09/10/15 09:25, Jan Beulich wrote:
>>>>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
>>>> All existing commands ignore the parameter so this does
>>>> not break the ABI.
>>> Does it not? What about the debug mode clobbering of hypercall
>>> argument registers?
>> That is an implementation detail of the hypervisor.  It is irrelevant to
>> guests whether Xen chooses to clobber the spare registers or not.
> Or in other words the effect here is to clobber one _less_ register, and
> the guest cannot have been relying on a register getting so clobbered (if
> nothing else it doesn't happen in debug=n builds).

Correct - that is definitely a better phrasing.

> The flip side is that we are now no longer clobbering that register even
> for existing sub-ops which do not use it (since the clobbering doesn't go
> down to the subop level). So there is a risk that a guest may come to
> depend on that register not being clobbered and then fail older debug=y
> hypervisors.
>
> This second scenario doesn't seem especially likely to me.
>
> Do we not already have one or two hypercalls where subops consume different
> numbers of parameters anyway? HYPERVISOR_sched_op I think has this property
> and we've not been too concerned.

SCHEDOP_shutdown(suspend) effectively has a third parameter for PV
guests, but that is done entirely by the toolstack using the VMs
register context.

Xen isn't aware of it, and the duration of do_sched_op() does have the
register clobbered.

~Andrew

Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Jan Beulich]

>>> On 09.10.15 at 14:46, <ian.campbell@citrix.com> wrote:
> On Fri, 2015-10-09 at 13:29 +0100, Andrew Cooper wrote:
>> On 09/10/15 09:25, Jan Beulich wrote:
>> > > > > On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
>> > > All existing commands ignore the parameter so this does
>> > > not break the ABI.
>> > Does it not? What about the debug mode clobbering of hypercall
>> > argument registers?
>> 
>> That is an implementation detail of the hypervisor.  It is irrelevant to
>> guests whether Xen chooses to clobber the spare registers or not.
> 
> Or in other words the effect here is to clobber one _less_ register, and
> the guest cannot have been relying on a register getting so clobbered (if
> nothing else it doesn't happen in debug=n builds).

No, the one less register clobbered is in the first clobbering phase,
where _unused_ inputs get clobbered (for hypervisor internal
consumption). The second clobbering phase destroys all _used_
input registers' contents (the guest visible values), and _this_ is
what results in ABI breakage (because callers assuming the
hypercall to take two arguments assume that the 3rd argument
register will retain its contents.

Jan

Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Ian Campbell]

On Fri, 2015-10-09 at 08:38 -0600, Jan Beulich wrote:
> > > > On 09.10.15 at 14:46, <ian.campbell@citrix.com> wrote:
> > On Fri, 2015-10-09 at 13:29 +0100, Andrew Cooper wrote:
> > > On 09/10/15 09:25, Jan Beulich wrote:
> > > > > > > On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> > > > > All existing commands ignore the parameter so this does
> > > > > not break the ABI.
> > > > Does it not? What about the debug mode clobbering of hypercall
> > > > argument registers?
> > > 
> > > That is an implementation detail of the hypervisor.  It is irrelevant
> > > to
> > > guests whether Xen chooses to clobber the spare registers or not.
> > 
> > Or in other words the effect here is to clobber one _less_ register,
> > and
> > the guest cannot have been relying on a register getting so clobbered
> > (if
> > nothing else it doesn't happen in debug=n builds).
> 
> No, the one less register clobbered is in the first clobbering phase,
> where _unused_ inputs get clobbered (for hypervisor internal
> consumption). The second clobbering phase destroys all _used_
> input registers' contents (the guest visible values), and _this_ is
> what results in ABI breakage (because callers assuming the
> hypercall to take two arguments assume that the 3rd argument
> register will retain its contents.

Ah, yes, that's correct. My mistake.

Ian.

Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Konrad Rzeszutek Wilk]

On Fri, Oct 09, 2015 at 08:38:34AM -0600, Jan Beulich wrote:
> >>> On 09.10.15 at 14:46, <ian.campbell@citrix.com> wrote:
> > On Fri, 2015-10-09 at 13:29 +0100, Andrew Cooper wrote:
> >> On 09/10/15 09:25, Jan Beulich wrote:
> >> > > > > On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> >> > > All existing commands ignore the parameter so this does
> >> > > not break the ABI.
> >> > Does it not? What about the debug mode clobbering of hypercall
> >> > argument registers?
> >> 
> >> That is an implementation detail of the hypervisor.  It is irrelevant to
> >> guests whether Xen chooses to clobber the spare registers or not.
> > 
> > Or in other words the effect here is to clobber one _less_ register, and
> > the guest cannot have been relying on a register getting so clobbered (if
> > nothing else it doesn't happen in debug=n builds).
> 
> No, the one less register clobbered is in the first clobbering phase,
> where _unused_ inputs get clobbered (for hypervisor internal
> consumption). The second clobbering phase destroys all _used_
> input registers' contents (the guest visible values), and _this_ is
> what results in ABI breakage (because callers assuming the
> hypercall to take two arguments assume that the 3rd argument
> register will retain its contents.

Thanks for explaining it! I see my patch missed the change to
hypercall_table and along with compiling with debug=y it all worked
so I didn't get the 'len' parameter to be clobbered.

Ugh.

Then the right way to make this work would be to only clobber
the third argument if the XENVER_buildid command was used? And
preserve the third argument only if XENVER_buildid command was used.

And not clobber third argument in all other cases. That would
require some nasty tweaking of entry.S.

Ugh. I think going to the original idea of just having an
xen_build_id_t[1024] would be the easiest.

Or I can do a structure:

struct xen_build_id_t {
	uint32_t	len; /* IN: size of the buffer. */
	uint32_t 	_pad; /* IN: MUST be zero. */
	XEN_GUEST_HANDLE_64(char) buf; /* OUT: Buffer with build_id. */
}

Any preference? The 'xen_build_id_t[1024]' looks nicer.
> 
> Jan
> 

Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Andrew Cooper]

On 28/10/15 17:55, Konrad Rzeszutek Wilk wrote:
> On Fri, Oct 09, 2015 at 08:38:34AM -0600, Jan Beulich wrote:
>>>>> On 09.10.15 at 14:46, <ian.campbell@citrix.com> wrote:
>>> On Fri, 2015-10-09 at 13:29 +0100, Andrew Cooper wrote:
>>>> On 09/10/15 09:25, Jan Beulich wrote:
>>>>>>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
>>>>>> All existing commands ignore the parameter so this does
>>>>>> not break the ABI.
>>>>> Does it not? What about the debug mode clobbering of hypercall
>>>>> argument registers?
>>>> That is an implementation detail of the hypervisor.  It is irrelevant to
>>>> guests whether Xen chooses to clobber the spare registers or not.
>>> Or in other words the effect here is to clobber one _less_ register, and
>>> the guest cannot have been relying on a register getting so clobbered (if
>>> nothing else it doesn't happen in debug=n builds).
>> No, the one less register clobbered is in the first clobbering phase,
>> where _unused_ inputs get clobbered (for hypervisor internal
>> consumption). The second clobbering phase destroys all _used_
>> input registers' contents (the guest visible values), and _this_ is
>> what results in ABI breakage (because callers assuming the
>> hypercall to take two arguments assume that the 3rd argument
>> register will retain its contents.
> Thanks for explaining it! I see my patch missed the change to
> hypercall_table and along with compiling with debug=y it all worked
> so I didn't get the 'len' parameter to be clobbered.
>
> Ugh.
>
> Then the right way to make this work would be to only clobber
> the third argument if the XENVER_buildid command was used? And
> preserve the third argument only if XENVER_buildid command was used.
>
> And not clobber third argument in all other cases. That would
> require some nasty tweaking of entry.S.
>
> Ugh. I think going to the original idea of just having an
> xen_build_id_t[1024] would be the easiest.
>
> Or I can do a structure:
>
> struct xen_build_id_t {
> 	uint32_t	len; /* IN: size of the buffer. */
> 	uint32_t 	_pad; /* IN: MUST be zero. */
> 	XEN_GUEST_HANDLE_64(char) buf; /* OUT: Buffer with build_id. */
> }
>
> Any preference? The 'xen_build_id_t[1024]' looks nicer.

The current interface the xen_version hypercall is mad.  It has the same
shortcoming as the C library function gets().

It is unfortunate that our current debug register-clobbering strategy
prevents altering the number of parameters in a forwards compatible.

There are two options to move forwards:
1) Introduce a new hypercall and relegate the existing one to being a
_compat.
2) Change our debug clobbering strategy.

We definitely dont any further contritions to the school of "pass a
pointer without a length and trust the other side".

I think it might be prudent to follow option 1), as that also allows to
fix other issues such as the arbitrary (and unreasonable IMO) 1k
restriction on the length of the hypervisor command line.

~Andrew

Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Konrad Rzeszutek Wilk]

On Wed, Oct 28, 2015 at 06:34:37PM +0000, Andrew Cooper wrote:
> On 28/10/15 17:55, Konrad Rzeszutek Wilk wrote:
> > On Fri, Oct 09, 2015 at 08:38:34AM -0600, Jan Beulich wrote:
> >>>>> On 09.10.15 at 14:46, <ian.campbell@citrix.com> wrote:
> >>> On Fri, 2015-10-09 at 13:29 +0100, Andrew Cooper wrote:
> >>>> On 09/10/15 09:25, Jan Beulich wrote:
> >>>>>>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> >>>>>> All existing commands ignore the parameter so this does
> >>>>>> not break the ABI.
> >>>>> Does it not? What about the debug mode clobbering of hypercall
> >>>>> argument registers?
> >>>> That is an implementation detail of the hypervisor.  It is irrelevant to
> >>>> guests whether Xen chooses to clobber the spare registers or not.
> >>> Or in other words the effect here is to clobber one _less_ register, and
> >>> the guest cannot have been relying on a register getting so clobbered (if
> >>> nothing else it doesn't happen in debug=n builds).
> >> No, the one less register clobbered is in the first clobbering phase,
> >> where _unused_ inputs get clobbered (for hypervisor internal
> >> consumption). The second clobbering phase destroys all _used_
> >> input registers' contents (the guest visible values), and _this_ is
> >> what results in ABI breakage (because callers assuming the
> >> hypercall to take two arguments assume that the 3rd argument
> >> register will retain its contents.
> > Thanks for explaining it! I see my patch missed the change to
> > hypercall_table and along with compiling with debug=y it all worked
> > so I didn't get the 'len' parameter to be clobbered.
> >
> > Ugh.
> >
> > Then the right way to make this work would be to only clobber
> > the third argument if the XENVER_buildid command was used? And
> > preserve the third argument only if XENVER_buildid command was used.
> >
> > And not clobber third argument in all other cases. That would
> > require some nasty tweaking of entry.S.
> >
> > Ugh. I think going to the original idea of just having an
> > xen_build_id_t[1024] would be the easiest.
> >
> > Or I can do a structure:
> >
> > struct xen_build_id_t {
> > 	uint32_t	len; /* IN: size of the buffer. */
> > 	uint32_t 	_pad; /* IN: MUST be zero. */
> > 	XEN_GUEST_HANDLE_64(char) buf; /* OUT: Buffer with build_id. */
> > }
> >
> > Any preference? The 'xen_build_id_t[1024]' looks nicer.
> 
> The current interface the xen_version hypercall is mad.  It has the same
> shortcoming as the C library function gets().
> 
> It is unfortunate that our current debug register-clobbering strategy
> prevents altering the number of parameters in a forwards compatible.
> 
> There are two options to move forwards:
> 1) Introduce a new hypercall and relegate the existing one to being a
> _compat.
> 2) Change our debug clobbering strategy.
> 
> We definitely dont any further contritions to the school of "pass a
> pointer without a length and trust the other side".
> 
> I think it might be prudent to follow option 1), as that also allows to
> fix other issues such as the arbitrary (and unreasonable IMO) 1k
> restriction on the length of the hypervisor command line.

<sigh> Yak shaving..

If we are going that route may I suggest another option (less
animal interaction):

3) Stick XENVER_build_id under the xSplice hypercall?


> 
> ~Andrew

Re: [PATCH v1 2/4] xen-version: Add third parameter (len) to the do_version hypercall.

[Jan Beulich]

>>> On 28.10.15 at 19:34, <andrew.cooper3@citrix.com> wrote:
> On 28/10/15 17:55, Konrad Rzeszutek Wilk wrote:
>> On Fri, Oct 09, 2015 at 08:38:34AM -0600, Jan Beulich wrote:
>>>>>> On 09.10.15 at 14:46, <ian.campbell@citrix.com> wrote:
>>>> On Fri, 2015-10-09 at 13:29 +0100, Andrew Cooper wrote:
>>>>> On 09/10/15 09:25, Jan Beulich wrote:
>>>>>>>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
>>>>>>> All existing commands ignore the parameter so this does
>>>>>>> not break the ABI.
>>>>>> Does it not? What about the debug mode clobbering of hypercall
>>>>>> argument registers?
>>>>> That is an implementation detail of the hypervisor.  It is irrelevant to
>>>>> guests whether Xen chooses to clobber the spare registers or not.
>>>> Or in other words the effect here is to clobber one _less_ register, and
>>>> the guest cannot have been relying on a register getting so clobbered (if
>>>> nothing else it doesn't happen in debug=n builds).
>>> No, the one less register clobbered is in the first clobbering phase,
>>> where _unused_ inputs get clobbered (for hypervisor internal
>>> consumption). The second clobbering phase destroys all _used_
>>> input registers' contents (the guest visible values), and _this_ is
>>> what results in ABI breakage (because callers assuming the
>>> hypercall to take two arguments assume that the 3rd argument
>>> register will retain its contents.
>> Thanks for explaining it! I see my patch missed the change to
>> hypercall_table and along with compiling with debug=y it all worked
>> so I didn't get the 'len' parameter to be clobbered.
>>
>> Ugh.
>>
>> Then the right way to make this work would be to only clobber
>> the third argument if the XENVER_buildid command was used? And
>> preserve the third argument only if XENVER_buildid command was used.
>>
>> And not clobber third argument in all other cases. That would
>> require some nasty tweaking of entry.S.
>>
>> Ugh. I think going to the original idea of just having an
>> xen_build_id_t[1024] would be the easiest.
>>
>> Or I can do a structure:
>>
>> struct xen_build_id_t {
>> 	uint32_t	len; /* IN: size of the buffer. */
>> 	uint32_t 	_pad; /* IN: MUST be zero. */
>> 	XEN_GUEST_HANDLE_64(char) buf; /* OUT: Buffer with build_id. */
>> }
>>
>> Any preference? The 'xen_build_id_t[1024]' looks nicer.
> 
> The current interface the xen_version hypercall is mad.  It has the same
> shortcoming as the C library function gets().
> 
> It is unfortunate that our current debug register-clobbering strategy
> prevents altering the number of parameters in a forwards compatible.
> 
> There are two options to move forwards:
> 1) Introduce a new hypercall and relegate the existing one to being a
> _compat.
> 2) Change our debug clobbering strategy.
> 
> We definitely dont any further contritions to the school of "pass a
> pointer without a length and trust the other side".

While I partly disagree with this one in the context here (there are no
pointers being passed without known length) ...

> I think it might be prudent to follow option 1), as that also allows to
> fix other issues such as the arbitrary (and unreasonable IMO) 1k
> restriction on the length of the hypervisor command line.

... I agree that this would be a proper route forward. However, I
can't see any downside to the less involved alternative Konrad
suggests, perhaps even without any extra indirection:

struct xen_build_id_t {
	uint32_t	len; /* IN: size of buf[]. */
	unsigned char buf[1]; /* OUT: Variable length buffer with build_id. */
}

That approach could then also be used for new sub-ops deprecating
ones where the current fixed size is an issue.

Jan

[PATCH v1 3/4] 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 size is provided to the hypervisor).

To make this work with libxc it requires expanding the
hypercall to use a second parameter. We provide an 'raw' version of
xc_version called xc_version_len. The wrapper around it is
xc_version which retains the automatic size detection.

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.

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>
---
 tools/flask/policy/policy/modules/xen/xen.te |  1 +
 tools/libxc/include/xenctrl.h                |  2 ++
 tools/libxc/xc_private.c                     | 37 +++++++++++++++-------------
 tools/libxc/xc_private.h                     |  4 ++-
 xen/arch/x86/Makefile                        | 11 +++++----
 xen/arch/x86/xen.lds.S                       |  6 +++++
 xen/common/kernel.c                          | 32 +++++++++++++++++++++++-
 xen/common/version.c                         | 37 ++++++++++++++++++++++++++++
 xen/include/public/version.h                 |  9 ++++++-
 xen/include/xen/version.h                    |  1 +
 xen/include/xsm/dummy.h                      |  1 +
 xen/xsm/flask/hooks.c                        |  3 +++
 xen/xsm/flask/policy/access_vectors          |  2 ++
 13 files changed, 121 insertions(+), 25 deletions(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
index d0ad758..98ba4f8 100644
--- a/tools/flask/policy/policy/modules/xen/xen.te
+++ b/tools/flask/policy/policy/modules/xen/xen.te
@@ -68,6 +68,7 @@ allow dom0_t xen_t:xen2 {
     resource_op
     psr_cmt_op
     psr_cat_op
+    version_priv
 };
 allow dom0_t xen_t:xen2 {
     pmu_ctrl
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 3bfa00b..8e1c0c0 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1633,6 +1633,8 @@ int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl);
 
 int xc_version(xc_interface *xch, int cmd, void *arg);
 
+int xc_version_len(xc_interface *xch, int cmd, void *arg, size_t len);
+
 int xc_flask_op(xc_interface *xch, xen_flask_op_t *op);
 
 /*
diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index 7c39897..c12dc34 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -674,11 +674,28 @@ int xc_sysctl(xc_interface *xch, struct xen_sysctl *sysctl)
     return do_sysctl(xch, sysctl);
 }
 
+int xc_version_len(xc_interface *xch, int cmd, void *arg, size_t len)
+{
+    DECLARE_HYPERCALL_BOUNCE(arg, len, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    int rc;
+
+    if ( (len != 0) && xc_hypercall_bounce_pre(xch, arg) )
+    {
+        PERROR("Could not bounce buffer for version hypercall");
+        return -ENOMEM;
+    }
+
+    rc = do_xen_version(xch, cmd, HYPERCALL_BUFFER(arg), len);
+
+    if ( len != 0 )
+        xc_hypercall_bounce_post(xch, arg);
+
+    return rc;
+}
+
 int xc_version(xc_interface *xch, int cmd, void *arg)
 {
-    DECLARE_HYPERCALL_BOUNCE(arg, 0, XC_HYPERCALL_BUFFER_BOUNCE_OUT); /* Size unknown until cmd decoded */
     size_t sz;
-    int rc;
 
     switch ( cmd )
     {
@@ -716,21 +733,7 @@ int xc_version(xc_interface *xch, int cmd, void *arg)
         ERROR("xc_version: unknown command %d\n", cmd);
         return -EINVAL;
     }
-
-    HYPERCALL_BOUNCE_SET_SIZE(arg, sz);
-
-    if ( (sz != 0) && xc_hypercall_bounce_pre(xch, arg) )
-    {
-        PERROR("Could not bounce buffer for version hypercall");
-        return -ENOMEM;
-    }
-
-    rc = do_xen_version(xch, cmd, HYPERCALL_BUFFER(arg));
-
-    if ( sz != 0 )
-        xc_hypercall_bounce_post(xch, arg);
-
-    return rc;
+    return xc_version_len(xch, cmd, arg, sz);
 }
 
 unsigned long xc_make_page_below_4G(
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 2df1d59..82ac1d4 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -227,7 +227,8 @@ void xc__hypercall_buffer_cache_release(xc_interface *xch);
 
 int do_xen_hypercall(xc_interface *xch, privcmd_hypercall_t *hypercall);
 
-static inline int do_xen_version(xc_interface *xch, int cmd, xc_hypercall_buffer_t *dest)
+static inline int do_xen_version(xc_interface *xch, int cmd,
+                                 xc_hypercall_buffer_t *dest, size_t len)
 {
     DECLARE_HYPERCALL;
     DECLARE_HYPERCALL_BUFFER_ARGUMENT(dest);
@@ -235,6 +236,7 @@ static inline int do_xen_version(xc_interface *xch, int cmd, xc_hypercall_buffer
     hypercall.op     = __HYPERVISOR_xen_version;
     hypercall.arg[0] = (unsigned long) cmd;
     hypercall.arg[1] = HYPERCALL_BUFFER_AS_ARG(dest);
+    hypercall.arg[2] = len;
 
     return do_xen_hypercall(xch, &hypercall);
 }
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index 39a8059..7e4140a 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -108,12 +108,13 @@ $(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 $@
+	$(OBJCOPY) --only-section=.note.gnu.build-id $@ build_id.o
 	rm -f $(@D)/.$(@F).[0-9]*
 
 EFI_LDFLAGS = $(patsubst -m%,-mi386pep,$(LDFLAGS)) --subsystem=10
@@ -128,7 +129,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 &&) :
@@ -141,8 +142,8 @@ $(TARGET).efi: prelink-efi.o efi.lds efi/relocs-dummy.o $(BASEDIR)/common/symbol
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
 	$(guard) $(NM) -n $(@D)/.$(@F).$(VIRT_BASE).1 | $(guard) $(BASEDIR)/tools/symbols >$(@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 7bab8de..48df798 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -229,7 +229,8 @@ void __init do_initcalls(void)
  */
 #define XENVER_CMD_XSM_CHECK    ( (1U << XENVER_compile_info) | \
                                   (1U << XENVER_changeset) | \
-                                  (1U << XENVER_commandline) )
+                                  (1U << XENVER_commandline) | \
+                                  (1U << XENVER_build_id) )
 DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg,
                 unsigned int len)
 {
@@ -367,6 +368,35 @@ 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:
+    {
+        int rc;
+        char *p = NULL;
+        unsigned int sz = 0;
+
+        if ( guest_handle_is_null(arg) )
+            return -EINVAL;
+
+        if ( len == 0 )
+            return -EINVAL;
+
+        if ( !guest_handle_okay(arg, len) )
+            return -EINVAL;
+
+        rc = xen_build_id(&p, &sz);
+        if ( rc )
+            return rc;
+
+        if ( sz > len )
+            return -ENOMEM;
+
+        if ( copy_to_guest(arg, p, sz) )
+            return -EFAULT;
+
+        return sz;
+    }
+
     }
 
     return -ENOSYS;
diff --git a/xen/common/version.c b/xen/common/version.c
index b152e27..26eeadf 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)
 {
@@ -55,3 +59,36 @@ const char *xen_banner(void)
 {
     return XEN_BANNER;
 }
+
+#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..e575d6b 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,12 @@ typedef struct xen_feature_info xen_feature_info_t;
 #define XENVER_commandline 9
 typedef char xen_commandline_t[1024];
 
+#define XENVER_build_id 10
+/*
+ * arg1 == pointer to char array, arg2 == size of char array.
+ * Return value is the actual size.
+ */
+
 #endif /* __XEN_PUBLIC_VERSION_H__ */
 
 /*
diff --git a/xen/include/xen/version.h b/xen/include/xen/version.h
index 81a3c7d..9584f36 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);
+int xen_build_id(char **p, unsigned int *len);
 
 #endif /* __XEN_VERSION_H__ */
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 2ee102e..66bc23e 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -729,6 +729,7 @@ static XSM_INLINE int xsm_version_op (XSM_DEFAULT_ARG uint32_t op)
     case XENVER_compile_info:
     case XENVER_changeset:
     case XENVER_commandline:
+    case XENVER_build_id:
         return xsm_default_action(XSM_PRIV, current->domain, NULL);
     case XENVER_version:
     case XENVER_extraversion:
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 4f1269d..829aaeb 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -1631,6 +1631,9 @@ static int flask_version_op (uint32_t op)
     case XENVER_commandline:
         return current_has_perm(current->domain, SECCLASS_DOMAIN2,
                                 DOMAIN2__VERSION_USE);
+    case XENVER_build_id:
+        return avc_has_perm(domain_sid(current->domain), SECINITSID_XEN,
+                            SECCLASS_XEN2, XEN2__VERSION_PRIV, NULL);
     case XENVER_version:
     case XENVER_extraversion:
     case XENVER_capabilities:
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index 6e2e5d1..58b75a0 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_build_id and other priv
+    version_priv
 }
 
 # Classes domain and domain2 consist of operations that a domain performs on
-- 
2.1.0

Re: [PATCH v1 3/4] XENVER_build_id: Provide ld-embedded build-ids

[Ian Campbell]

On Thu, 2015-10-08 at 22:56 -0400, Konrad Rzeszutek Wilk wrote:
> @@ -1633,6 +1633,8 @@ int xc_sysctl(xc_interface *xch, struct xen_sysctl
> *sysctl);
>  
>  int xc_version(xc_interface *xch, int cmd, void *arg);
>  
> +int xc_version_len(xc_interface *xch, int cmd, void *arg, size_t len);

If we are going this route (not sure if Jan's comments on #2 invalidate
this approach) then please just update the xc_version API and the in tree
callers.

libxc doesn't have a stable API so there is no need to provide variants for
compatibility.

Re: [PATCH v1 3/4] XENVER_build_id: Provide ld-embedded build-ids

[Martin Pohlack]

On 09.10.2015 04:56, Konrad Rzeszutek Wilk wrote:
> @@ -367,6 +368,35 @@ 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:
> +    {
> +        int rc;
> +        char *p = NULL;
> +        unsigned int sz = 0;
> +
> +        if ( guest_handle_is_null(arg) )
> +            return -EINVAL;
> +
> +        if ( len == 0 )
> +            return -EINVAL;
> +
> +        if ( !guest_handle_okay(arg, len) )
> +            return -EINVAL;

Shouldn't this return -EFAULT?

> +
> +        rc = xen_build_id(&p, &sz);
> +        if ( rc )
> +            return rc;
> +
> +        if ( sz > len )
> +            return -ENOMEM;
> +
> +        if ( copy_to_guest(arg, p, sz) )
> +            return -EFAULT;
> +
> +        return sz;
> +    }
> +
>      }
>  
>      return -ENOSYS;

Martin

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

Re: [PATCH v1 3/4] XENVER_build_id: Provide ld-embedded build-ids

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 3782 bytes --]

On 09/10/15 03:56, Konrad Rzeszutek Wilk wrote:
> @@ -367,6 +368,35 @@ 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:
> +    {
> +        int rc;
> +        char *p = NULL;
> +        unsigned int sz = 0;
> +
> +        if ( guest_handle_is_null(arg) )
> +            return -EINVAL;

A NULL guest handle should return size, in the same way that we have
size queries with other hypercalls.  In the future, build-id will
probably extend to sha256 and get//// longer as a result.

> +
> +        if ( len == 0 )
> +            return -EINVAL;

This check is redundant with the "sz > len" check below.

> +
> +        if ( !guest_handle_okay(arg, len) )
> +            return -EINVAL;

This check is performed by copy_to_guest() below.

> +
> +        rc = xen_build_id(&p, &sz);
> +        if ( rc )
> +            return rc;
> +
> +        if ( sz > len )
> +            return -ENOMEM;

ENOBUFS

> +
> +        if ( copy_to_guest(arg, p, sz) )
> +            return -EFAULT;
> +
> +        return sz;
> +    }
> +
>      }
>  
>      return -ENOSYS;
> diff --git a/xen/common/version.c b/xen/common/version.c
> index b152e27..26eeadf 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)
>  {
> @@ -55,3 +59,36 @@ const char *xen_banner(void)
>  {
>      return XEN_BANNER;
>  }
> +
> +#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 Elf_Note __note_gnu_build_id_start[],
__note_gnu_build_id_end[];

> +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;

const Elf_Note *n = __note_gnu_build_id_start;

> +
> +    /* Something is wrong. */
> +    if ( __note_gnu_build_id_end <= (char *)&__note_gnu_build_id_start )

Need to check for a full Note header as well.

if ( &n[1] > __note_gnu_build_id_end )

> +        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);

This information could be cached in a couple of static variables, so the
sanity checks are only performed once.

> +
> +    return 0;
> +}
> +#endif
> diff --git a/xen/include/public/version.h b/xen/include/public/version.h
> index 44f26b0..e575d6b 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,12 @@ typedef struct xen_feature_info xen_feature_info_t;
>  #define XENVER_commandline 9
>  typedef char xen_commandline_t[1024];
>  
> +#define XENVER_build_id 10
> +/*
> + * arg1 == pointer to char array, arg2 == size of char array.
> + * Return value is the actual size.

Return value is the number of bytes written, or -ve error.

~Andrew

[-- Attachment #1.2: Type: text/html, Size: 5700 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v1 3/4] XENVER_build_id: Provide ld-embedded build-ids

[Jan Beulich]

>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -108,12 +108,13 @@ $(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 \

You first need to figure whether $(LD) actually supports --build-id=.

Jan

Re: [PATCH v1 3/4] XENVER_build_id: Provide ld-embedded build-ids

[Konrad Rzeszutek Wilk]

On Fri, Oct 09, 2015 at 09:18:43AM -0600, Jan Beulich wrote:
> >>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -108,12 +108,13 @@ $(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 \
> 
> You first need to figure whether $(LD) actually supports --build-id=.

<sigh> Seems I just stumbled upon this email. Please ignore the other one.
> 
> Jan
> 

[PATCH v1 4/4] 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>
---
 tools/libxl/libxl.c         | 16 ++++++++++++++++
 tools/libxl/libxl_types.idl |  1 +
 tools/libxl/xl_cmdimpl.c    |  1 +
 3 files changed, 18 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index efa6462..f9af78c 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5249,6 +5249,7 @@ libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr)
     return ret;
 }
 
+#define BUILD_ID_LEN 1024 /* Same size as xen_commandline. */
 const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
 {
     union {
@@ -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;
+        char build_id[BUILD_ID_LEN];
     } u;
     long xen_version;
+    int rc;
     libxl_version_info *info = &ctx->version_info;
 
     if (info->xen_version_extra != NULL)
@@ -5295,8 +5298,21 @@ 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);
 
+    rc = xc_version_len(ctx->xch, XENVER_build_id, &u.build_id, BUILD_ID_LEN);
+    if (rc > 0) {
+        unsigned int i;
+
+        info->build_id = (char *)malloc((rc * 2) + 1);
+
+        for (i = 0; i < rc && (i + 1) * 2 < BUILD_ID_LEN; i++)
+            snprintf(&info->build_id[i * 2], 3, "%02hhx", u.build_id[i]);
+
+        info->build_id[i*2]='\0';
+    } else
+        info->build_id = strdup("");
     return info;
 }
+#undef BUILD_ID_LEN
 
 libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid,
                                        int *nr_vcpus_out, int *nr_cpus_out)
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index d6ef9a2..232749b 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 365798b..4f31099 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -5541,6 +5541,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

Re: [PATCH v1 4/4] libxl: info: Display build_id of the hypervisor.

[Ian Campbell]

On Thu, 2015-10-08 at 22:56 -0400, Konrad Rzeszutek Wilk wrote:
> If the hypervisor is built with we will display it.
> 
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  tools/libxl/libxl.c         | 16 ++++++++++++++++
>  tools/libxl/libxl_types.idl |  1 +
>  tools/libxl/xl_cmdimpl.c    |  1 +
>  3 files changed, 18 insertions(+)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index efa6462..f9af78c 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5249,6 +5249,7 @@ libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx,
> int *nr)
>      return ret;
>  }
>  
> +#define BUILD_ID_LEN 1024 /* Same size as xen_commandline. */

This either needs to be well defined in the hypercall ABI or the code here
needs to cope with expanding the buffer on ENOBUFS and trying again.

Ian.

Re: [PATCH v1 4/4] libxl: info: Display build_id of the hypervisor.

[Andrew Cooper]

On 09/10/15 03:56, Konrad Rzeszutek Wilk wrote:
> +    rc = xc_version_len(ctx->xch, XENVER_build_id, &u.build_id, BUILD_ID_LEN);
> +    if (rc > 0) {
> +        unsigned int i;
> +
> +        info->build_id = (char *)malloc((rc * 2) + 1);
> +
> +        for (i = 0; i < rc && (i + 1) * 2 < BUILD_ID_LEN; i++)
> +            snprintf(&info->build_id[i * 2], 3, "%02hhx", u.build_id[i]);
> +
> +        info->build_id[i*2]='\0';
> +    } else
> +        info->build_id = strdup("");

info->build_id is unconditionally leaked, given this patch.

~Andrew

Re: [PATCH v1 4/4] libxl: info: Display build_id of the hypervisor.

[Ian Campbell]

On Fri, 2015-10-09 at 13:59 +0100, Andrew Cooper wrote:
> On 09/10/15 03:56, Konrad Rzeszutek Wilk wrote:
> > +    rc = xc_version_len(ctx->xch, XENVER_build_id, &u.build_id,
> > BUILD_ID_LEN);
> > +    if (rc > 0) {
> > +        unsigned int i;
> > +
> > +        info->build_id = (char *)malloc((rc * 2) + 1);
> > +
> > +        for (i = 0; i < rc && (i + 1) * 2 < BUILD_ID_LEN; i++)
> > +            snprintf(&info->build_id[i * 2], 3, "%02hhx",
> > u.build_id[i]);
> > +
> > +        info->build_id[i*2]='\0';
> > +    } else
> > +        info->build_id = strdup("");
> 
> info->build_id is unconditionally leaked, given this patch.

It should be freed by libxl_version_info_dispose, which any correct callers
should already be using.


Ian.

Re: [PATCH v1 4/4] libxl: info: Display build_id of the hypervisor.

[Andrew Cooper]

On 09/10/15 14:06, Ian Campbell wrote:
> On Fri, 2015-10-09 at 13:59 +0100, Andrew Cooper wrote:
>> On 09/10/15 03:56, Konrad Rzeszutek Wilk wrote:
>>> +    rc = xc_version_len(ctx->xch, XENVER_build_id, &u.build_id,
>>> BUILD_ID_LEN);
>>> +    if (rc > 0) {
>>> +        unsigned int i;
>>> +
>>> +        info->build_id = (char *)malloc((rc * 2) + 1);
>>> +
>>> +        for (i = 0; i < rc && (i + 1) * 2 < BUILD_ID_LEN; i++)
>>> +            snprintf(&info->build_id[i * 2], 3, "%02hhx",
>>> u.build_id[i]);
>>> +
>>> +        info->build_id[i*2]='\0';
>>> +    } else
>>> +        info->build_id = strdup("");
>> info->build_id is unconditionally leaked, given this patch.
> It should be freed by libxl_version_info_dispose, which any correct callers
> should already be using.

Ah - so it will.  Sorry for the noise.

~Andrew

Re: [PATCH v1 4/4] libxl: info: Display build_id of the hypervisor.

[Ian Campbell]

On Fri, 2015-10-09 at 14:06 +0100, Ian Campbell wrote:
> On Fri, 2015-10-09 at 13:59 +0100, Andrew Cooper wrote:
> > On 09/10/15 03:56, Konrad Rzeszutek Wilk wrote:
> > > +    rc = xc_version_len(ctx->xch, XENVER_build_id, &u.build_id,
> > > BUILD_ID_LEN);
> > > +    if (rc > 0) {
> > > +        unsigned int i;
> > > +
> > > +        info->build_id = (char *)malloc((rc * 2) + 1);
> > > +
> > > +        for (i = 0; i < rc && (i + 1) * 2 < BUILD_ID_LEN; i++)
> > > +            snprintf(&info->build_id[i * 2], 3, "%02hhx",
> > > u.build_id[i]);
> > > +
> > > +        info->build_id[i*2]='\0';
> > > +    } else
> > > +        info->build_id = strdup("");
> > 
> > info->build_id is unconditionally leaked, given this patch.
> 
> It should be freed by libxl_version_info_dispose, which any correct callers
> should already be using.

This is a special case which is caching the result in the CTX, and the call
to dispose is actually in libxl_ctx_free not the caller, so the code is OK
but my "any correct callers" comment was bogus.

The caller can't/shouldn't call dispose because libxl_get_version_info
returns a const.

Ian.

Re: [PATCH v1 4/4] libxl: info: Display build_id of the hypervisor.

[Ian Campbell]

On Thu, 2015-10-08 at 22:56 -0400, Konrad Rzeszutek Wilk wrote:
> If the hypervisor is built with we will display it.

I think there is a word missing in this sentence. Perhaps "it" after
"with", or better "a build_id" or "blah blah feature enabled".

> @@ -5295,8 +5298,21 @@ 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);
>  
> +    rc = xc_version_len(ctx->xch, XENVER_build_id, &u.build_id, BUILD_ID_LEN);

Please see tools/libxl/CODING_STYLE. A variable called rc must only ever
contain libxl error codes (ERROR_*), which xc_version_len does not return.

"r" or "ret" is appropriate for the return value from a system call or
xc_*.

> +    if (rc > 0) {

Do you intentionally silently ignore a failure here? I think at the very
least you should LOG all but the ones which are expected and which you have
deemed tolerable.

> +        unsigned int i;
> +
> +        info->build_id = (char *)malloc((rc * 2) + 1);

Lack of an error check here, but in any case libxl__zalloc(NOGC, ...)
instead, so it isn't needed anyway.

> +
> +        for (i = 0; i < rc && (i + 1) * 2 < BUILD_ID_LEN; i++)
> +            snprintf(&info->build_id[i * 2], 3, "%02hhx", 
> u.build_id[i]);
> +
> +        info->build_id[i*2]='\0';

You can drop after switching to libxl__zalloc, since the buffer starts out
zeroed.

diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index d6ef9a2..232749b 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),

A #define LIBXL_HAVE_* in libxl.h is required to signal the presence of
this new field.

Re: [PATCH v1] Add build-id to XENVER hypercall.

[Jan Beulich]

>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> However they also change the behavior of the existing hypercall
> for XENVER_[compile_info|changeset|commandline] and make them
> dom0 accessible. This is if XSM is built in or not (though with
> XSM one can expose it to a guest if desired).

Wasn't the outcome of the previous discussion that we should not
alter default behavior for existing sub-ops? And even if I'm
misremembering, I can see reasons for not exposing the command
line, but what value has not exposing compile info and changeset
again? The more that the tool stack uses the two, and as we know
tool stacks or parts thereof can live in unprivileged domains. Plus
there is also a (hg-centric and hence generally broken) attempt to
store it in hvm_save().

> Please take a look and provide your feedback at your leisure.
> 
> Note:
>  * Hadn't tried compiling it on ARM cross compiler lately. In
>    the past I had to #ifdef CONFIG_ARM as the ARM code did not
>    use any ELF code so none of the ELF parts made any sense.
>  * The EFI build works kindof. It is missing an build_id.o stanza.

Hmm, I'm confused: Does this patch set work everywhere, or does
it not? In the latter case, shouldn't it be tagged RFC?

Jan

Re: [PATCH v1] Add build-id to XENVER hypercall.

[Andrew Cooper]

On 09/10/15 09:17, Jan Beulich wrote:
>>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
>> However they also change the behavior of the existing hypercall
>> for XENVER_[compile_info|changeset|commandline] and make them
>> dom0 accessible. This is if XSM is built in or not (though with
>> XSM one can expose it to a guest if desired).
> Wasn't the outcome of the previous discussion that we should not
> alter default behavior for existing sub-ops?

I raised a worry that some guests might break if they suddenly have
access to this information cut off.

> And even if I'm
> misremembering, I can see reasons for not exposing the command
> line, but what value has not exposing compile info and changeset
> again?

There is a fear that providing such information makes it easier for
attackers who have an exploit for a specific binary.

> The more that the tool stack uses the two, and as we know
> tool stacks or parts thereof can live in unprivileged domains.

I would argue than a fully unprivileged toolstack domain has no need for
any information from this hypercall.  It it needs some privilege, then
XSM is in use and it can be given access.

> Plus
> there is also a (hg-centric and hence generally broken) attempt to
> store it in hvm_save().

I will be addressing this in due course with my further cpuid plans.

~Andrew

Re: [PATCH v1] Add build-id to XENVER hypercall.

[Konrad Rzeszutek Wilk]

On Fri, Oct 09, 2015 at 01:15:42PM +0100, Andrew Cooper wrote:
> On 09/10/15 09:17, Jan Beulich wrote:
> >>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> >> However they also change the behavior of the existing hypercall
> >> for XENVER_[compile_info|changeset|commandline] and make them
> >> dom0 accessible. This is if XSM is built in or not (though with
> >> XSM one can expose it to a guest if desired).
> > Wasn't the outcome of the previous discussion that we should not
> > alter default behavior for existing sub-ops?
> 
> I raised a worry that some guests might break if they suddenly have
> access to this information cut off.

Let me double-confirm that the guests are OK with this being
gone. I did ran tests to see if the worked, but hadn't actually tried
acessing (/sys/hypervisor/xen*) the values.

> 
> > And even if I'm
> > misremembering, I can see reasons for not exposing the command
> > line, but what value has not exposing compile info and changeset
> > again?
> 
> There is a fear that providing such information makes it easier for
> attackers who have an exploit for a specific binary.
> 
> > The more that the tool stack uses the two, and as we know
> > tool stacks or parts thereof can live in unprivileged domains.
> 
> I would argue than a fully unprivileged toolstack domain has no need for
> any information from this hypercall.  It it needs some privilege, then
> XSM is in use and it can be given access.

What he said :-)
> 
> > Plus
> > there is also a (hg-centric and hence generally broken) attempt to
> > store it in hvm_save().
> 
> I will be addressing this in due course with my further cpuid plans.
> 
> ~Andrew

Re: [PATCH v1] Add build-id to XENVER hypercall.

[Jan Beulich]

>>> On 09.10.15 at 15:25, <konrad.wilk@oracle.com> wrote:
> On Fri, Oct 09, 2015 at 01:15:42PM +0100, Andrew Cooper wrote:
>> On 09/10/15 09:17, Jan Beulich wrote:
>> >>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
>> >> However they also change the behavior of the existing hypercall
>> >> for XENVER_[compile_info|changeset|commandline] and make them
>> >> dom0 accessible. This is if XSM is built in or not (though with
>> >> XSM one can expose it to a guest if desired).
>> > Wasn't the outcome of the previous discussion that we should not
>> > alter default behavior for existing sub-ops?
>> 
>> I raised a worry that some guests might break if they suddenly have
>> access to this information cut off.
> 
> Let me double-confirm that the guests are OK with this being
> gone. I did ran tests to see if the worked, but hadn't actually tried
> acessing (/sys/hypervisor/xen*) the values.

Well, this is the kind of thing you can't find out by testing _some_
guest(s) - you'd need to test with all possible ones, which of course
is not feasible. Hence we need to be very conservative when
deciding to restrict part of a so far guest-kind-indifferent ABI.

Jan

Re: [PATCH v1] Add build-id to XENVER hypercall.

[Konrad Rzeszutek Wilk]

On Fri, Oct 09, 2015 at 09:14:00AM -0600, Jan Beulich wrote:
> >>> On 09.10.15 at 15:25, <konrad.wilk@oracle.com> wrote:
> > On Fri, Oct 09, 2015 at 01:15:42PM +0100, Andrew Cooper wrote:
> >> On 09/10/15 09:17, Jan Beulich wrote:
> >> >>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> >> >> However they also change the behavior of the existing hypercall
> >> >> for XENVER_[compile_info|changeset|commandline] and make them
> >> >> dom0 accessible. This is if XSM is built in or not (though with
> >> >> XSM one can expose it to a guest if desired).
> >> > Wasn't the outcome of the previous discussion that we should not
> >> > alter default behavior for existing sub-ops?
> >> 
> >> I raised a worry that some guests might break if they suddenly have
> >> access to this information cut off.
> > 
> > Let me double-confirm that the guests are OK with this being
> > gone. I did ran tests to see if the worked, but hadn't actually tried
> > acessing (/sys/hypervisor/xen*) the values.
> 
> Well, this is the kind of thing you can't find out by testing _some_
> guest(s) - you'd need to test with all possible ones, which of course
> is not feasible. Hence we need to be very conservative when

I was thinking to test:

F19-64 F19-32 F17-64 F17-32 F16-32
F16-64 F15-32 F15-64 NetBSD FreeBSD RHEL5-64 RHEL5-32 SLES11-32 SLES12-32
OL6_X86_64_PVHVM OL6_X86_64_PV OL5_X86_64_PVHVM Win2K (with SuSE PV drivers)
WinXP (with Windows GPL drivers) and Windows 7 (with Windows GPL), SOL12

And see what happens when those are not available (poke in /sysfs or whatever
I can)

> deciding to restrict part of a so far guest-kind-indifferent ABI.

Right, so only three of them are off.

Perhaps an another option would be to return success and fill out the
value with an empty string?

That actually sounds nicer.
> 
> Jan
> 

Re: [PATCH v1] Add build-id to XENVER hypercall.

[Konrad Rzeszutek Wilk]

On Wed, Oct 28, 2015 at 11:42:41AM -0400, Konrad Rzeszutek Wilk wrote:
> On Fri, Oct 09, 2015 at 09:14:00AM -0600, Jan Beulich wrote:
> > >>> On 09.10.15 at 15:25, <konrad.wilk@oracle.com> wrote:
> > > On Fri, Oct 09, 2015 at 01:15:42PM +0100, Andrew Cooper wrote:
> > >> On 09/10/15 09:17, Jan Beulich wrote:
> > >> >>>> On 09.10.15 at 04:56, <konrad.wilk@oracle.com> wrote:
> > >> >> However they also change the behavior of the existing hypercall
> > >> >> for XENVER_[compile_info|changeset|commandline] and make them
> > >> >> dom0 accessible. This is if XSM is built in or not (though with
> > >> >> XSM one can expose it to a guest if desired).
> > >> > Wasn't the outcome of the previous discussion that we should not
> > >> > alter default behavior for existing sub-ops?
> > >> 
> > >> I raised a worry that some guests might break if they suddenly have
> > >> access to this information cut off.
> > > 
> > > Let me double-confirm that the guests are OK with this being
> > > gone. I did ran tests to see if the worked, but hadn't actually tried
> > > acessing (/sys/hypervisor/xen*) the values.
> > 
> > Well, this is the kind of thing you can't find out by testing _some_
> > guest(s) - you'd need to test with all possible ones, which of course
> > is not feasible. Hence we need to be very conservative when
> 
> I was thinking to test:
> 
> F19-64 F19-32 F17-64 F17-32 F16-32
> F16-64 F15-32 F15-64 NetBSD FreeBSD RHEL5-64 RHEL5-32 SLES11-32 SLES12-32
> OL6_X86_64_PVHVM OL6_X86_64_PV OL5_X86_64_PVHVM Win2K (with SuSE PV drivers)
> WinXP (with Windows GPL drivers) and Windows 7 (with Windows GPL), SOL12
> 
> And see what happens when those are not available (poke in /sysfs or whatever
> I can)
> 
> > deciding to restrict part of a so far guest-kind-indifferent ABI.
> 
> Right, so only three of them are off.
> 
> Perhaps an another option would be to return success and fill out the
> value with an empty string?
> 
> That actually sounds nicer.

Like this:

>From f5672c4b72361132798c0ec4bd124c9ddc80bd44 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Date: Mon, 28 Sep 2015 09:00:58 -0400
Subject: [PATCH] xsm/libxl/xen_version: Add XSM for the xen_version hypercall.

All of XENVER_* have now an XSM check.

The XENVER_[compile_info|changeset|commandline] are now
guarded by an XSM check for priviliged domains.

We allow the initial domain to see these while the other
guests are not permitted. But to not make them unhappy
we will return 0 or empty strings.

The rest: XENVER_[version|extraversion|capabilities|
parameters|get_features|page_size|guest_handle] behave
as before - allowed by default for guests (thought if the
XSM checks fail then we will return empty strings as well).

Also in case XSM for all version_* is disabled we add code
in libxl so that it won't divide by zero.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
v2: Do XSM check for all the XENVER_ ops.
v3: Add empty data conditions.
---
 tools/flask/policy/policy/modules/xen/xen.te |  9 ++++
 tools/libxl/libxl.c                          |  2 +
 xen/common/kernel.c                          | 63 ++++++++++++++++++++++------
 xen/include/xsm/dummy.h                      | 23 ++++++++++
 xen/include/xsm/xsm.h                        |  6 +++
 xen/xsm/dummy.c                              |  1 +
 xen/xsm/flask/hooks.c                        | 27 ++++++++++++
 xen/xsm/flask/policy/access_vectors          |  5 +++
 8 files changed, 124 insertions(+), 12 deletions(-)

diff --git a/tools/flask/policy/policy/modules/xen/xen.te b/tools/flask/policy/policy/modules/xen/xen.te
index d35ae22..40dd7f4 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]
+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/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 22bbc29..3c03b6a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5288,6 +5288,8 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
     info->virt_start = u.p_parms.virt_start;
 
     info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
+    if (!info->pagesize) /* No divide by zero! */
+	info->pagesize = 1;
 
     xc_version(ctx->xch, XENVER_commandline, &u.xen_commandline);
     info->commandline = strdup(u.xen_commandline);
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 6a3196a..c98c58c 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,12 +227,15 @@ void __init do_initcalls(void)
 /*
  * Simple hypercalls.
  */
-
 DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
+    int empty_data = xsm_version_op(XSM_HOOK, cmd);
+
     switch ( cmd )
     {
     case XENVER_version:
+        if ( empty_data )
+            return 0;
         return (xen_major_version() << 16) | xen_minor_version();
 
     case XENVER_extraversion:
@@ -239,7 +243,8 @@ 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());
+        if ( !empty_data )
+            safe_strcpy(extraversion, xen_extra_version());
         if ( copy_to_guest(arg, extraversion, ARRAY_SIZE(extraversion)) )
             return -EFAULT;
         return 0;
@@ -250,10 +255,13 @@ 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());
+        if ( !empty_data )
+        {
+            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());
+        }
         if ( copy_to_guest(arg, &info, 1) )
             return -EFAULT;
         return 0;
@@ -264,7 +272,8 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         xen_capabilities_info_t info;
 
         memset(info, 0, sizeof(info));
-        arch_get_xen_caps(&info);
+        if ( !empty_data )
+            arch_get_xen_caps(&info);
 
         if ( copy_to_guest(arg, info, ARRAY_SIZE(info)) )
             return -EFAULT;
@@ -277,6 +286,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
             .virt_start = HYPERVISOR_VIRT_START
         };
 
+        if ( empty_data )
+            params.virt_start = 0;
+
         if ( copy_to_guest(arg, &params, 1) )
             return -EFAULT;
         return 0;
@@ -288,7 +300,8 @@ 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());
+        if ( !empty_data )
+            safe_strcpy(chgset, xen_changeset());
         if ( copy_to_guest(arg, chgset, ARRAY_SIZE(chgset)) )
             return -EFAULT;
         return 0;
@@ -302,9 +315,14 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         if ( copy_from_guest(&fi, arg, 1) )
             return -EFAULT;
 
+        if ( empty_data )
+            memset(&fi, 0, sizeof(fi));
+
         switch ( fi.submap_idx )
         {
         case 0:
+            if ( empty_data )
+                break;
             fi.submap = (1U << XENFEAT_memory_op_vnode_supported);
             if ( VM_ASSIST(d, pae_extended_cr3) )
                 fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
@@ -345,19 +363,40 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
     }
 
     case XENVER_pagesize:
+        if ( empty_data )
+            return 0;
         return (!guest_handle_is_null(arg) ? -EINVAL : PAGE_SIZE);
 
     case XENVER_guest_handle:
-        if ( copy_to_guest(arg, current->domain->handle,
-                           ARRAY_SIZE(current->domain->handle)) )
+    {
+        xen_domain_handle_t hdl;
+        unsigned int len;
+
+        if ( empty_data )
+        {
+            len = sizeof(hdl);
+            memset(&hdl, 0, len);
+        } else
+            len = ARRAY_SIZE(current->domain->handle);
+
+        if ( copy_to_guest(arg, empty_data ? hdl : current->domain->handle,
+                           len) )
             return -EFAULT;
         return 0;
-
+    }
     case XENVER_commandline:
-        if ( copy_to_guest(arg, saved_cmdline, ARRAY_SIZE(saved_cmdline)) )
+    {
+        xen_commandline_t empty;
+
+        if ( empty_data )
+            memset(&empty, 0, ARRAY_SIZE(empty));
+
+        if ( copy_to_guest(arg, empty_data ? empty : saved_cmdline,
+                           ARRAY_SIZE(saved_cmdline)) )
             return -EFAULT;
         return 0;
     }
+    }
 
     return -ENOSYS;
 }
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 9fe372c..7fb6949 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -720,4 +720,27 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG struct domain *d, unsigned int
     }
 }
 
+#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_compile_info:
+    case XENVER_changeset:
+    case XENVER_commandline:
+        return xsm_default_action(XSM_PRIV, current->domain, NULL);
+    case XENVER_version:
+    case XENVER_extraversion:
+    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 -EPERM;
+    }
+}
+
 #endif /* CONFIG_X86 */
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index ba3caed..cd3e9b8 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
@@ -727,6 +728,11 @@ static inline int xsm_pmu_op (xsm_default_t def, struct domain *d, unsigned int
     return xsm_ops->pmu_op(d, op);
 }
 
+static inline int xsm_version_op (xsm_default_t def, uint32_t op)
+{
+    return xsm_ops->version_op(op);
+}
+
 #endif /* CONFIG_X86 */
 
 #endif /* XSM_NO_WRAPPERS */
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..a08a5fe 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,31 @@ 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_compile_info:
+    case XENVER_changeset:
+    case XENVER_commandline:
+        return avc_has_perm(dsid, SECINITSID_XEN, SECCLASS_XEN2,
+                            XEN2__VERSION_PRIV, NULL);
+    case XENVER_version:
+    case XENVER_extraversion:
+    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 -EPERM;
+    }
+}
+
 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 +1785,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..806f159 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] usage.
+   version_priv
 }
 
 # Classes domain and domain2 consist of operations that a domain performs on
@@ -242,6 +244,9 @@ class domain2
     mem_sharing
 # XEN_DOMCTL_psr_cat_op
     psr_cat_op
+# XENVER_[version|extraversion|capabilities|parameters|get_features|
+# page_size|guest_handle] usage.
+    version_use
 }
 
 # Similar to class domain, but primarily contains domctls related to HVM domains
-- 
2.1.0

> > 
> > Jan
> > 

Re: [PATCH v1] Add build-id to XENVER hypercall.

[Jan Beulich]

>>> On 28.10.15 at 20:00, <konrad.wilk@oracle.com> wrote:
> On Wed, Oct 28, 2015 at 11:42:41AM -0400, Konrad Rzeszutek Wilk wrote:
>> Perhaps an another option would be to return success and fill out the
>> value with an empty string?
>> 
>> That actually sounds nicer.

I disagree. You still change the ABI this way, the more that ...

> Like this:
> 
> From f5672c4b72361132798c0ec4bd124c9ddc80bd44 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Date: Mon, 28 Sep 2015 09:00:58 -0400
> Subject: [PATCH] xsm/libxl/xen_version: Add XSM for the xen_version 
> hypercall.
> 
> All of XENVER_* have now an XSM check.
> 
> The XENVER_[compile_info|changeset|commandline] are now
> guarded by an XSM check for priviliged domains.

... this matches what the patch does only in the dummy case (the
full policy case may yield any kind of behavior).

Nevertheless a couple of comments on the patch itself:

> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5288,6 +5288,8 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
>      info->virt_start = u.p_parms.virt_start;
>  
>      info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
> +    if (!info->pagesize) /* No divide by zero! */
> +	info->pagesize = 1;

I can't see any reason whatsoever to hide the page size from guests.

>  DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
> +    int empty_data = xsm_version_op(XSM_HOOK, cmd);

The variable name kind of suggests it to have boolean meaning, and its
uses below don't help at all making clear that's not the case. Perhaps
better to make it bool_t and use !! above?

>      switch ( cmd )
>      {
>      case XENVER_version:
> +        if ( empty_data )
> +            return 0;
>          return (xen_major_version() << 16) | xen_minor_version();

Another part I can't see a reason to hide. In fact, this may break
guests which adapt their behavior (use of certain hypercalls)
depending on hypervisor version.

> @@ -277,6 +286,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>              .virt_start = HYPERVISOR_VIRT_START
>          };
>  
> +        if ( empty_data )
> +            params.virt_start = 0;

This again may break guests (wanting to determine how much of the
address space to leave untouched). Our kernels use this (albeit with
proper error checking, so they wouldn't stop working, they just
would waste address space).

> @@ -302,9 +315,14 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( copy_from_guest(&fi, arg, 1) )
>              return -EFAULT;
>  
> +        if ( empty_data )
> +            memset(&fi, 0, sizeof(fi));
> +
>          switch ( fi.submap_idx )
>          {
>          case 0:
> +            if ( empty_data )
> +                break;
>              fi.submap = (1U << XENFEAT_memory_op_vnode_supported);
>              if ( VM_ASSIST(d, pae_extended_cr3) )
>                  fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);

This one, afaict, is _specifically_ meant to be available to everyone.

> --- a/xen/include/xsm/dummy.h
> +++ b/xen/include/xsm/dummy.h
> @@ -720,4 +720,27 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG struct domain *d, unsigned int
>      }
>  }
>  
> +#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_compile_info:
> +    case XENVER_changeset:
> +    case XENVER_commandline:

I'd expect these three to be replaced by default: - all subops should
always be accessible to privileged domains.

Jan

Re: [PATCH v1] Add build-id to XENVER hypercall.

[Konrad Rzeszutek Wilk]

On Thu, Oct 29, 2015 at 02:55:25AM -0600, Jan Beulich wrote:
> >>> On 28.10.15 at 20:00, <konrad.wilk@oracle.com> wrote:
> > On Wed, Oct 28, 2015 at 11:42:41AM -0400, Konrad Rzeszutek Wilk wrote:
> >> Perhaps an another option would be to return success and fill out the
> >> value with an empty string?
> >> 
> >> That actually sounds nicer.
> 
> I disagree. You still change the ABI this way, the more that ...
> 
> > Like this:
> > 
> > From f5672c4b72361132798c0ec4bd124c9ddc80bd44 Mon Sep 17 00:00:00 2001
> > From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > Date: Mon, 28 Sep 2015 09:00:58 -0400
> > Subject: [PATCH] xsm/libxl/xen_version: Add XSM for the xen_version 
> > hypercall.
> > 
> > All of XENVER_* have now an XSM check.
> > 
> > The XENVER_[compile_info|changeset|commandline] are now
> > guarded by an XSM check for priviliged domains.
> 
> ... this matches what the patch does only in the dummy case (the
> full policy case may yield any kind of behavior).

<nods>
> 
> Nevertheless a couple of comments on the patch itself:
> 
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -5288,6 +5288,8 @@ const libxl_version_info* libxl_get_version_info(libxl_ctx *ctx)
> >      info->virt_start = u.p_parms.virt_start;
> >  
> >      info->pagesize = xc_version(ctx->xch, XENVER_pagesize, NULL);
> > +    if (!info->pagesize) /* No divide by zero! */
> > +	info->pagesize = 1;
> 
> I can't see any reason whatsoever to hide the page size from guests.
> 
> >  DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >  {
> > +    int empty_data = xsm_version_op(XSM_HOOK, cmd);
> 
> The variable name kind of suggests it to have boolean meaning, and its
> uses below don't help at all making clear that's not the case. Perhaps
> better to make it bool_t and use !! above?

<nodes>
> 
> >      switch ( cmd )
> >      {
> >      case XENVER_version:
> > +        if ( empty_data )
> > +            return 0;
> >          return (xen_major_version() << 16) | xen_minor_version();
> 
> Another part I can't see a reason to hide. In fact, this may break
> guests which adapt their behavior (use of certain hypercalls)
> depending on hypervisor version.
> 
> > @@ -277,6 +286,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >              .virt_start = HYPERVISOR_VIRT_START
> >          };
> >  
> > +        if ( empty_data )
> > +            params.virt_start = 0;
> 
> This again may break guests (wanting to determine how much of the
> address space to leave untouched). Our kernels use this (albeit with
> proper error checking, so they wouldn't stop working, they just
> would waste address space).
> 
> > @@ -302,9 +315,14 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
> >          if ( copy_from_guest(&fi, arg, 1) )
> >              return -EFAULT;
> >  
> > +        if ( empty_data )
> > +            memset(&fi, 0, sizeof(fi));
> > +
> >          switch ( fi.submap_idx )
> >          {
> >          case 0:
> > +            if ( empty_data )
> > +                break;
> >              fi.submap = (1U << XENFEAT_memory_op_vnode_supported);
> >              if ( VM_ASSIST(d, pae_extended_cr3) )
> >                  fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
> 
> This one, afaict, is _specifically_ meant to be available to everyone.


OK, so we go back to that some of the subops should _not_ be behind
an XSM check as they are meant to be available to everyone.

Or rather - there is no point of an XSM check at all for those.


> 
> > --- a/xen/include/xsm/dummy.h
> > +++ b/xen/include/xsm/dummy.h
> > @@ -720,4 +720,27 @@ static XSM_INLINE int xsm_pmu_op (XSM_DEFAULT_ARG struct domain *d, unsigned int
> >      }
> >  }
> >  
> > +#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_compile_info:
> > +    case XENVER_changeset:
> > +    case XENVER_commandline:
> 
> I'd expect these three to be replaced by default: - all subops should
> always be accessible to privileged domains.

/me nods.
> 
> Jan
> 

Re: [PATCH v1] Add build-id to XENVER hypercall.

[Jan Beulich]

>>> On 29.10.15 at 20:47, <konrad.wilk@oracle.com> wrote:
> On Thu, Oct 29, 2015 at 02:55:25AM -0600, Jan Beulich wrote:
>> >>> On 28.10.15 at 20:00, <konrad.wilk@oracle.com> wrote:
>> > @@ -302,9 +315,14 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>> >          if ( copy_from_guest(&fi, arg, 1) )
>> >              return -EFAULT;
>> >  
>> > +        if ( empty_data )
>> > +            memset(&fi, 0, sizeof(fi));
>> > +
>> >          switch ( fi.submap_idx )
>> >          {
>> >          case 0:
>> > +            if ( empty_data )
>> > +                break;
>> >              fi.submap = (1U << XENFEAT_memory_op_vnode_supported);
>> >              if ( VM_ASSIST(d, pae_extended_cr3) )
>> >                  fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
>> 
>> This one, afaict, is _specifically_ meant to be available to everyone.
> 
> OK, so we go back to that some of the subops should _not_ be behind
> an XSM check as they are meant to be available to everyone.
> 
> Or rather - there is no point of an XSM check at all for those.

Yes - excluding certain sub-ops from being always accessible should
be the exception (i.e. where truly warranted by security
considerations). Only in those cases would I view it as appropriate
to return e.g. blank strings instead of the previous valid data.

Jan

Re: [PATCH v1] Add build-id to XENVER hypercall.

[Jan Beulich]

>>> On 09.10.15 at 14:15, <andrew.cooper3@citrix.com> wrote:
> On 09/10/15 09:17, Jan Beulich wrote:
>> The more that the tool stack uses the two, and as we know
>> tool stacks or parts thereof can live in unprivileged domains.
> 
> I would argue than a fully unprivileged toolstack domain has no need for
> any information from this hypercall.  It it needs some privilege, then
> XSM is in use and it can be given access.

True.

Jan