From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751857AbbJLINm (ORCPT ); Mon, 12 Oct 2015 04:13:42 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:36492 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751398AbbJLINj (ORCPT ); Mon, 12 Oct 2015 04:13:39 -0400 Date: Mon, 12 Oct 2015 10:13:35 +0200 From: Ingo Molnar To: "Huang, Ying" Cc: Peter Zijlstra , lkp@01.org, 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 Message-ID: <20151012081334.GA23595@gmail.com> References: <87612cinlg.fsf@yhuang-dev.intel.com> <20151012073355.GA16543@gmail.com> <87y4f88rc8.fsf@yhuang-dev.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87y4f88rc8.fsf@yhuang-dev.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Huang, Ying wrote: > 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. Yeah, I just made a merge that includes just to strscpy() related bits: b94371b0917a Merge tag 'v4.3-rc5' into core/strings, to pick up strscpy() fixes (Note, it might take a few minutes for korg git mirrors to pick up this merge.) All that tree does is that it makes strlcpy() use strscpy(): > > +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); Do you see the same KASAN failure with that commit? If my analysis below is correct, then the failure should go away. Analysis: The stack dump: [ 22.242067] [] strlcpy+0xc8/0x250 [ 22.242067] [] cgroup_release_agent_write+0x67/0xa0 [ 22.242067] [] cgroup_file_write+0x75/0x180 [ 22.242067] [] kernfs_fop_write+0x17e/0x210 [ 22.242067] [] __vfs_write+0x57/0x170 [ 22.242067] [] vfs_write+0xeb/0x240 Implicates this strlcpy(): spin_lock(&release_agent_path_lock); strlcpy(cgrp->root->release_agent_path, strstrip(buf), sizeof(cgrp->root->release_agent_path)); spin_unlock(&release_agent_path_lock); where: include/linux/cgroup-defs.h: char release_agent_path[PATH_MAX]; the target buffer sizing looks pretty robust (because simple). And the input buffer side looks safe as well, by my reading: 'buf' here seems like a regular write operation, with a 'buf' and a 'size' parameter - layered in through various layers of abstraction: struct cftype::write, used in kernel/cgroup.c: cgroup_file_write() - no size checks, no guarantee that we have a string this is called via: struct kernfs_ops::write via kernfs which guarantees string termination in fs/kernfs/file.c's kernfs_fop_write(): buf[len] = '\0'; /* guarantee string termination */ ops = kernfs_ops(of->kn); if (ops->write) len = ops->write(of, buf, len, *ppos); and that's a stable, private string local to the calling task. So my guess is that this is the bug that got fixed by: 990486c8af04 ("strscpy: zero any trailing garbage bytes in the destination") that that systemd passed in a string with leading whitespace, thus strtrim() created an unaligned string, which caused the strscpy() to access past the end of the kmalloc() buffer. Thanks, Ingo