| Message ID | 20170809070706.13481-40-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 09, 2017 at 09:06:53AM +0200, Juergen Gross wrote: > Where possible check validity of parameters in _cmdline_parse() and > issue a warning message in case of an error detected. > > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Tim Deegan <tim@xen.org> > Cc: Wei Liu <wei.liu2@citrix.com> > Signed-off-by: Juergen Gross <jgross@suse.com> > --- > xen/common/kernel.c | 44 ++++++++++++++++++++++++++++++++++---------- > 1 file changed, 34 insertions(+), 10 deletions(-) > > diff --git a/xen/common/kernel.c b/xen/common/kernel.c > index ce7cb8adb5..3fd3abe79c 100644 > --- a/xen/common/kernel.c > +++ b/xen/common/kernel.c > @@ -23,9 +23,11 @@ enum system_state system_state = SYS_STATE_early_boot; > xen_commandline_t saved_cmdline; > static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE; > > -static void __init assign_integer_param( > +static int __init assign_integer_param( > const struct kernel_param *param, uint64_t val) > { > + unsigned int bits = param->len * 8; > + BITS_PER_BYTE here. Otherwise: Reviewed-by: Wei Liu <wei.liu2@citrix.com>
On 10/08/17 15:02, Wei Liu wrote: > On Wed, Aug 09, 2017 at 09:06:53AM +0200, Juergen Gross wrote: >> Where possible check validity of parameters in _cmdline_parse() and >> issue a warning message in case of an error detected. >> >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> >> Cc: George Dunlap <George.Dunlap@eu.citrix.com> >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> >> Cc: Jan Beulich <jbeulich@suse.com> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Tim Deegan <tim@xen.org> >> Cc: Wei Liu <wei.liu2@citrix.com> >> Signed-off-by: Juergen Gross <jgross@suse.com> >> --- >> xen/common/kernel.c | 44 ++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 34 insertions(+), 10 deletions(-) >> >> diff --git a/xen/common/kernel.c b/xen/common/kernel.c >> index ce7cb8adb5..3fd3abe79c 100644 >> --- a/xen/common/kernel.c >> +++ b/xen/common/kernel.c >> @@ -23,9 +23,11 @@ enum system_state system_state = SYS_STATE_early_boot; >> xen_commandline_t saved_cmdline; >> static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE; >> >> -static void __init assign_integer_param( >> +static int __init assign_integer_param( >> const struct kernel_param *param, uint64_t val) >> { >> + unsigned int bits = param->len * 8; >> + > > BITS_PER_BYTE here. Okay. > > Otherwise: > > Reviewed-by: Wei Liu <wei.liu2@citrix.com> BTW: I've spotted a problem with parse_bool() in my patch: It should accept an empty string as "true", as specifying e.g. "sync_console" should set this option instead of issuing an error message. Does your R-b: still stand with the correction below? @@ -176,7 +200,8 @@ int __init parse_bool(const char *s) !strcmp("on", s) || !strcmp("true", s) || !strcmp("enable", s) || - !strcmp("1", s) ) + !strcmp("1", s) || + !strcmp("", s) ) return 1; return -1; Thanks, Juergen
>>> On 10.08.17 at 15:24, <jgross@suse.com> wrote: > @@ -176,7 +200,8 @@ int __init parse_bool(const char *s) > !strcmp("on", s) || > !strcmp("true", s) || > !strcmp("enable", s) || > - !strcmp("1", s) ) > + !strcmp("1", s) || > + !strcmp("", s) ) But not strcmp() please in such a case - !*s is quite sufficient there. Jan
On 10/08/17 15:32, Jan Beulich wrote: >>>> On 10.08.17 at 15:24, <jgross@suse.com> wrote: >> @@ -176,7 +200,8 @@ int __init parse_bool(const char *s) >> !strcmp("on", s) || >> !strcmp("true", s) || >> !strcmp("enable", s) || >> - !strcmp("1", s) ) >> + !strcmp("1", s) || >> + !strcmp("", s) ) > > But not strcmp() please in such a case - !*s is quite sufficient there. Okay. Juergen
On Thu, Aug 10, 2017 at 03:24:05PM +0200, Juergen Gross wrote: > On 10/08/17 15:02, Wei Liu wrote: > > On Wed, Aug 09, 2017 at 09:06:53AM +0200, Juergen Gross wrote: > >> Where possible check validity of parameters in _cmdline_parse() and > >> issue a warning message in case of an error detected. > >> > >> Cc: Andrew Cooper <andrew.cooper3@citrix.com> > >> Cc: George Dunlap <George.Dunlap@eu.citrix.com> > >> Cc: Ian Jackson <ian.jackson@eu.citrix.com> > >> Cc: Jan Beulich <jbeulich@suse.com> > >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > >> Cc: Stefano Stabellini <sstabellini@kernel.org> > >> Cc: Tim Deegan <tim@xen.org> > >> Cc: Wei Liu <wei.liu2@citrix.com> > >> Signed-off-by: Juergen Gross <jgross@suse.com> > >> --- > >> xen/common/kernel.c | 44 ++++++++++++++++++++++++++++++++++---------- > >> 1 file changed, 34 insertions(+), 10 deletions(-) > >> > >> diff --git a/xen/common/kernel.c b/xen/common/kernel.c > >> index ce7cb8adb5..3fd3abe79c 100644 > >> --- a/xen/common/kernel.c > >> +++ b/xen/common/kernel.c > >> @@ -23,9 +23,11 @@ enum system_state system_state = SYS_STATE_early_boot; > >> xen_commandline_t saved_cmdline; > >> static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE; > >> > >> -static void __init assign_integer_param( > >> +static int __init assign_integer_param( > >> const struct kernel_param *param, uint64_t val) > >> { > >> + unsigned int bits = param->len * 8; > >> + > > > > BITS_PER_BYTE here. > > Okay. > > > > > Otherwise: > > > > Reviewed-by: Wei Liu <wei.liu2@citrix.com> > > BTW: I've spotted a problem with parse_bool() in my patch: It should > accept an empty string as "true", as specifying e.g. "sync_console" > should set this option instead of issuing an error message. > > Does your R-b: still stand with the correction below? > Yes (with Jan's comment addressed)
diff --git a/xen/common/kernel.c b/xen/common/kernel.c index ce7cb8adb5..3fd3abe79c 100644 --- a/xen/common/kernel.c +++ b/xen/common/kernel.c @@ -23,9 +23,11 @@ enum system_state system_state = SYS_STATE_early_boot; xen_commandline_t saved_cmdline; static const char __initconst opt_builtin_cmdline[] = CONFIG_CMDLINE; -static void __init assign_integer_param( +static int __init assign_integer_param( const struct kernel_param *param, uint64_t val) { + unsigned int bits = param->len * 8; + switch ( param->len ) { case sizeof(uint8_t): @@ -43,14 +45,17 @@ static void __init assign_integer_param( default: BUG(); } + + return ( (val & (~0ULL << bits)) && ~(val | (~0ULL >> (65 - bits))) ) ? + -EOVERFLOW : 0; } static void __init _cmdline_parse(const char *cmdline) { char opt[128], *optval, *optkey, *q; - const char *p = cmdline; + const char *p = cmdline, *s; const struct kernel_param *param; - int bool_assert; + int bool_assert, rc = 0; for ( ; ; ) { @@ -97,8 +102,9 @@ static void __init _cmdline_parse(const char *cmdline) !strncmp(param->name, opt, q + 1 - opt) ) { optval[-1] = '='; - ((void (*)(const char *))param->var)(q); + rc = ((int (*)(const char *))param->var)(q); optval[-1] = '\0'; + break; } continue; } @@ -106,24 +112,34 @@ static void __init _cmdline_parse(const char *cmdline) switch ( param->type ) { case OPT_STR: + rc = 0; strlcpy(param->var, optval, param->len); break; case OPT_UINT: - assign_integer_param( + rc = assign_integer_param( param, - simple_strtoll(optval, NULL, 0)); + simple_strtoll(optval, &s, 0)); + if ( *s ) + rc = -EINVAL; break; case OPT_BOOL: - if ( !parse_bool(optval) ) + rc = parse_bool(optval); + if ( rc == -1 ) + break; + if ( !rc ) bool_assert = !bool_assert; + rc = 0; assign_integer_param(param, bool_assert); break; case OPT_SIZE: - assign_integer_param( + rc = assign_integer_param( param, - parse_size_and_unit(optval, NULL)); + parse_size_and_unit(optval, &s)); + if ( *s ) + rc = -EINVAL; break; case OPT_CUSTOM: + rc = -EINVAL; if ( !bool_assert ) { if ( *optval ) @@ -131,13 +147,21 @@ static void __init _cmdline_parse(const char *cmdline) safe_strcpy(opt, "no"); optval = opt; } - ((void (*)(const char *))param->var)(optval); + rc = ((int (*)(const char *))param->var)(optval); break; default: BUG(); break; } + + break; } + + if ( rc ) + printk("parameter \"%s\" has invalid value \"%s\"!\n", optkey, + optval); + if ( param >= __setup_end ) + printk("parameter \"%s\" unknown!\n", optkey); } }
Where possible check validity of parameters in _cmdline_parse() and issue a warning message in case of an error detected. Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/common/kernel.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-)