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

[Juergen Gross <jgross@xxxxxxxx>]

On 22/08/17 13:31, Jan Beulich wrote:
>>>> On 16.08.17 at 14:52, <jgross@xxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel