From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932401Ab2INTfk (ORCPT ); Fri, 14 Sep 2012 15:35:40 -0400 Received: from mail-wi0-f178.google.com ([209.85.212.178]:39856 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932327Ab2INTfi (ORCPT ); Fri, 14 Sep 2012 15:35:38 -0400 From: Sasha Levin To: tj@kernel.org Cc: aris@redhat.com, dan.carpenter@oracle.com, fengguang.wu@intel.com, linux-kernel@vger.kernel.org, Sasha Levin Subject: [PATCH 1/2] Revert "xattr: mark variable as uninitialized to make both gcc and smatch happy" Date: Fri, 14 Sep 2012 21:35:53 +0200 Message-Id: <1347651354-16289-1-git-send-email-levinsasha928@gmail.com> X-Mailer: git-send-email 1.7.12 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This reverts commit 0142145ddb1d6c841be4eae2c7a32dd18ad34b24. Short version: Not initializing 'new_xattr' at the beginning of __simple_xattr_set() may lead to dereferencing it later on in the function. Long version: The fix for the warnings generated by smatch due to 'new_xattr' being dereferenced without a check from being non-NULL is incorrect. The problem is that the fix removed initialization of new_xattr with NULL, which meant that new_xattr could be anything at the beginning of __simple_xattr_set(), and might have not been initialized at any point throughout the function. In case new_xattr does get left uninitialized ('value == 0' case) and XATTR_REPLACE being set, the fix will actually lead us to dereferencing new_xattr even if we wouldn't have done so before. Why? Looking at the original code: if (flags & XATTR_REPLACE) { xattr = new_xattr; err = -ENODATA; } else if (new_xattr) { list_add(&new_xattr->list, &xattrs->head); xattr = NULL; } out: spin_unlock(&xattrs->lock); if (xattr) { kfree(xattr->name); kfree(xattr); } return err; We see that in the case of XATTR_REPLACE, we will assign new_xattr to xattr. The problem is that due to optimizations by gcc we will actually NULL deref xattr at the 'out:' exit label even after checking that it's not NULL. gcc will optimize the code after the fix to look (roughly) as follows: if (flags & XATTR_REPLACE) { xattr = new_xattr; err = -ENODATA; } else if (new_xattr) { list_add(&new_xattr->list, &xattrs->head); spin_unlock(&xattrs->lock); return err; } out: spin_unlock(&xattrs->lock); kfree(xattr->name); kfree(xattr); return err; Since 'xattr = new_xattr' assignment can no longer result in NULL, the NULL check in the out label was moved to the 'else if', and xattr will get dereferenced even though new_xattr may have been NULL. To confirm the story above, we can easily trigger that by triggering __simple_xattr_set() from a shmem mount: [ 19.397907] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 [ 19.398034] IP: [] __simple_xattr_set+0xf0/0x140 [ 19.398034] PGD 2741f067 PUD 2742e067 PMD 0 [ 19.398034] Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC [ 19.398034] Dumping ftrace buffer: [ 19.398034] (ftrace buffer empty) [ 19.398034] CPU 0 [ 19.410800] Pid: 5913, comm: nomnom Tainted: G W 3.6.0-rc5-next-20120914-sasha-00001-ge76e16e-dirty #332 [ 19.410853] RIP: 0010:[] [] __simple_xattr_set+0xf0/0x140 [ 19.416311] RSP: 0018:ffff88002742ddb8 EFLAGS: 00010296 [ 19.416311] RAX: 0000000000080000 RBX: ffff880019c42308 RCX: 0000000000000000 [ 19.416311] RDX: 0000000000000004 RSI: 00000000004ea000 RDI: 0000000000000001 [ 19.416311] RBP: ffff88002742ddf8 R08: 0000000000000000 R09: ffff8800274308f0 [ 19.416311] R10: 0000000000000000 R11: 0000000000000000 R12: 00000000ffffffc3 [ 19.416311] R13: 0000000000000000 R14: ffff880019c42308 R15: 0000000000000000 [ 19.416311] FS: 00007facea183700(0000) GS:ffff88000d800000(0000) knlGS:0000000000000000 [ 19.416311] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 19.416311] CR2: 0000000000000010 CR3: 000000002741e000 CR4: 00000000000406f0 [ 19.416311] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 [ 19.416311] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 [ 19.444594] Process nomnom (pid: 5913, threadinfo ffff88002742c000, task ffff880027430000) [ 19.444594] Stack: [ 19.450225] 0000000200000000 ffff880019c42318 ffff880019c42360 ffffffff84338738 [ 19.450225] ffff880019c42360 0000000000000000 ffff880019c42360 0000000000000000 [ 19.450225] ffff88002742de08 ffffffff812964c3 ffff88002742de28 ffffffff81215359 [ 19.450225] Call Trace: [ 19.450225] [] simple_xattr_remove+0x13/0x20 [ 19.450225] [] shmem_removexattr+0x59/0x70 [ 19.450225] [] ima_inode_post_setattr+0xad/0xc0 [ 19.450225] [] notify_change+0x34c/0x3b0 [ 19.450225] [] do_truncate+0x64/0xa0 [ 19.450225] [] sys_truncate+0x12e/0x1c0 [ 19.450225] [] ? tracesys+0x7e/0xe6 [ 19.450225] [] tracesys+0xe1/0xe6 [ 19.450225] Code: fb 75 b8 f6 45 c4 02 41 bc c3 ff ff ff 75 42 4c 89 f2 48 89 de 4c 89 ef e8 8e 77 74 00 48 8b 7d c8 45 31 e4 e8 02 38 4b 02 eb 38 <49> 8b 7f 10 e8 57 f1 fb ff 4c 89 ff e8 4f f1 fb ff eb 25 41 bc [ 19.450225] RIP [] __simple_xattr_set+0xf0/0x140 [ 19.450225] RSP [ 19.450225] CR2: 0000000000000010 Signed-off-by: Sasha Levin --- fs/xattr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/xattr.c b/fs/xattr.c index f4516a9..508ec1d 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -847,7 +847,7 @@ static int __simple_xattr_set(struct simple_xattrs *xattrs, const char *name, const void *value, size_t size, int flags) { struct simple_xattr *xattr; - struct simple_xattr *uninitialized_var(new_xattr); + struct simple_xattr *new_xattr = NULL; int err = 0; /* value == NULL means remove */ -- 1.7.12