Messages in this thread Patch in this message | | | From | "Huang\, Ying" <> | Subject | Re: [LKP] [lkp] [string] 5f6f0801f5: BUG: KASan: out of bounds access in strlcpy+0xc8/0x250 at addr ffff88011a666ee0 | Date | Mon, 12 Oct 2015 15:43:19 +0800 |
| |
Ingo Molnar <mingo@kernel.org> writes:
> * 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?
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] [<ffffffff8176e231>] dump_stack+0x4e/0x7d [ 22.242067] [<ffffffff81203c18>] print_trailer+0xf8/0x150 [ 22.242067] [<ffffffff812068c1>] object_err+0x31/0x40 [ 22.242067] [<ffffffff8120a6e5>] kasan_report_error+0x1e5/0x3f0 [ 22.242067] [<ffffffff811d9c63>] ? anon_vma_interval_tree_insert+0x123/0x140 [ 22.242067] [<ffffffff8120a9d4>] kasan_report+0x34/0x40 [ 22.242067] [<ffffffff8177a3e8>] ? strlcpy+0xc8/0x250 [ 22.242067] [<ffffffff81209ed4>] __asan_load8+0x64/0xa0 [ 22.242067] [<ffffffff8177a3e8>] strlcpy+0xc8/0x250 [ 22.242067] [<ffffffff8117b1b7>] cgroup_release_agent_write+0x67/0xa0 [ 22.242067] [<ffffffff81179925>] cgroup_file_write+0x75/0x180 [ 22.242067] [<ffffffff811798b0>] ? cgroup_init_cftypes+0x160/0x160 [ 22.242067] [<ffffffff812af81e>] kernfs_fop_write+0x17e/0x210 [ 22.242067] [<ffffffff8121cf67>] __vfs_write+0x57/0x170 [ 22.242067] [<ffffffff81117b73>] ? preempt_count_sub+0x13/0xe0 [ 22.242067] [<ffffffff8113c211>] ? update_fast_ctr+0x51/0x80 [ 22.242067] [<ffffffff8121d2bb>] vfs_write+0xeb/0x240 [ 22.242067] [<ffffffff8121d513>] SyS_write+0x53/0xb0 [ 22.242067] [<ffffffff8242f276>] 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 <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 > _______________________________________________ > LKP mailing list > LKP@lists.01.org > https://lists.01.org/mailman/listinfo/lkp
| |