linux-kernel - Re: [PATCH 1/1] x86: sanitize argument of clearcpuid command-line option

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170117071738.GA3041@gmail.com>
Date:   Tue, 17 Jan 2017 08:17:39 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     "Odzioba, Lukasz" <lukasz.odzioba@...el.com>
Cc:     Borislav Petkov <bp@...en8.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "hpa@...or.com" <hpa@...or.com>, "x86@...nel.org" <x86@...nel.org>,
        "luto@...nel.org" <luto@...nel.org>,
        "slaoub@...il.com" <slaoub@...il.com>, "bp@...e.de" <bp@...e.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "Kleen, Andi" <andi.kleen@...el.com>,
        Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH 1/1] x86: sanitize argument of clearcpuid command-line
 option


* Odzioba, Lukasz <lukasz.odzioba@...el.com> wrote:

> >	pr_warn("x86/cpu: Ignoring invalid "clearcpuid=%s' option!\n", arg)
> >
> > Which would save quite a bit of head scratching and frustration when someone has a 
> > bad enough day to add silly typos to the kernel cmdline.
> 
> Is there any particular reason why we have such warnings only for early params?
> early_param handlers return non-zero values on success:
> 	linux/init.h: " * Emits warning if fn returns non-zero."
> __setup handlers in most cases seem to return 1 on success, is the expected
> behaviour documented somewhere?
> 
> After looking at some of the ~500 usages of __setup macro it seems that handler's ret
> code doesn't matter so much, because it is treated differently in various parts
> of the kernel. If we make it consistent possibly it could be solved similarly to 
> early params by something like this: 
> 
> diff --git a/init/main.c b/init/main.c
> index b0c9d6f..261178e 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -182,8 +182,12 @@ static bool __init obsolete_checksetup(char *line)
>                                 pr_warn("Parameter %s is obsolete, ignored\n",
>                                         p->str);
>                                 return true;
> -                       } else if (p->setup_func(line + n))
> -                               return true;
> +                       } else {
> +                               if (p->setup_func(line + n))
> +                                       return true;
> +                               else
> +                                       pr_warn("Malformed option '%s'\n", line);
> +                       }

That looks sensible to me! I'd tweak the message slightly:

	pr_warn("error: Ignoring invalid boot parameter '%s'\n", line);

to make it more clear that it's a boot option that has a problem (there are many 
other types of options), and to make sure the user knows that we ignored that 
option.

Mind sending this as a proper patch, with akpm Cc:-ed?

Thanks,

	Ingo