From: Ingo Molnar <mingo@kernel.org> To: kernel test robot <ying.huang@linux.intel.com> Cc: lkp@01.org, LKML <linux-kernel@vger.kernel.org>, Thomas Gleixner <tglx@linutronix.de>, Peter Zijlstra <peterz@infradead.org>, Andrew Morton <akpm@linux-foundation.org>, Linus Torvalds <torvalds@linux-foundation.org>, 0day robot <fengguang.wu@intel.com> Subject: Re: [lkp] [string] 5f6f0801f5: BUG: KASan: out of bounds access in strlcpy+0xc8/0x250 at addr ffff88011a666ee0 Date: Mon, 12 Oct 2015 09:33:55 +0200 [thread overview] Message-ID: <20151012073355.GA16543@gmail.com> (raw) In-Reply-To: <87612cinlg.fsf@yhuang-dev.intel.com> * kernel test robot <ying.huang@linux.intel.com> wrote: > FYI, we noticed the below changes on > > git://internal_mailing_list_patch_tree Ingo-Molnar/string-Improve-the-generic-strlcpy-implementation > commit 5f6f0801f5fdfce4984c6a14f99dbfbb417acb66 ("string: Improve the generic strlcpy() implementation") Hm, there's no such commit ID anywhere I can see - did you rebase my tree perhaps? I am guessing that you rebased the attached WIP commit I have in -tip (not permanently committed), which bases strlcpy() off strscpy() and through which strscpy() gains a couple of hundred usage sites: +size_t strlcpy(char *dst, const char *src, size_t dst_size) +{ + int 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); Now depending on what tree you tested it on, this KASAN report might either be a known and meanwhile strscpy() bug - or might perhaps be something new! The old, known fix is: 990486c8af04 strscpy: zero any trailing garbage bytes in the destination > [ 22.205482] systemd[1]: RTC configured in localtime, applying delta of 480 minutes to system time. > [ 22.214569] random: systemd urandom read with 11 bits of entropy available > [ 22.241378] ================================================================== > [ 22.242067] BUG: KASan: out of bounds access in strlcpy+0xc8/0x250 at addr ffff88011a666ee0 > [ 22.242067] Read of size 8 by task systemd/1 > [ 22.242067] ============================================================================= > [ 22.242067] BUG kmalloc-64 (Not tainted): kasan: bad access detected > [ 22.242067] ----------------------------------------------------------------------------- > [ 22.242067] > [ 22.242067] Disabling lock debugging due to kernel taint > [ 22.242067] INFO: Slab 0xffffea0004699980 objects=64 used=64 fp=0x (null) flags=0x200000000000080 > [ 22.242067] INFO: Object 0xffff88011a666ec0 @offset=3776 fp=0x7379732f62696c2f > [ 22.242067] > [ 22.242067] Bytes b4 ffff88011a666eb0: 00 00 00 00 00 00 00 00 a7 4b c2 ef 07 00 00 00 .........K...... > [ 22.242067] Object ffff88011a666ec0: 2f 6c 69 62 2f 73 79 73 74 65 6d 64 2f 73 79 73 /lib/systemd/sys Is there any stack trace of this bad access? The lack of stack trace and the unknown commit ID make it really hard to analyze this bug. Thanks, Ingo ====================> >From 53ef1538dfe8d9ed57676c567efd0d551d0a3255 Mon Sep 17 00:00:00 2001 From: Ingo Molnar <mingo@kernel.org> Date: Mon, 5 Oct 2015 10:56:50 +0200 Subject: [PATCH] string: Improve the generic strlcpy() implementation The current strlcpy() implementation has two implementational weaknesses: 1) There's a (largely theoretical) race: 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; } If another CPU or an interrupt changes the source string after the strlen(), but before the copy is complete, and shortens the source string, then we copy over the NUL byte of the source buffer - including fragments of earlier source string tails. The target buffer will still be properly NUL terminated - but it will be a shorter string than the returned 'ret' source buffer length. (despite there not being truncation.) The s390 arch implementation has the same race AFAICS. This may cause bugs if the return code is subsequently used to assume that it is equal to the destination string's length. (While in reality it's shorter.) The race is not automatically lethal, because it's guaranteed that the returned length is indeed zero-delimited (due to the overlong copy we did) - so if the string is memcpy()-ed, then it will still result in a weirdly padded but valid string. But if any subsequent use of the return code relies on the return code being equal to a subsequent call of strlen(dest), then that use might lead to bugs. I.e. our implementation of strlcpy() is indeed racy and unrobust. But we can fix this race: by basing strlcpy() on the newly introduced strscpy() API we iterate over the string in a single go and determine the length and copy the string at once. Like strscpy(), but with strlcpy() return semantics. This also makes strlcpy() faster. 2) Another problem is that strlcpy() will also happily do bad stuff if we pass it a negative size. Instead of that we will from now on print a (one time) warning (by virtue of strscpy()'s overflow checking) and return. Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: linux-kernel@vger.kernel.org Signed-off-by: Ingo Molnar <mingo@kernel.org> --- lib/string.c | 51 +++++++++++++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/lib/string.c b/lib/string.c index 96970f8a04eb..15f41de4a1b3 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 @@ -235,6 +209,31 @@ 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 = 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
WARNING: multiple messages have this Message-ID (diff)
From: Ingo Molnar <mingo@kernel.org> To: lkp@lists.01.org Subject: Re: [string] 5f6f0801f5: BUG: KASan: out of bounds access in strlcpy+0xc8/0x250 at addr ffff88011a666ee0 Date: Mon, 12 Oct 2015 09:33:55 +0200 [thread overview] Message-ID: <20151012073355.GA16543@gmail.com> (raw) In-Reply-To: <87612cinlg.fsf@yhuang-dev.intel.com> [-- Attachment #1: Type: text/plain, Size: 7210 bytes --] * kernel test robot <ying.huang@linux.intel.com> wrote: > FYI, we noticed the below changes on > > git://internal_mailing_list_patch_tree Ingo-Molnar/string-Improve-the-generic-strlcpy-implementation > commit 5f6f0801f5fdfce4984c6a14f99dbfbb417acb66 ("string: Improve the generic strlcpy() implementation") Hm, there's no such commit ID anywhere I can see - did you rebase my tree perhaps? I am guessing that you rebased the attached WIP commit I have in -tip (not permanently committed), which bases strlcpy() off strscpy() and through which strscpy() gains a couple of hundred usage sites: +size_t strlcpy(char *dst, const char *src, size_t dst_size) +{ + int 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); Now depending on what tree you tested it on, this KASAN report might either be a known and meanwhile strscpy() bug - or might perhaps be something new! The old, known fix is: 990486c8af04 strscpy: zero any trailing garbage bytes in the destination > [ 22.205482] systemd[1]: RTC configured in localtime, applying delta of 480 minutes to system time. > [ 22.214569] random: systemd urandom read with 11 bits of entropy available > [ 22.241378] ================================================================== > [ 22.242067] BUG: KASan: out of bounds access in strlcpy+0xc8/0x250 at addr ffff88011a666ee0 > [ 22.242067] Read of size 8 by task systemd/1 > [ 22.242067] ============================================================================= > [ 22.242067] BUG kmalloc-64 (Not tainted): kasan: bad access detected > [ 22.242067] ----------------------------------------------------------------------------- > [ 22.242067] > [ 22.242067] Disabling lock debugging due to kernel taint > [ 22.242067] INFO: Slab 0xffffea0004699980 objects=64 used=64 fp=0x (null) flags=0x200000000000080 > [ 22.242067] INFO: Object 0xffff88011a666ec0 @offset=3776 fp=0x7379732f62696c2f > [ 22.242067] > [ 22.242067] Bytes b4 ffff88011a666eb0: 00 00 00 00 00 00 00 00 a7 4b c2 ef 07 00 00 00 .........K...... > [ 22.242067] Object ffff88011a666ec0: 2f 6c 69 62 2f 73 79 73 74 65 6d 64 2f 73 79 73 /lib/systemd/sys Is there any stack trace of this bad access? The lack of stack trace and the unknown commit ID make it really hard to analyze this bug. Thanks, Ingo ====================> >From 53ef1538dfe8d9ed57676c567efd0d551d0a3255 Mon Sep 17 00:00:00 2001 From: Ingo Molnar <mingo@kernel.org> Date: Mon, 5 Oct 2015 10:56:50 +0200 Subject: [PATCH] string: Improve the generic strlcpy() implementation The current strlcpy() implementation has two implementational weaknesses: 1) There's a (largely theoretical) race: 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; } If another CPU or an interrupt changes the source string after the strlen(), but before the copy is complete, and shortens the source string, then we copy over the NUL byte of the source buffer - including fragments of earlier source string tails. The target buffer will still be properly NUL terminated - but it will be a shorter string than the returned 'ret' source buffer length. (despite there not being truncation.) The s390 arch implementation has the same race AFAICS. This may cause bugs if the return code is subsequently used to assume that it is equal to the destination string's length. (While in reality it's shorter.) The race is not automatically lethal, because it's guaranteed that the returned length is indeed zero-delimited (due to the overlong copy we did) - so if the string is memcpy()-ed, then it will still result in a weirdly padded but valid string. But if any subsequent use of the return code relies on the return code being equal to a subsequent call of strlen(dest), then that use might lead to bugs. I.e. our implementation of strlcpy() is indeed racy and unrobust. But we can fix this race: by basing strlcpy() on the newly introduced strscpy() API we iterate over the string in a single go and determine the length and copy the string at once. Like strscpy(), but with strlcpy() return semantics. This also makes strlcpy() faster. 2) Another problem is that strlcpy() will also happily do bad stuff if we pass it a negative size. Instead of that we will from now on print a (one time) warning (by virtue of strscpy()'s overflow checking) and return. Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: linux-kernel(a)vger.kernel.org Signed-off-by: Ingo Molnar <mingo@kernel.org> --- lib/string.c | 51 +++++++++++++++++++++++++-------------------------- 1 file changed, 25 insertions(+), 26 deletions(-) diff --git a/lib/string.c b/lib/string.c index 96970f8a04eb..15f41de4a1b3 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 @@ -235,6 +209,31 @@ 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 = 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
next prev parent reply other threads:[~2015-10-12 7:34 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-10-12 6:54 [lkp] [string] 5f6f0801f5: BUG: KASan: out of bounds access in strlcpy+0xc8/0x250 at addr ffff88011a666ee0 kernel test robot 2015-10-12 6:54 ` kernel test robot 2015-10-12 7:33 ` Ingo Molnar [this message] 2015-10-12 7:33 ` Ingo Molnar 2015-10-12 7:43 ` [LKP] [lkp] " Huang, Ying 2015-10-12 7:43 ` Huang, Ying 2015-10-12 8:13 ` [LKP] [lkp] " Ingo Molnar 2015-10-12 8:13 ` Ingo Molnar 2015-10-12 7:51 ` [LKP] [lkp] " Fengguang Wu 2015-10-12 7:51 ` Fengguang Wu 2015-10-12 7:58 ` [LKP] [lkp] " Fengguang Wu 2015-10-12 7:58 ` Fengguang Wu 2015-10-12 8:17 ` [LKP] [lkp] " Ingo Molnar 2015-10-12 8:17 ` Ingo Molnar 2015-10-12 8:34 ` [LKP] [lkp] " Fengguang Wu 2015-10-12 8:34 ` Fengguang Wu 2015-10-12 8:19 ` [LKP] [lkp] " Fengguang Wu 2015-10-12 8:19 ` Fengguang Wu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20151012073355.GA16543@gmail.com \ --to=mingo@kernel.org \ --cc=akpm@linux-foundation.org \ --cc=fengguang.wu@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=lkp@01.org \ --cc=peterz@infradead.org \ --cc=tglx@linutronix.de \ --cc=torvalds@linux-foundation.org \ --cc=ying.huang@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.