Re: [Xen-devel] [PATCH v3 39/52] xen: check parameter validity when parsing command line

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.08.17 at 14:52, <jgross@xxxxxxxx> wrote:
>  static void __init _cmdline_parse(const char *cmdline)
>  {
>      char opt[128], *optval, *optkey, *q;
> -    const char *p = cmdline;
> +    const char *p = cmdline, *s, *key;
>      const struct kernel_param *param;
> -    int bool_assert;
> +    int bool_assert, rctmp, rc;
> +    bool found;

If you touch this anyway, I think bool_assert should become bool too.
And perhaps worthwhile shrinking the scope of at least some of the
variables you add/touch.

> @@ -131,13 +157,21 @@ static void __init _cmdline_parse(const char *cmdline)
>                      safe_strcpy(opt, "no");
>                      optval = opt;
>                  }
> -                ((void (*)(const char *))param->var)(optval);
> +                rctmp = param->par.func(optval);
>                  break;
>              default:
>                  BUG();
>                  break;
>              }
> +
> +            if ( !rc )
> +                rc = rctmp;
>          }
> +
> +        if ( rc )
> +            printk("parameter \"%s\" has invalid value \"%s\"!\n", key, 
> optval);

Since a few different rc values are possible by now, it's perhaps
worth also logging rc.

> @@ -176,7 +210,8 @@ int __init parse_bool(const char *s)
>           !strcmp("on", s) ||
>           !strcmp("true", s) ||
>           !strcmp("enable", s) ||
> -         !strcmp("1", s) )
> +         !strcmp("1", s) ||
> +         !*s )
>          return 1;

Careful with this: Taking the "iommu=" example that I've commented
on in the other patch already, much depends on what you mean to
do about the problem there: "iommu=,..." should not end up
meaning "iommu=on,...".

> --- a/xen/include/xen/init.h
> +++ b/xen/include/xen/init.h
> @@ -83,7 +83,10 @@ struct kernel_param {
>          OPT_CUSTOM
>      } type;
>      unsigned int len;
> -    void *var;
> +    union {
> +        void *var;
> +        int (*func)(const char *);
> +    } par;
>  };
>  
>  extern const struct kernel_param __setup_start[], __setup_end[];
> @@ -95,23 +98,38 @@ extern const struct kernel_param __setup_start[], 
> __setup_end[];
>  
>  #define custom_param(_name, _var) \
>      __setup_str __setup_str_##_var[] = _name; \
> -    __kparam __setup_##_var = { __setup_str_##_var, OPT_CUSTOM, 0, _var }
> +    __kparam __setup_##_var = \
> +        { .name = __setup_str_##_var, \
> +          .type = OPT_CUSTOM, \
> +          .par.func = _var }

I much appreciate this, as it is only now that we can be sure all
handler functions are of the intended type.

Jan


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