Re: [Xen-devel] [PATCH v1 1/4] xen: add hypercall for getting parameters at runtime

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 06.03.19 at 13:58, <vliaskovitis@xxxxxxxx> wrote:
> +static int get_params(const char *cmdline, char *values,
> +                      const struct kernel_param *start,
> +                      const struct kernel_param *end)
> +{
> +    char opt[128], *optkey, *q;
> +    const char *p = cmdline, *val = values;

Why is val a pointer to const char? There are several casts below
because of this, and all these casts should go away.

> +    const struct kernel_param *param;
> +    int len, rc = 0;
> +    uint64_t param_int;
> +    bool found;
> +
> +    if (!values)
> +        return -EFAULT;
> +
> +    for ( ; ; )
> +    {
> +        /* Skip whitespace. */
> +        while ( *p == ' ' )
> +            p++;

For a user exposed interface I think it would be better to at least
use isspace() here.

> +        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 */

Even if you copy existing code, please correct obvious style
violations in the new instance - here there are blanks missing
around binary operators.

> +                *q++ = *p;
> +            p++;
> +        }
> +        *q = '\0';
> +
> +        /* Boolean parameters can be inverted with 'no-' prefix. */

This is an inapplicable comment in this function.

> +        found = false;

I don't think you need this variable here, or if so, it shouldn't
be boolean: Either you mean to support returning data for
multiple matching options (there are no runtime ones at
present, but we at least used to have multiple-instance
boot time ones), in which case you may need to invent a
means to disambiguate them. Or you don't, in which case
you could bail from the loop once you've found a match.

> +        for ( param = start; param < end; param++ )
> +        {
> +
> +            if ( strcmp(param->name, optkey) )
> +                continue;
> +
> +            found = true;
> +            switch ( param->type )
> +            {
> +            case OPT_STR:
> +                len = snprintf((char*)val, sizeof(values), "%s ",

Here and below, sizeof(values) is wrong in two ways: For one
it's sizeof(char*), not sizeof(char[nn]). And then you need to
subtract the amount of space consumed already.

> +                               (char*)param->par.var);
> +                val += len;
> +                break;
> +            case OPT_UINT:
> +                get_integer_param(param, &param_int);
> +                len = snprintf((char*)val, sizeof(values), "%lu ", 
> param_int);

This is going to fail to build on 32-bit Arm. I'm also
unconvinced this is appropriate if the value is actually
a signed quantity.

> +                val += len;
> +                break;
> +            case OPT_BOOL:
> +                get_integer_param(param, &param_int);
> +                len = snprintf((char*)val, sizeof(values), "%s",

Above you add trailing blanks - why not here?

> +                               param_int ? "true" : "false");
> +                val += len;
> +                break;
> +            case OPT_SIZE:
> +            case OPT_CUSTOM:
> +                rc = -EINVAL;
> +                break;

I can see why custom parameters can't be dealt with yet,
but why also size ones? As to custom ones - four out of the
seven runtime parameters we currently have are custom
ones, so this limits the utility of the change quite a bit.

> +            default:
> +                BUG();
> +                break;
> +            }
> +        }
> +
> +        if ( !found )
> +        {
> +            printk("get-parameters: parameter \"%s\" unknown!\n", optkey);

I don't think logging of a message is appropriate here.

> +            rc = -EINVAL;
> +        }
> +    }
> +    *val = '\0';

Blank line above this one please. But does this even build, with val
being pointer to const char?

> @@ -199,6 +303,11 @@ int runtime_parse(const char *line)
>      return parse_params(line, __param_start, __param_end);
>  }
>  
> +int runtime_get_parameter(const char *line, char *values)
> +{
> +    return get_params(line, values, __param_start, __param_end);
> +}

I don't see the need for this wrapper.

> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -501,6 +501,51 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>  
>          break;
>      }
> +    case XEN_SYSCTL_get_parameter:
> +    {
> +#define XEN_GET_PARAMETER_MAX_SIZE 1023

May I suggest to re-use XEN_SET_PARAMETER_MAX_SIZE,
perhaps after renaming to XEN_PARAMETER_MAX_SIZE?

> +        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_GET_PARAMETER_MAX_SIZE )
> +        {
> +            ret = -E2BIG;
> +            break;
> +        }
> +        params = xmalloc_bytes(op->u.get_parameter.size + 1);
> +        values = xmalloc_bytes(XEN_GET_PARAMETER_MAX_SIZE);
> +        if ( !params )
> +        {
> +            ret = -ENOMEM;
> +            break;
> +        }

What if params is non-NULL, but values is NULL? Also here and in one
instance below you leak memory on the error paths.

> +        if ( copy_from_guest(params, op->u.get_parameter.params,
> +                             op->u.get_parameter.size) )
> +            ret = -EFAULT;
> +        else
> +        {
> +            params[op->u.get_parameter.size] = 0;

Even if this is a sysctl, please use array_access_nospec() here.

> +            ret = runtime_get(params, values);
> +
> +            if ( copy_to_guest(op->u.get_parameter.values, values,
> +                               strlen(values)) )

You should only copy when ret is zero.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel