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

[Ingo Molnar]

* Borislav Petkov <bp@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxx>
> 
> 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