linux-kernel - Re: [PATCH] string: Improve the generic strlcpy() implementation

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:	Tue, 6 Oct 2015 10:03:46 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Rasmus Villemoes <linux@...musvillemoes.dk>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	Chris Metcalf <cmetcalf@...hip.com>,
	open list <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH] string: Improve the generic strlcpy() implementation


* Rasmus Villemoes <linux@...musvillemoes.dk> wrote:

> On Mon, Oct 05 2015, Ingo Molnar <mingo@...nel.org> wrote:
> 
> > * Linus Torvalds <torvalds@...ux-foundation.org> wrote:
> >
> >> So I finally pulled it. I like the patch, I like the new interface, but 
> >> despite that I wasn't really sure if I wanted to pull it in - thus the long 
> >> delay of me just seeing this in my list of pending pulls for almost a month, 
> >> but never really getting to the point where I decided I want to commit to it.
> >
> > Interesting. I noticed that strscpy() says this in its comments:
> >
> >  * In addition, the implementation is robust to the string changing out
> >  * from underneath it, unlike the current strlcpy() implementation.
> >
> > The strscpy() interface is very nice, but shouldn't we also fix this strlcpy() 
> > unrobustness/race it refers to, in light of the 2000+ existing strlcpy() call 
> > sites?
> 
> How about every single occurence of %s in a format string? vsnprintf also has 
> that "issue", but has it actually ever been a problem? The window for something 
> bad to happen is probably also much larger in the printf case, and especially 
> when when some %p extension is used and/or the vsnprintf user is kasprintf() 
> (where we 'replay' the formatting, having hopefully obtained a correct-sized 
> buffer).
> 
> In fact, writing this, it occurs to me that we should probably check the return 
> value of the second vsnprintf call in kasprintf and compare to the first, 
> issuing a warning if they don't match.
> 
> I'm not against making strlcpy more robust, but I think the theoretical race is 
> far more likely to manifest through a member of the printf family.

So the printf family is generally less frequently used in ABI output than string 
copies, but yeah, since there are 15,000 s[n]printf() calls in the kernel it's 
more likely to be an issue not just by virtue of timing, but also by sheer mass of 
usage, statistically.

So I'd improve it all in the following order:

  - fix the strscpy() uninitialized use

  - base strlcpy() on strscpy() via the patch I sent. This makes all users faster 
    and eliminates the theoretical race.

  - phase out 'simple' strlcpy() uses via an automated patch. This gets rid of 
    2,000 strlcpy() call sites in a single pass.

  - phase out the remaining two dozen or so 'complex' strlcpy() uses one by one.

  - mark strlcpy() deprecated, add checkpatch warning.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/