lkml.org 
[lkml]   [2017]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/1] x86: sanitize argument of clearcpuid command-line option

* 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

\
 
 \ /
  Last update: 2017-01-05 08:56    [W:0.094 / U:0.176 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site