| Message ID | 20190322192809.3002-2-vliaskovitis@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support for reading hypervisor parameters at runtime | expand |
>>> On 22.03.19 at 20:28, <vliaskovitis@suse.com> wrote: > Limitations: > - Custom runtime parameters (OPT_CUSTOM) are not supported yet. > - For integer parameters (OPT_UINT), only unsigned parameters are printed > correctly. For this latter case I wonder whether it wouldn't be better to return back the raw binary value, allowing the caller to format it in suitable ways (e.g. both signed and unsigned, or dec and hex). > +int runtime_get_params(const char *cmdline, char *values, I don't think "cmdline" is the correct term here ("opts" would be a possible better name), but then again this keeps it nicely in line with the actual cmdline parsing function. > + 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; > + > + if (!values) Style (missing blanks). But the question is whether this conditional is useful at all. The only caller passes a non-NULL value anyway. Otherwise, why wouldn't you check cmdline as well? > + return -EFAULT; > + > + for ( ; ; ) > + { > + /* Skip whitespace. */ > + while ( isspace(*p) ) > + p++; > + if ( *p == '\0' ) > + break; > + > + /* Grab the next whitespace-delimited option. */ > + q = optkey = opt; > + while ( (*p != ' ') && (*p != '\0') ) You've used isspace() above - why not here? > + { > + if ( (q - opt) < (sizeof(opt)-1) ) /* avoid overflow */ Missing blanks (around binary operator). I also think that you should return an error upon buffer overflow, instead of potentially returning the wrong option's value. > + *q++ = *p; > + 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); What you return here is the value as set into the buffer when the option was parsed, and before that string was actually consumed. Is this really what's interesting to the user? I'd rather expect them to be after what is actually in effect. Since we've decided against a "get" per-option hook, I consider it acceptable this way as long as the behavior is spelled out amongst the limitations. > + break; > + case OPT_UINT: I think (hope) I've said so on v1 already: Please have blank lines between individual case blocks. > + case OPT_SIZE: > + get_integer_param(param, ¶m_int); > + len = snprintf(val + bufpos, maxlen - bufpos, > + "%"PRIu64" ", param_int); > + break; > + case OPT_BOOL: > + get_integer_param(param, ¶m_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; Please be consistent as to whether you add "break;" in a default case positioned last in a switch(). Personally I'd prefer them to be there, but I won't insist. All I'd like to see is no mixed style within a single patch at least. > + } > + > + if (len < 0) > + rc = len; > + else if (len < maxlen - bufpos) > + /* if output was not truncated update buffer position */ > + bufpos += (size_t) len; > + else if (len > 0) > + rc = -ENOMEM; Various style issues here: Missing blanks inside if(), malformed and improperly indented comment. I also don't see the need for the cast, and if it was needed there would be a stray blank there. > + break; > + } > + > + /* no parameter was matched */ > + if ( param >= __param_end ) > + { > + rc = -EINVAL; > + } Stray braces. > + if (rc) > + break; Missing blanks again. But perhaps better make the loop "while ( !rc )" anyway, or "do { ... } while ( !rc );". > @@ -501,6 +501,57 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) > > break; > } > + case XEN_SYSCTL_get_parameter: As per above, blank line above here please. > + { > + char *params; > + char *values; Put both on the same line? > + 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; > + } > + > + else if ( copy_from_guest(params, op->u.get_parameter.params, > + op->u.get_parameter.size) ) The "else" is pointless here with the "break;" above, but if you were to keep it, then there should be no blank line ahead of it. The second line is also insufficiently indented. > + 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)) ) Indentation again. > + { > + ret = -EFAULT; > + } Stray braces again. Jan
Sorry for the delay, I was on a long vacation. On Fri, 2019-04-05 at 17:01 +0200, Jan Beulich wrote:On 22.03.19 at 20:28, <vliaskovitis@suse.com> wrote: > > Limitations: > > - Custom runtime parameters (OPT_CUSTOM) are not supported yet. > > - For integer parameters (OPT_UINT), only unsigned parameters are > > printed > > correctly. > > For this latter case I wonder whether it wouldn't be better to > return back the raw binary value, allowing the caller to format > it in suitable ways (e.g. both signed and unsigned, or dec and > hex). Currently the caller is oblivious to the parameters and their types, and all retrieved values are printed together in a single string (see patch 4/4). I'd like to keep it like that for simplicity. Otherwise I think the caller has to find the types of requested parameters to produce the right formatting. Actually, since OPT_UINT is used for both signed and unsigned integer parameters, case-by-case parameter formatting would be required to do this, and the libx* callers do not have that knowledge. A "get_" per-parameter function pointer, which would handle formatting for each parameter, be it int, uint, string or custom, seems cleaner to me than leaving it to the caller. By the way, the current implementation searches in [__param_start __param_end), which are only the runtime parameters, not all parameters, correct? So the series should be renamed to "Support for reading runtime-only hypervisor parameters". The command is useful for checking parameters that can be changed at runtime after all. I believe there are currently no signed integer runtime parameters. So alternatively we could add a warning to the commit message and/or to the code stating that signed integer runtime parameters, if added, would not be printed correctly at the moment. This would gloss over rather than solve the issue. If correct formatting for all possible types is a requirement, the get_function may be needed. Or we could separate integer from unsigned integer parameters with an additional OPT_INT parameter type. I don't know if either of these is desirable or simply overkill to implement just for this command. I 'll send a v3 when there is agreement on the above. [...] > > + { > > + case OPT_STR: > > + len = snprintf(val + bufpos, maxlen - bufpos, "%s > > ", > > + (char*)param->par.var); > > What you return here is the value as set into the buffer when the > option was parsed, and before that string was actually consumed. > Is this really what's interesting to the user? I'd rather expect them > to be after what is actually in effect. > > Since we've decided against a "get" per-option hook, I consider it > acceptable this way as long as the behavior is spelled out amongst > the limitations. > I 'll add this limitation to the commit message in the next version. thanks, - Vasilis
>>> On 30.04.19 at 21:05, <vliaskovitis@suse.com> wrote: > On Fri, 2019-04-05 at 17:01 +0200, Jan Beulich wrote:On 22.03.19 at > 20:28, <vliaskovitis@suse.com> wrote: >> > Limitations: >> > - Custom runtime parameters (OPT_CUSTOM) are not supported yet. >> > - For integer parameters (OPT_UINT), only unsigned parameters are >> > printed >> > correctly. >> >> For this latter case I wonder whether it wouldn't be better to >> return back the raw binary value, allowing the caller to format >> it in suitable ways (e.g. both signed and unsigned, or dec and >> hex). > > Currently the caller is oblivious to the parameters and their types, > and all retrieved values are printed together in a single string (see > patch 4/4). I'd like to keep it like that for simplicity. Otherwise I > think the caller has to find the types of requested parameters to > produce the right formatting. Actually, since OPT_UINT is used for both > signed and unsigned integer parameters, case-by-case parameter > formatting would be required to do this, and the libx* callers do not > have that knowledge. A "get_" per-parameter function pointer, which > would handle formatting for each parameter, be it int, uint, string or > custom, seems cleaner to me than leaving it to the caller. I disagree. The caller requires no knowledge of the actual format. It could easily format the string twice (once assuming it might be a signed quantity, and another time assuming it might be an unsigned one). All it would need to know is the width of the binary representation. > By the way, the current implementation searches in [__param_start > __param_end), which are only the runtime parameters, not all > parameters, correct? So the series should be renamed to "Support for > reading runtime-only hypervisor parameters". The command is useful for > checking parameters that can be changed at runtime after all. Yes, such renaming would seem desirable. > I believe there are currently no signed integer runtime parameters. So > alternatively we could add a warning to the commit message and/or to > the code stating that signed integer runtime parameters, if added, > would not be printed correctly at the moment. This would gloss over > rather than solve the issue. That's an option. As for other aspects, I don't heavily mind cases not getting dealt with right away which have no practical use right now, as long as the restrictions are clearly spelled out, such that no-one will be surprised when trying to add a runtime option beyond what the code is able to deal with. Jan
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..2d12a5bcf6 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,115 @@ 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(); + } + + 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; + + if (!values) + return -EFAULT; + + for ( ; ; ) + { + /* Skip whitespace. */ + while ( isspace(*p) ) + p++; + if ( *p == '\0' ) + break; + + /* Grab the next whitespace-delimited option. */ + q = optkey = opt; + while ( (*p != ' ') && (*p != '\0') ) + { + if ( (q - opt) < (sizeof(opt)-1) ) /* avoid overflow */ + *q++ = *p; + 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: + get_integer_param(param, ¶m_int); + len = snprintf(val + bufpos, maxlen - bufpos, + "%"PRIu64" ", param_int); + break; + case OPT_BOOL: + get_integer_param(param, ¶m_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 += (size_t) len; + else if (len > 0) + rc = -ENOMEM; + + break; + } + + /* no parameter was matched */ + if ( param >= __param_end ) + { + rc = -EINVAL; + } + + if (rc) + break; + } + + 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..1b65bd617e 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,57 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) u_sysctl) break; } + case XEN_SYSCTL_get_parameter: + { + char *params; + char *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; + } + + else 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 89939f43c8..96440a6041 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
Add a sysctl hypercall to support getting hypervisor parameters at runtime. Limitations: - Custom runtime parameters (OPT_CUSTOM) are not supported yet. - For integer parameters (OPT_UINT), only unsigned parameters are printed correctly. Signed-off-by: Vasilis Liaskovitis <vliaskovitis@suse.com> --- tools/flask/policy/modules/dom0.te | 2 +- xen/common/kernel.c | 110 ++++++++++++++++++++++++++++ xen/common/sysctl.c | 55 +++++++++++++- 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, 188 insertions(+), 3 deletions(-)