From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752325AbcAFRKm (ORCPT ); Wed, 6 Jan 2016 12:10:42 -0500 Received: from mx2.suse.de ([195.135.220.15]:36706 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751761AbcAFRKj (ORCPT ); Wed, 6 Jan 2016 12:10:39 -0500 Date: Wed, 6 Jan 2016 18:10:32 +0100 From: Borislav Petkov To: Dave Hansen Cc: x86@kernel.org, linux-kernel@vger.kernel.org, dave.hansen@linux.intel.com, hpa@zytor.com, fenghua.yu@intel.com, yu-cheng.yu@intel.com Subject: Re: [PATCH 1/5] x86: fix early command-line parsing when matching at end Message-ID: <20160106171032.GA20321@pd.tnic> References: <20151222225237.08CDE5F1@viggo.jf.intel.com> <20151222225238.9AEB560C@viggo.jf.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20151222225238.9AEB560C@viggo.jf.intel.com> 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 On Tue, Dec 22, 2015 at 02:52:38PM -0800, Dave Hansen wrote: > > From: Dave Hansen > > The x86 early command line parsing in cmdline_find_option_bool() > is buggy. If it matches a specified 'option' all the way to > the end of the command-line, it will consider it a match. > > For instance, > > cmdline = "foo"; > cmdline_find_option_bool(cmdline, "fool"); > > will return 1. This is particularly annoying since we have > actual FPU options like "noxsave" and "noxsaves" So, > command-line "foo bar noxsave" will match *BOTH* a "noxsave" and > "noxsaves". (This turns out not to be an actual problem because > "noxsave" implies "noxsaves", but it's still confusing.) > > To fix this, we simplify the code and stop tracking 'len'. 'len' > was trying to indicate either the NULL terminator *OR* the end of > a non-NULL-terminated commandline at 'COMMAND_LINE_SIZE'. But, > each of the three states is *already* checking 'cmdline' for a > NULL terminator. > > We _only_ need to check if we have overrun 'COMMAND_LINE_SIZE', > and that we can do without keeping 'len' around. > > Also add some commends to clarify what is going on. > > Signed-off-by: Dave Hansen > Cc: Borislav Petkov > Cc: H. Peter Anvin > Cc: linux-kernel@vger.kernel.org > Cc: fenghua.yu@intel.com > Cc: yu-cheng.yu@intel.com > --- > > b/arch/x86/lib/cmdline.c | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff -puN arch/x86/lib/cmdline.c~x86-broken-end-of-string-command-line-parsing arch/x86/lib/cmdline.c > --- a/arch/x86/lib/cmdline.c~x86-broken-end-of-string-command-line-parsing 2015-12-22 11:56:58.639149442 -0800 > +++ b/arch/x86/lib/cmdline.c 2015-12-22 11:56:58.642149577 -0800 > @@ -21,12 +21,14 @@ static inline int myisspace(u8 c) > * @option: option string to look for > * > * Returns the position of that @option (starts counting with 1) > - * or 0 on not found. > + * or 0 on not found. @option will only be found if it is found > + * as an entire word in @cmdline. For instance, if @option="car" > + * then a cmdline which contains "cart" will not match. > */ > int cmdline_find_option_bool(const char *cmdline, const char *option) > { > char c; > - int len, pos = 0, wstart = 0; > + int pos = 0, wstart = 0; > const char *opptr = NULL; > enum { > st_wordstart = 0, /* Start of word/after whitespace */ > @@ -37,11 +39,14 @@ int cmdline_find_option_bool(const char > if (!cmdline) > return -1; /* No command line */ > > - len = min_t(int, strlen(cmdline), COMMAND_LINE_SIZE); > - if (!len) > + if (!strlen(cmdline)) > return 0; > > - while (len--) { > + /* > + * This 'pos' check ensures we do not overrun > + * a non-NULL-terminated 'cmdline' > + */ > + while (pos < COMMAND_LINE_SIZE) { So this is a little bit strange: you're checking strlen(cmdline) above but iterating until COMMAND_LINE_SIZE. I think we should make it even more palatable: while (pos < min_t(int, strlen(cmdline), COMMAND_LINE_SIZE)) so that it is obvious how far we iterate. I know, I know, it boils down to the same code because each state is checking !c but this way it is clearer what we're doing when someone looks at that code again. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --