From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751430AbbJLHnY (ORCPT ); Mon, 12 Oct 2015 03:43:24 -0400 Received: from mga03.intel.com ([134.134.136.65]:13755 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751116AbbJLHnW (ORCPT ); Mon, 12 Oct 2015 03:43:22 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,671,1437462000"; d="scan'208";a="578773142" From: "Huang\, Ying" To: Ingo Molnar Cc: Peter Zijlstra , , LKML , Thomas Gleixner , "0day robot" , Linus Torvalds , Andrew Morton Subject: Re: [LKP] [lkp] [string] 5f6f0801f5: BUG: KASan: out of bounds access in strlcpy+0xc8/0x250 at addr ffff88011a666ee0 References: <87612cinlg.fsf@yhuang-dev.intel.com> <20151012073355.GA16543@gmail.com> Date: Mon, 12 Oct 2015 15:43:19 +0800 In-Reply-To: <20151012073355.GA16543@gmail.com> (Ingo Molnar's message of "Mon, 12 Oct 2015 09:33:55 +0200") Message-ID: <87y4f88rc8.fsf@yhuang-dev.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Ingo Molnar writes: > * kernel test robot 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? The test is for patch from LKML instead of git tree. That is, you patch is tested via applying it to a -rc kernel. Do you have a commit in your tree for this? We can test that to confirm. > 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? There is dmesg file attached in the original report email. The stack trace is as follow, [ 22.242067] Call Trace: [ 22.242067] [] dump_stack+0x4e/0x7d [ 22.242067] [] print_trailer+0xf8/0x150 [ 22.242067] [] object_err+0x31/0x40 [ 22.242067] [] kasan_report_error+0x1e5/0x3f0 [ 22.242067] [] ? anon_vma_interval_tree_insert+0x123/0x140 [ 22.242067] [] kasan_report+0x34/0x40 [ 22.242067] [] ? strlcpy+0xc8/0x250 [ 22.242067] [] __asan_load8+0x64/0xa0 [ 22.242067] [] strlcpy+0xc8/0x250 [ 22.242067] [] cgroup_release_agent_write+0x67/0xa0 [ 22.242067] [] cgroup_file_write+0x75/0x180 [ 22.242067] [] ? cgroup_init_cftypes+0x160/0x160 [ 22.242067] [] kernfs_fop_write+0x17e/0x210 [ 22.242067] [] __vfs_write+0x57/0x170 [ 22.242067] [] ? preempt_count_sub+0x13/0xe0 [ 22.242067] [] ? update_fast_ctr+0x51/0x80 [ 22.242067] [] vfs_write+0xeb/0x240 [ 22.242067] [] SyS_write+0x53/0xb0 [ 22.242067] [] entry_SYSCALL_64_fastpath+0x16/0x75 Best Regards, Huang, Ying > 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 > 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 > Cc: Andrew Morton > Cc: Peter Zijlstra > Cc: Thomas Gleixner > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Ingo Molnar > --- > 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 > _______________________________________________ > LKP mailing list > LKP@lists.01.org > https://lists.01.org/mailman/listinfo/lkp