| 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
| ||
|
Message-ID: <20151005131034.GA27954@gmail.com>
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