* Borislav Petkov <b...@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.odzi...@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 <b...@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
