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
| ||
|
Date: Mon, 5 Oct 2015 15:10:34 +0200 From: Ingo Molnar <mingo@...nel.org> To: Linus Torvalds <torvalds@...ux-foundation.org> Cc: 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 * Linus Torvalds <torvalds@...ux-foundation.org> wrote: > On Mon, Oct 5, 2015 at 12:27 PM, Ingo Molnar <mingo@...nel.org> wrote: > > > > 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? > > Well, I'm not sure the race really matters. [...] Yeah, so if we do the word-by-word optimization for strlcpy() then the race is fixed 'automatically', for free. But you are right: > [...] I personally think strlcpy() is a horrible interface, and the thing is, > the return value of strlcpy (which is what can race) is kind of useless because > it's not actually the size of the resulting string *anyway* (because of the > overflow issue). > > So I'm not sure it's worth even fixing. > Also, if you do this, then you're better off using the (hopefully optimized) > "strlen()" for the tail part of the strlcpy destination for the overflow case > that didn't get copied. Indeed, this on top of my patch should do that: lib/string.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/string.c b/lib/string.c index e0cfca299606..dfd24b557e84 100644 --- a/lib/string.c +++ b/lib/string.c @@ -212,10 +212,7 @@ size_t strlcpy(char *dest, const char *src, size_t dest_size) if (dest_size) dest[dest_size-1] = '\0'; - while (src[src_len]) - src_len++; - - return src_len; + return strlen(src) + src_len; } EXPORT_SYMBOL(strlcpy); #endif (untested) > In other words, I think your patch is overly fragile and complex. Well, it's mostly a copy of strscpy() with obvious conversion of the return convention, but you are right to point out: > Instead, you might choose to implement strlcpy() in terms of > "strscpy()" and "strlen()". > > Something like > > int strlcpy(dst, src, len) > { > // do the actual copy > int n = strscpy(dst, src, len); > > // handle the insane and broken strlcpy overflow return value > if (n < 0) > return len + strlen(src+len); > > return n; > } > > but I didn't actually verify that the above is correct for all the corner case. Hm, so I considered doing that initially, but managed to convince myself that it's not equivalent: but on a second thought your variant should indeed work! > The point being, that you really shouldn't waste your time implementing the > broken BSD strlcpy crap as an actual first-class implementation. You're better > off just using a strscpy() as the primary engine, and then implementing the > broken strlcpy interfaces on top of it. > > Does the above work? I'd take a patch that implements that if it's tested and > somebody has thought about it a lot. But I don't like your patch that open-codes > the insane interface with complex and fragile code. So the below untested variant does that plus an overflow check - it's only FYI, not signed off yet. Thanks, Ingo ==============> Not-Signed-off-by: Ingo Molnar <mingo@...nel.org> lib/string.c | 60 ++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/lib/string.c b/lib/string.c index 8dbb7b1eab50..6b89c035df74 100644 --- a/lib/string.c +++ b/lib/string.c @@ -124,32 +124,6 @@ char *strncpy(char *dest, const char *src, size_t count) EXPORT_SYMBOL(strncpy); #endif -#ifndef __HAVE_ARCH_STRLCPY -/** - * strlcpy - Copy a C-string into a sized buffer - * @dest: Where to copy the string to - * @src: Where to copy the string from - * @size: size of destination buffer - * - * Compatible with *BSD: the result is always a valid - * NUL-terminated string that fits in the buffer (unless, - * of course, the buffer size is zero). It does not pad - * out the result like strncpy() does. - */ -size_t strlcpy(char *dest, const char *src, size_t size) -{ - size_t ret = strlen(src); - - if (size) { - size_t len = (ret >= size) ? size - 1 : ret; - memcpy(dest, src, len); - dest[len] = '\0'; - } - return ret; -} -EXPORT_SYMBOL(strlcpy); -#endif - #ifndef __HAVE_ARCH_STRSCPY /** * strscpy - Copy a C-string into a sized buffer @@ -234,6 +208,40 @@ ssize_t strscpy(char *dest, const char *src, size_t count) EXPORT_SYMBOL(strscpy); #endif +#ifndef __HAVE_ARCH_STRLCPY +/** + * strlcpy - Copy a C-string into a sized buffer + * @dst: Where to copy the string to + * @src: Where to copy the string from + * @dst_size: size of destination buffer + * + * Compatible with *BSD: the result is always a valid + * NUL-terminated string that fits in the buffer (unless, + * of course, the buffer size is zero). It does not pad + * out the result like strncpy() does. + */ +size_t strlcpy(char *dst, const char *src, size_t dst_size) +{ + int ret; + + /* Overflow check: */ + if (unlikely((ssize_t)dst_size < 0)) { + WARN_ONCE(1, "strlcpy(): dst_size < 0 underflow!"); + return strlen(src); + } + + ret = strscpy(dst, src, dst_size); + + /* Handle the insane and broken strlcpy() overflow return value: */ + if (ret < 0) + return dst_size + strlen(src+dst_size); + + return ret; +} +EXPORT_SYMBOL(strlcpy); +#endif + + #ifndef __HAVE_ARCH_STRCAT /** * strcat - Append one %NUL-terminated string to another -- 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/
Powered by blists - more mailing lists