linux-kernel - Re: [PATCH] params: Fix an overflow in param_attr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 28 Sep 2017 10:02:23 +0200
From:   Jean Delvare <jdelvare@...e.de>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Baoquan He <bhe@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        "H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH] params: Fix an overflow in param_attr_show

On Wed, 27 Sep 2017 15:31:04 +0200, Ingo Molnar wrote:
> * Jean Delvare <jdelvare@...e.de> wrote:
> > > So the \n additions to the STANDARD_PARAM_DEF() lines
> > >   
> > > > +STANDARD_PARAM_DEF(byte, unsigned char, "%hhu\n", kstrtou8);
> > > > +STANDARD_PARAM_DEF(short, short, "%hi\n", kstrtos16);
> > > > +STANDARD_PARAM_DEF(ushort, unsigned short, "%hu\n", kstrtou16);  
> > > 
> > > are not necessary anymore, with the other changes? If so then I'd leave them 
> > > without the \n - that's also easier to read.  
> > 
> > What other changes are you referring to? I'm confused. Are you sure you
> > read the patch entirely before commenting on it?  
> 
> I was referring to the rest of the patch, which avoids the overflow even if the \n 
> is not present in the pattern.

You make things sound complex, when my patch is so simple. I'm simply
changing the point at which the trailing \n is added. The \n must be
present in the pattern, so "even if the \n is not present in the
pattern" was out of scope. And it turns out that it doesn't matter at
all anyway.

> (...)
> So what I was asking, what happens if someone adds a new entry and
> forgets the \n?
> 
> This is not hypothetical - for example this commit:
> 
>   b4210b810e50 ("Add module param type 'ullong'")
> 
> ... added a new entry for a new param type. It's entirely possible for
> new additions to happen here.
> (...)
> At minimum I'd suggest aligning the definitions vertically, to make sure
> any missing \n stands out more, visually:
> 
> STANDARD_PARAM_DEF(byte,	unsigned char,		"%hhu\n",	kstrtou8);
> STANDARD_PARAM_DEF(short,	short,			"%hi\n",	kstrtos16);
> STANDARD_PARAM_DEF(ushort,	unsigned short,		"%hu\n",	kstrtou16);
> STANDARD_PARAM_DEF(int,	int,			"%i\n",		kstrtoint);
> STANDARD_PARAM_DEF(uint,	unsigned int,		"%u\n",		kstrtouint);
> STANDARD_PARAM_DEF(long,	long,			"%li\n",	kstrtol);
> STANDARD_PARAM_DEF(ulong,	unsigned long,		"%lu\n",	kstrtoul);
> STANDARD_PARAM_DEF(ullong,	unsigned long long,	"%llu\n",	kstrtoull);

Sure it is possible to add a new parameter type. But why would the
person adding it forget the \n? I can't imagine that someone adding a
new type would type the new line of code character by character. Such an
operation is calling for copy, paste and edit, at which point there is
no reason why the \n would be actively deleted. Or this is sabotage,
really ;-)

Aligning parameters vertically as you suggest above is probably a good
idea for overall readability anyway, so I can change my patch to do
that, as I am modifying these lines anyway. It is pretty much
independent from the fix per se, but if it makes you happy...

-- 
Jean Delvare
SUSE L3 Support