Messages in this thread | | | Date | Wed, 6 Jan 2016 18:10:32 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH 1/5] x86: fix early command-line parsing when matching at end |
| |
On Tue, Dec 22, 2015 at 02:52:38PM -0800, Dave Hansen wrote: > > From: Dave Hansen <dave.hansen@linux.intel.com> > > 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 <dave.hansen@linux.intel.com> > Cc: Borislav Petkov <bp@suse.de> > Cc: H. Peter Anvin <hpa@zytor.com> > 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) --
| |