Re: [Xen-devel] [PATCH v3 48/52] xen: add hypercall for setting parameters at runtime

2017-08-23 Thread Jan Beulich 
>>> On 23.08.17 at 11:52,  wrote:
> On 22/08/17 13:31, Jan Beulich wrote:
> On 16.08.17 at 14:52,  wrote:
>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -1096,6 +1096,21 @@ struct xen_sysctl_livepatch_op {
>>>  typedef struct xen_sysctl_livepatch_op xen_sysctl_livepatch_op_t;
>>>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_livepatch_op_t);
>>>  
>>> +/*
>>> + * XEN_SYSCTL_set_parameter
>>> + *
>>> + * Change hypervisor parameters at runtime.
>>> + * The input string is parsed similar to the boot parameters.
>>> + */
>>> +
>>> +struct xen_sysctl_set_parameter {
>>> +XEN_GUEST_HANDLE_64(char) params;   /* IN: pointer to parameters. 
>>> */
>>> +uint16_t size;  /* IN: size of parameters. */
>> 
>> The combination of length and zero terminator is always a little
>> ambiguous: I think it should be clarified in the comment what
>> behavior to expect, unless you want to either disallow
>> embedded NULs or drop the size field.
> 
> Okay.
> 
> Are you fine with e.g.:
> 
> /* Parameters are a single string terminated by a NUL byte of max. size
>characters. Multiple settings can be specified by separating them
>with blanks. */

Sounds good.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 48/52] xen: add hypercall for setting parameters at runtime

2017-08-23 Thread Juergen Gross 
On 22/08/17 13:31, Jan Beulich wrote:
 On 16.08.17 at 14:52,  wrote:
>> --- a/xen/common/sysctl.c
>> +++ b/xen/common/sysctl.c
>> @@ -467,6 +467,42 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
>> u_sysctl)
>>  copyback = 1;
>>  break;
>>  
>> +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] ||
>> + op->u.set_parameter.pad[2] )
>> +{
>> +ret = -EINVAL;
>> +break;
>> +}
>> +if ( op->u.set_parameter.size > XEN_SET_PARAMETER_MAX_SIZE )
>> +{
>> +ret = -E2BIG;
>> +break;
>> +}
>> +params = xmalloc_bytes(op->u.set_parameter.size + 1);
>> +if ( !params )
>> +{
>> +ret = -ENOMEM;
>> +break;
>> +}
>> +if ( __copy_from_guest(params, op->u.set_parameter.params,
>> +   op->u.set_parameter.size) )
> 
> You didn't verify the handle earlier, so I think this needs to be
> copy_from_guest().

Aah, yes, of course.

> 
>> --- a/xen/include/public/sysctl.h
>> +++ b/xen/include/public/sysctl.h
>> @@ -1096,6 +1096,21 @@ struct xen_sysctl_livepatch_op {
>>  typedef struct xen_sysctl_livepatch_op xen_sysctl_livepatch_op_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_livepatch_op_t);
>>  
>> +/*
>> + * XEN_SYSCTL_set_parameter
>> + *
>> + * Change hypervisor parameters at runtime.
>> + * The input string is parsed similar to the boot parameters.
>> + */
>> +
>> +struct xen_sysctl_set_parameter {
>> +XEN_GUEST_HANDLE_64(char) params;   /* IN: pointer to parameters. */
>> +uint16_t size;  /* IN: size of parameters. */
> 
> The combination of length and zero terminator is always a little
> ambiguous: I think it should be clarified in the comment what
> behavior to expect, unless you want to either disallow
> embedded NULs or drop the size field.

Okay.

Are you fine with e.g.:

/* Parameters are a single string terminated by a NUL byte of max. size
   characters. Multiple settings can be specified by separating them
   with blanks. */


Juergen

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 48/52] xen: add hypercall for setting parameters at runtime

2017-08-22 Thread Jan Beulich 
>>> On 16.08.17 at 14:52,  wrote:
> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -467,6 +467,42 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>  copyback = 1;
>  break;
>  
> +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] ||
> + op->u.set_parameter.pad[2] )
> +{
> +ret = -EINVAL;
> +break;
> +}
> +if ( op->u.set_parameter.size > XEN_SET_PARAMETER_MAX_SIZE )
> +{
> +ret = -E2BIG;
> +break;
> +}
> +params = xmalloc_bytes(op->u.set_parameter.size + 1);
> +if ( !params )
> +{
> +ret = -ENOMEM;
> +break;
> +}
> +if ( __copy_from_guest(params, op->u.set_parameter.params,
> +   op->u.set_parameter.size) )

You didn't verify the handle earlier, so I think this needs to be
copy_from_guest().

> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -1096,6 +1096,21 @@ struct xen_sysctl_livepatch_op {
>  typedef struct xen_sysctl_livepatch_op xen_sysctl_livepatch_op_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_sysctl_livepatch_op_t);
>  
> +/*
> + * XEN_SYSCTL_set_parameter
> + *
> + * Change hypervisor parameters at runtime.
> + * The input string is parsed similar to the boot parameters.
> + */
> +
> +struct xen_sysctl_set_parameter {
> +XEN_GUEST_HANDLE_64(char) params;   /* IN: pointer to parameters. */
> +uint16_t size;  /* IN: size of parameters. */

The combination of length and zero terminator is always a little
ambiguous: I think it should be clarified in the comment what
behavior to expect, unless you want to either disallow
embedded NULs or drop the size field.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH v3 48/52] xen: add hypercall for setting parameters at runtime

2017-08-16 Thread Juergen Gross 
Add a sysctl hypercall to support setting parameters similar to
command line parameters, but at runtime. The parameters to set are
specified as a string, just like the boot parameters.

Cc: Daniel De Graaf 
Cc: Ian Jackson 
Cc: Wei Liu 
Cc: Andrew Cooper 
Cc: George Dunlap 
Cc: Jan Beulich 
Cc: Konrad Rzeszutek Wilk 
Cc: Stefano Stabellini 
Cc: Tim Deegan 
Signed-off-by: Juergen Gross 
---
V2:
- corrected XSM test (Daniel De Graaf)

V3:
- check pad[] to be zero (Jan Beulich)
- return E2BIG in case of parameters too long (Jan Beulich)
- move max. parameter size define to sysctl.c (Jan Beulich)
---
 tools/flask/policy/modules/dom0.te  |  2 +-
 xen/common/sysctl.c | 36 
 xen/include/public/sysctl.h | 17 +
 xen/xsm/flask/hooks.c   |  3 +++
 xen/xsm/flask/policy/access_vectors |  2 ++
 5 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/tools/flask/policy/modules/dom0.te 
b/tools/flask/policy/modules/dom0.te
index d0a4d91ac0..338caaf41e 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_cat_op pmu_ctrl get_symbol
get_cpu_levelling_caps get_cpu_featureset livepatch_op
-   gcov_op
+   gcov_op set_parameter
 };
 
 # Allow dom0 to use all XENVER_ subops that have checks.
diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index ae58a0f650..d08580f492 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -467,6 +467,42 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
u_sysctl)
 copyback = 1;
 break;
 
+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] ||
+ op->u.set_parameter.pad[2] )
+{
+ret = -EINVAL;
+break;
+}
+if ( op->u.set_parameter.size > XEN_SET_PARAMETER_MAX_SIZE )
+{
+ret = -E2BIG;
+break;
+}
+params = xmalloc_bytes(op->u.set_parameter.size + 1);
+if ( !params )
+{
+ret = -ENOMEM;
+break;
+}
+if ( __copy_from_guest(params, op->u.set_parameter.params,
+   op->u.set_parameter.size) )
+ret = -EFAULT;
+else
+{
+params[op->u.set_parameter.size] = 0;
+ret = runtime_parse(params);
+}
+
+xfree(params);
+
+break;
+}
+
 default:
 ret = arch_do_sysctl(op, u_sysctl);
 copyback = 0;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 9e51af61e1..29ef08efb1 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1096,6 +1096,21 @@ struct xen_sysctl_livepatch_op {
 typedef struct xen_sysctl_livepatch_op xen_sysctl_livepatch_op_t;
 DEFINE_XEN_GUEST_HANDLE(xen_sysctl_livepatch_op_t);
 
+/*
+ * XEN_SYSCTL_set_parameter
+ *
+ * Change hypervisor parameters at runtime.
+ * The input string is parsed similar to the boot parameters.
+ */
+
+struct xen_sysctl_set_parameter {
+XEN_GUEST_HANDLE_64(char) params;   /* IN: pointer to parameters. */
+uint16_t size;  /* IN: size of parameters. */
+uint16_t pad[3];/* IN: MUST be zero. */
+};
+typedef struct xen_sysctl_set_parameter xen_sysctl_set_parameter_t;
+DEFINE_XEN_GUEST_HANDLE(xen_sysctl_set_parameter_t);
+
 struct xen_sysctl {
 uint32_t cmd;
 #define XEN_SYSCTL_readconsole1
@@ -1124,6 +1139,7 @@ struct xen_sysctl {
 #define XEN_SYSCTL_get_cpu_levelling_caps25
 #define XEN_SYSCTL_get_cpu_featureset26
 #define XEN_SYSCTL_livepatch_op  27
+#define XEN_SYSCTL_set_parameter 28
 uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
 union {
 struct xen_sysctl_readconsole   readconsole;
@@ -1152,6 +1168,7 @@ struct xen_sysctl {
 struct xen_sysctl_cpu_levelling_caps cpu_levelling_caps;
 struct xen_sysctl_cpu_featuresetcpu_featureset;
 struct xen_sysctl_livepatch_op  livepatch;
+struct xen_sysctl_set_parameter set_parameter;
 uint8_t pad[128];
 } u;
 };
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index 91146275bb..ad6e458822 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -827,6 +827,9 @@ static int flask_sysctl(int cmd)
 case XEN_SYSCTL_gcov_op:
 return avc_current_has_perm(SECINITSID_XEN, SECCLASS_XEN2,