diff mbox series

[v4,1/4] xen: add hypercall for reading runtime parameters

Message ID 20190528145416.16918-2-vliaskovitis@suse.com (mailing list archive)
State New, archived
Headers show
Series Support for reading runtime hypervisor parameters | expand

Commit Message

Vasilis LIaskovitis May 28, 2019, 2:54 p.m. UTC
Add a sysctl hypercall to support reading hypervisor runtime parameters.

Limitations:
- Custom runtime parameters (OPT_CUSTOM) are not supported yet.
- For integer parameters (OPT_UINT), only unsigned parameters are printed
correctly.
- The implementation only reads runtime parameters, but it can be changed
to read all hypervisor parameters if needed. 

Signed-off-by: Vasilis Liaskovitis <vliaskovitis@suse.com>
---
 tools/flask/policy/modules/dom0.te  |   2 +-
 xen/common/kernel.c                 | 118 ++++++++++++++++++++++++++++
 xen/common/sysctl.c                 |  52 +++++++++++-
 xen/include/public/sysctl.h         |  18 +++++
 xen/include/xen/lib.h               |   1 +
 xen/xsm/flask/hooks.c               |   3 +
 xen/xsm/flask/policy/access_vectors |   2 +
 7 files changed, 193 insertions(+), 3 deletions(-)

Comments

Jan Beulich June 7, 2019, 1:49 p.m. UTC | #1
>>> On 28.05.19 at 16:54, <vliaskovitis@suse.com> wrote:
> +int runtime_get_params(const char *cmdline, char *values,
> +                      size_t maxlen)
> +{
> +    char opt[128], *optkey, *q, *val = values;
> +    const char *p = cmdline;
> +    const struct kernel_param *param;
> +    int rc = 0, len = 0;
> +    size_t bufpos = 0;
> +    uint64_t param_int;
> +
> +    while ( !rc )
> +    {
> +        /* Skip whitespace. */
> +        while ( isspace(*p) )
> +            p++;
> +        if ( *p == '\0' )
> +            break;
> +
> +        /* Grab the next whitespace-delimited option. */
> +        q = optkey = opt;
> +        while ( !isspace(*p) && (*p != '\0') )
> +        {
> +            if ( (q - opt) < (sizeof(opt) - 1) ) /* avoid overflow */
> +                *q++ = *p;
> +            else return -ENOMEM;
> +            p++;
> +        }
> +        *q = '\0';
> +
> +        for ( param = __param_start; param < __param_end; param++ )
> +        {
> +            if ( strcmp(param->name, optkey) )
> +                continue;
> +
> +            switch ( param->type )
> +            {
> +            case OPT_STR:
> +                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
> +                               (char*)param->par.var);
> +                break;
> +
> +            case OPT_UINT:
> +            case OPT_SIZE:
> +                /* Signed integer parameters are not supported yet.
> +                 * While there are no runtime signed integer parameters
> +                 * at the moment, adding one and trying to get its value
> +                 * with the current implementation will output the wrong
> +                 * value.
> +                 */

Comment style.

> +                get_integer_param(param, &param_int);
> +                len = snprintf(val + bufpos, maxlen - bufpos,
> +                               "%"PRIu64" ", param_int);
> +                break;
> +
> +            case OPT_BOOL:
> +                get_integer_param(param, &param_int);
> +                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
> +                               param_int ? "true" : "false");
> +                break;
> +
> +            case OPT_CUSTOM:
> +                /* Custom parameters are not supported yet. */
> +                rc = -EINVAL;
> +                break;
> +
> +            default:
> +                BUG();
> +                break;
> +            }
> +
> +            if ( len < 0 )
> +                rc = len;
> +            else if ( len < maxlen - bufpos )
> +                /* if output was not truncated update buffer position. */

Again. It's pretty odd to have a full stop (which isn't mandatory, but
I appreciate it being there) yet start with a lower case letter (which
isn't permitted).

> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -466,9 +466,9 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>              copyback = 1;
>          break;
>  
> +#define XEN_PARAMETER_MAX_SIZE 1023
>      case XEN_SYSCTL_set_parameter:
>      {
> -#define XEN_SET_PARAMETER_MAX_SIZE 1023
>          char *params;
>  
>          if ( op->u.set_parameter.pad[0] || op->u.set_parameter.pad[1] ||
> @@ -477,7 +477,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>              ret = -EINVAL;
>              break;
>          }
> -        if ( op->u.set_parameter.size > XEN_SET_PARAMETER_MAX_SIZE )
> +        if ( op->u.set_parameter.size > XEN_PARAMETER_MAX_SIZE )
>          {
>              ret = -E2BIG;
>              break;
> @@ -501,6 +501,54 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
>  
>          break;
>      }
> +    case XEN_SYSCTL_get_parameter:

Blank line between non-fall-through case blocks please.

These are all issues which could easily be fixed while committing,
and other than those I'm now fine with the patch. However, I'm
still not overly happy with the restrictions named, some of which
derive from certain interface decisions (like using ASCII strings
as return values). I'd therefore like to have at least one other
maintainer's opinion as to whether to indeed go with this interface
before giving my ack.

Jan
diff mbox series

Patch

diff --git a/tools/flask/policy/modules/dom0.te b/tools/flask/policy/modules/dom0.te
index a347d664f8..681d1a101b 100644
--- a/tools/flask/policy/modules/dom0.te
+++ b/tools/flask/policy/modules/dom0.te
@@ -16,7 +16,7 @@  allow dom0_t xen_t:xen {
 allow dom0_t xen_t:xen2 {
 	resource_op psr_cmt_op psr_alloc pmu_ctrl get_symbol
 	get_cpu_levelling_caps get_cpu_featureset livepatch_op
-	coverage_op set_parameter
+	coverage_op set_parameter get_parameter
 };
 
 # Allow dom0 to use all XENVER_ subops that have checks.
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 612575430f..6695ffc372 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -12,6 +12,7 @@ 
 #include <xen/paging.h>
 #include <xen/guest_access.h>
 #include <xen/hypercall.h>
+#include <xen/ctype.h>
 #include <xsm/xsm.h>
 #include <asm/current.h>
 #include <public/version.h>
@@ -52,6 +53,123 @@  static int assign_integer_param(const struct kernel_param *param, uint64_t val)
     return 0;
 }
 
+static int get_integer_param(const struct kernel_param *param, uint64_t *val)
+{
+    switch ( param->len )
+    {
+    case sizeof(uint8_t):
+        *val = *(uint8_t *)param->par.var;
+        break;
+
+    case sizeof(uint16_t):
+        *val = *(uint16_t *)param->par.var;
+        break;
+
+    case sizeof(uint32_t):
+        *val = *(uint32_t *)param->par.var;
+        break;
+
+    case sizeof(uint64_t):
+        *val = *(uint64_t *)param->par.var;
+        break;
+
+    default:
+        BUG();
+        break;
+    }
+
+    return 0;
+}
+
+int runtime_get_params(const char *cmdline, char *values,
+                      size_t maxlen)
+{
+    char opt[128], *optkey, *q, *val = values;
+    const char *p = cmdline;
+    const struct kernel_param *param;
+    int rc = 0, len = 0;
+    size_t bufpos = 0;
+    uint64_t param_int;
+
+    while ( !rc )
+    {
+        /* Skip whitespace. */
+        while ( isspace(*p) )
+            p++;
+        if ( *p == '\0' )
+            break;
+
+        /* Grab the next whitespace-delimited option. */
+        q = optkey = opt;
+        while ( !isspace(*p) && (*p != '\0') )
+        {
+            if ( (q - opt) < (sizeof(opt) - 1) ) /* avoid overflow */
+                *q++ = *p;
+            else return -ENOMEM;
+            p++;
+        }
+        *q = '\0';
+
+        for ( param = __param_start; param < __param_end; param++ )
+        {
+            if ( strcmp(param->name, optkey) )
+                continue;
+
+            switch ( param->type )
+            {
+            case OPT_STR:
+                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
+                               (char*)param->par.var);
+                break;
+
+            case OPT_UINT:
+            case OPT_SIZE:
+                /* Signed integer parameters are not supported yet.
+                 * While there are no runtime signed integer parameters
+                 * at the moment, adding one and trying to get its value
+                 * with the current implementation will output the wrong
+                 * value.
+                 */
+                get_integer_param(param, &param_int);
+                len = snprintf(val + bufpos, maxlen - bufpos,
+                               "%"PRIu64" ", param_int);
+                break;
+
+            case OPT_BOOL:
+                get_integer_param(param, &param_int);
+                len = snprintf(val + bufpos, maxlen - bufpos, "%s ",
+                               param_int ? "true" : "false");
+                break;
+
+            case OPT_CUSTOM:
+                /* Custom parameters are not supported yet. */
+                rc = -EINVAL;
+                break;
+
+            default:
+                BUG();
+                break;
+            }
+
+            if ( len < 0 )
+                rc = len;
+            else if ( len < maxlen - bufpos )
+                /* if output was not truncated update buffer position. */
+                bufpos += len;
+            else if ( len > 0 )
+                rc = -ENOMEM;
+
+            break;
+        }
+
+        /* no parameter was matched */
+        if ( param >= __param_end )
+            rc = -EINVAL;
+    }
+
+    return rc;
+}
+
 static int parse_params(const char *cmdline, const struct kernel_param *start,
                         const struct kernel_param *end)
 {
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index c0aa6bde4e..20be6a6d8d 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -466,9 +466,9 @@  long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
             copyback = 1;
         break;
 
+#define XEN_PARAMETER_MAX_SIZE 1023
     case XEN_SYSCTL_set_parameter:
     {
-#define XEN_SET_PARAMETER_MAX_SIZE 1023
         char *params;
 
         if ( op->u.set_parameter.pad[0] || op->u.set_parameter.pad[1] ||
@@ -477,7 +477,7 @@  long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
             ret = -EINVAL;
             break;
         }
-        if ( op->u.set_parameter.size > XEN_SET_PARAMETER_MAX_SIZE )
+        if ( op->u.set_parameter.size > XEN_PARAMETER_MAX_SIZE )
         {
             ret = -E2BIG;
             break;
@@ -501,6 +501,54 @@  long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl)
 
         break;
     }
+    case XEN_SYSCTL_get_parameter:
+    {
+        char *params, *values;
+
+        if ( op->u.get_parameter.pad[0] || op->u.get_parameter.pad[1] ||
+             op->u.get_parameter.pad[2] )
+        {
+            ret = -EINVAL;
+            break;
+        }
+        if ( op->u.get_parameter.size > XEN_PARAMETER_MAX_SIZE )
+        {
+            ret = -E2BIG;
+            break;
+        }
+        params = xmalloc_bytes(op->u.get_parameter.size + 1);
+        if ( !params )
+        {
+            ret = -ENOMEM;
+            break;
+        }
+
+        values = xmalloc_bytes(XEN_PARAMETER_MAX_SIZE);
+        if ( !values )
+        {
+            xfree(params);
+            ret = -ENOMEM;
+            break;
+        }
+
+        if ( copy_from_guest(params, op->u.get_parameter.params,
+                             op->u.get_parameter.size) )
+            ret = -EFAULT;
+        else
+        {
+            params[op->u.set_parameter.size] = 0;
+            ret = runtime_get_params(params, values, XEN_PARAMETER_MAX_SIZE);
+
+            if ( !ret && copy_to_guest(op->u.get_parameter.values, values,
+                                       strlen(values)) )
+                ret = -EFAULT;
+        }
+
+        xfree(params);
+        xfree(values);
+
+        break;
+    }
 
     default:
         ret = arch_do_sysctl(op, u_sysctl);
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index c49b4dcc99..7d77d57115 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1100,6 +1100,22 @@  typedef struct xen_sysctl_cpu_policy xen_sysctl_cpu_policy_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_cpu_policy_t);
 #endif
 
+/*
+ * XEN_SYSCTL_get_parameter
+ *
+ * Read hypervisor parameters at runtime.
+ * Parameters are a single string terminated by a NUL byte of max. size
+ * characters. Multiple settings can be specified by separating them
+ * with blanks.
+ */
+
+struct xen_sysctl_get_parameter {
+    XEN_GUEST_HANDLE_64(char) params;       /* IN: pointer to parameters. */
+    XEN_GUEST_HANDLE_64(char) values;       /* OUT: pointer to output values. */
+    uint16_t size;                          /* IN: size of parameters. */
+    uint16_t pad[3];                        /* IN: MUST be zero. */
+};
+
 struct xen_sysctl {
     uint32_t cmd;
 #define XEN_SYSCTL_readconsole                    1
@@ -1130,6 +1146,7 @@  struct xen_sysctl {
 #define XEN_SYSCTL_livepatch_op                  27
 #define XEN_SYSCTL_set_parameter                 28
 #define XEN_SYSCTL_get_cpu_policy                29
+#define XEN_SYSCTL_get_parameter                 30
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -1162,6 +1179,7 @@  struct xen_sysctl {
 #if defined(__i386__) || defined(__x86_64__)
         struct xen_sysctl_cpu_policy        cpu_policy;
 #endif
+        struct xen_sysctl_get_parameter     get_parameter;
         uint8_t                             pad[128];
     } u;
 };
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index e0b7bcb6b7..6e6367bb7a 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -71,6 +71,7 @@  struct domain;
 void cmdline_parse(const char *cmdline);
 int runtime_parse(const char *line);
 int parse_bool(const char *s, const char *e);
+int runtime_get_params(const char *cmdline, char *values, size_t maxlen);
 
 /**
  * Given a specific name, parses a string of the form:
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 3d00c747f6..1b832e9a4c 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -830,6 +830,9 @@  static int flask_sysctl(int cmd)
     case XEN_SYSCTL_set_parameter:
         return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,
                                     XEN2__SET_PARAMETER, NULL);
+    case XEN_SYSCTL_get_parameter:
+        return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,
+                                    XEN2__GET_PARAMETER, NULL);
 
     default:
         return avc_unknown_permission("sysctl", cmd);
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index e00448b776..c5ee21d852 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -103,6 +103,8 @@  class xen2
     coverage_op
 # XEN_SYSCTL_set_parameter
     set_parameter
+# XEN_SYSCTL_get_parameter
+    get_parameter
 }
 
 # Classes domain and domain2 consist of operations that a domain performs on