From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965717AbdAEH4M (ORCPT ); Thu, 5 Jan 2017 02:56:12 -0500 Received: from mail-wj0-f195.google.com ([209.85.210.195]:33769 "EHLO mail-wj0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S938138AbdAEH4E (ORCPT ); Thu, 5 Jan 2017 02:56:04 -0500 Date: Thu, 5 Jan 2017 08:55:59 +0100 From: Ingo Molnar To: Borislav Petkov Cc: Lukasz Odzioba , linux-kernel@vger.kernel.org, tglx@linutronix.de, mingo@redhat.com, hpa@zytor.com, x86@kernel.org, luto@kernel.org, slaoub@gmail.com, bp@suse.de, dave.hansen@linux.intel.com, andi.kleen@intel.com Subject: Re: [PATCH 1/1] x86: sanitize argument of clearcpuid command-line option Message-ID: <20170105075559.GA2098@gmail.com> References: <1482933340-11857-1-git-send-email-lukasz.odzioba@intel.com> <20161229102105.GD11221@nazgul.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161229102105.GD11221@nazgul.tnic> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Borislav Petkov 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 > > --- > > 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 > > 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