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

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

[Lukasz Odzioba]

A negative number can be specified in the cmdline which will be used as
setup_clear_cpu_cap() argument. With that we can clear/set some bit in
memory predceeding boot_cpu_data/cpu_caps_cleared which may cause kernel
to misbehave. This patch adds lower bound check to setup_disablecpuid().

Fixes: ac72e7888a61 ("x86: add generic clearcpuid=... option")

Signed-off-by: Lukasz Odzioba <lukasz.odzioba@intel.com>
---
As an example let's change definition of one_hundred variable:
ffffffff81c4eeec d one_hundred
ffffffff81d69720 D boot_cpu_data (0x14 is x86_capability offset)

8*(0xffffffff81d69734-0xffffffff81c4eeec) => 9257536 -2 because we
want to clear the second bit. With clearcpuid=-9257534 we change the
definition of one_hundread to 96 which is used among other things
as sysfs' max value for swappiness, so we can check the effect like so:
# echo 96 >  /proc/sys/vm/swappiness
# echo 97 >  /proc/sys/vm/swappiness
-bash: echo: write error: Invalid argument
---
 arch/x86/kernel/cpu/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index dc1697c..9bab7a8 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1221,7 +1221,7 @@ static __init int setup_disablecpuid(char *arg)
 {
 	int bit;
 
-	if (get_option(&arg, &bit) && bit < NCAPINTS*32)
+	if (get_option(&arg, &bit) && bit >= 0 && bit < NCAPINTS * 32)
 		setup_clear_cpu_cap(bit);
 	else
 		return 0;
-- 
1.8.3.1

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

[Borislav Petkov]

On Wed, Dec 28, 2016 at 02:55:40PM +0100, Lukasz Odzioba wrote:
> A negative number can be specified in the cmdline which will be used as
> setup_clear_cpu_cap() argument. With that we can clear/set some bit in
> memory predceeding boot_cpu_data/cpu_caps_cleared which may cause kernel
> to misbehave. This patch adds lower bound check to setup_disablecpuid().
> 
> Fixes: ac72e7888a61 ("x86: add generic clearcpuid=... option")
> 
> Signed-off-by: Lukasz Odzioba <lukasz.odzioba@intel.com>
> ---
> As an example let's change definition of one_hundred variable:
> ffffffff81c4eeec d one_hundred
> ffffffff81d69720 D boot_cpu_data (0x14 is x86_capability offset)
> 
> 8*(0xffffffff81d69734-0xffffffff81c4eeec) => 9257536 -2 because we
> want to clear the second bit. With clearcpuid=-9257534 we change the
> definition of one_hundread to 96 which is used among other things
> as sysfs' max value for swappiness, so we can check the effect like so:
> # echo 96 >  /proc/sys/vm/swappiness
> # echo 97 >  /proc/sys/vm/swappiness
> -bash: echo: write error: Invalid argument
> ---
>  arch/x86/kernel/cpu/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index dc1697c..9bab7a8 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1221,7 +1221,7 @@ static __init int setup_disablecpuid(char *arg)
>  {
>  	int bit;
>  
> -	if (get_option(&arg, &bit) && bit < NCAPINTS*32)
> +	if (get_option(&arg, &bit) && bit >= 0 && bit < NCAPINTS * 32)
>  		setup_clear_cpu_cap(bit);
>  	else
>  		return 0;
> -- 

Yap, that's a good catch!

Acked-by: Borislav Petkov <bp@suse.de>

I even got a splat while experimenting with this:


[    1.234575] BUG: unable to handle kernel paging request at ffffffff858bd540
[    1.236535] IP: memcpy_erms+0x6/0x10
[    1.236535] PGD 1c10067 
[    1.236535] PUD 1c11063 
[    1.236535] PMD 0 
[    1.236535] 
[    1.236535] Oops: 0000 [#1] PREEMPT SMP
[    1.236535] Modules linked in:
[    1.236535] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.10.0-rc1+ #8
[    1.236535] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Debian-1.8.2-1 04/01/2014
[    1.236535] task: ffff88007c3f8000 task.stack: ffffc9000031c000
[    1.236535] RIP: 0010:memcpy_erms+0x6/0x10
[    1.236535] RSP: 0000:ffffc9000031fd90 EFLAGS: 00010286
[    1.236535] RAX: ffff88007b94c600 RBX: 0000000000000180 RCX: 0000000000000180
[    1.236535] RDX: 0000000000000180 RSI: ffffffff858bd540 RDI: ffff88007b94c600
[    1.236535] RBP: ffffc9000031fda8 R08: ffff88007b94c600 R09: 0000000000000000
[    1.236535] R10: 0000000000000000 R11: 0000000000000000 R12: ffffffff858bd540
[    1.236535] R13: ffffffff81cfa780 R14: ffffffff81cfa800 R15: 0000000000000000
[    1.236535] FS:  0000000000000000(0000) GS:ffff88007ed00000(0000) knlGS:0000000000000000
[    1.236535] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[    1.236535] CR2: ffffffff858bd540 CR3: 0000000001c0f000 CR4: 00000000001406e0
[    1.236535] Call Trace:
[    1.236535]  ? kmemdup+0x36/0x50
[    1.236535]  ip6_route_net_init+0xb1/0x1a5
[    1.236535]  ops_init.constprop.17+0x3e/0x160
[    1.236535]  register_pernet_operations+0x9a/0xd0
[    1.236535]  ? unix_diag_init+0x12/0x12
[    1.236535]  register_pernet_subsys+0x2a/0x40
[    1.236535]  ip6_route_init+0x7f/0x28b
[    1.236535]  ? unix_diag_init+0x12/0x12
[    1.236535]  inet6_init+0x18c/0x335
[    1.236535]  do_one_initcall+0x53/0x1a0
[    1.236535]  ? parse_args+0x260/0x3e0
[    1.236535]  kernel_init_freeable+0x11d/0x1a3
[    1.236535]  ? rest_init+0x140/0x140
[    1.236535]  kernel_init+0xe/0x100
[    1.236535]  ret_from_fork+0x27/0x40
[    1.236535] Code: c3 90 90 90 90 eb 1e 0f 1f 00 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 f3 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 <f3> a4 c3 0f 1f 80 00 00 00 00 48 89 f8 48 83 fa 20 72 7e 40 38 
[    1.236535] RIP: memcpy_erms+0x6/0x10 RSP: ffffc9000031fd90
[    1.236535] CR2: ffffffff858bd540
[    1.236535] ---[ end trace 1c01a71156422a06 ]---


-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

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

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Wed, Dec 28, 2016 at 02:55:40PM +0100, Lukasz Odzioba wrote:
> > A negative number can be specified in the cmdline which will be used as
> > setup_clear_cpu_cap() argument. With that we can clear/set some bit in
> > memory predceeding boot_cpu_data/cpu_caps_cleared which may cause kernel
> > to misbehave. This patch adds lower bound check to setup_disablecpuid().
> > 
> > Fixes: ac72e7888a61 ("x86: add generic clearcpuid=... option")
> > 
> > Signed-off-by: Lukasz Odzioba <lukasz.odzioba@intel.com>
> > ---
> > As an example let's change definition of one_hundred variable:
> > ffffffff81c4eeec d one_hundred
> > ffffffff81d69720 D boot_cpu_data (0x14 is x86_capability offset)
> > 
> > 8*(0xffffffff81d69734-0xffffffff81c4eeec) => 9257536 -2 because we
> > want to clear the second bit. With clearcpuid=-9257534 we change the
> > definition of one_hundread to 96 which is used among other things
> > as sysfs' max value for swappiness, so we can check the effect like so:
> > # echo 96 >  /proc/sys/vm/swappiness
> > # echo 97 >  /proc/sys/vm/swappiness
> > -bash: echo: write error: Invalid argument
> > ---
> >  arch/x86/kernel/cpu/common.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index dc1697c..9bab7a8 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -1221,7 +1221,7 @@ static __init int setup_disablecpuid(char *arg)
> >  {
> >  	int bit;
> >  
> > -	if (get_option(&arg, &bit) && bit < NCAPINTS*32)
> > +	if (get_option(&arg, &bit) && bit >= 0 && bit < NCAPINTS * 32)
> >  		setup_clear_cpu_cap(bit);
> >  	else
> >  		return 0;
> > -- 
> 
> Yap, that's a good catch!
> 
> Acked-by: Borislav Petkov <bp@suse.de>
> 
> I even got a splat while experimenting with this:
> 
> 
> [    1.234575] BUG: unable to handle kernel paging request at ffffffff858bd540
> [    1.236535] IP: memcpy_erms+0x6/0x10

Good one, queued it up.

Btw., another (separate) fix would be to keep the kernel's option filtering code 
from being passive aggressive:

        if (get_option(&arg, &bit) && bit >= 0 && bit < NCAPINTS * 32)
                setup_clear_cpu_cap(bit);
        else
                return 0;

When we don't accept the value we should at least inform the user (via a printk 
that includes the 'clearcpuid' token in its message) that we totally ignored 
whatever he wanted. Something like:

	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.

Thanks,

	Ingo

[tip:x86/urgent] x86/cpu: Fix bootup crashes by sanitizing the argument of the 'clearcpuid=' command-line option

[tip-bot for Lukasz Odzioba]

Commit-ID:  dd853fd216d1485ed3045ff772079cc8689a9a4a
Gitweb:     http://git.kernel.org/tip/dd853fd216d1485ed3045ff772079cc8689a9a4a
Author:     Lukasz Odzioba <lukasz.odzioba@intel.com>
AuthorDate: Wed, 28 Dec 2016 14:55:40 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 5 Jan 2017 08:54:34 +0100

x86/cpu: Fix bootup crashes by sanitizing the argument of the 'clearcpuid=' command-line option

A negative number can be specified in the cmdline which will be used as
setup_clear_cpu_cap() argument. With that we can clear/set some bit in
memory predceeding boot_cpu_data/cpu_caps_cleared which may cause kernel
to misbehave. This patch adds lower bound check to setup_disablecpuid().

Boris Petkov reproduced a crash:

  [    1.234575] BUG: unable to handle kernel paging request at ffffffff858bd540
  [    1.236535] IP: memcpy_erms+0x6/0x10

Signed-off-by: Lukasz Odzioba <lukasz.odzioba@intel.com>
Acked-by: Borislav Petkov <bp@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: andi.kleen@intel.com
Cc: bp@alien8.de
Cc: dave.hansen@linux.intel.com
Cc: luto@kernel.org
Cc: slaoub@gmail.com
Fixes: ac72e7888a61 ("x86: add generic clearcpuid=... option")
Link: http://lkml.kernel.org/r/1482933340-11857-1-git-send-email-lukasz.odzioba@intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index dc1697c..9bab7a8 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1221,7 +1221,7 @@ static __init int setup_disablecpuid(char *arg)
 {
 	int bit;
 
-	if (get_option(&arg, &bit) && bit < NCAPINTS*32)
+	if (get_option(&arg, &bit) && bit >= 0 && bit < NCAPINTS * 32)
 		setup_clear_cpu_cap(bit);
 	else
 		return 0;

RE: [PATCH 1/1] x86: sanitize argument of clearcpuid command-line option

[Odzioba, Lukasz]

On Thursday, January 5, 2017 8:56 AM, Ingo Molnar wrote:
>
> Good one, queued it up.

Hi Ingo, thanks for picking up the patch.

> When we don't accept the value we should at least inform the user (via a printk 
> that includes the 'clearcpuid' token in its message) that we totally ignored 
> whatever he wanted. Something like:
> 
>	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);
+                       }

Thanks,
Lukas

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

[Ingo Molnar]

* Odzioba, Lukasz <lukasz.odzioba@intel.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

RE: [PATCH 1/1] x86: sanitize argument of clearcpuid command-line option

[Odzioba, Lukasz]

On Tuesday, January 17, 2017 8:18 AM, Ingo Molnar wrote: 
> Mind sending this as a proper patch, with akpm Cc:-ed?

Not at all, I'll put it together this week.

Thanks,
Lukas