| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1347651354-16289-1-git-send-email-levinsasha928@gmail.com>
Date: Fri, 14 Sep 2012 21:35:53 +0200
From: Sasha Levin <levinsasha928@...il.com>
To: tj@...nel.org
Cc: aris@...hat.com, dan.carpenter@...cle.com, fengguang.wu@...el.com,
linux-kernel@...r.kernel.org, Sasha Levin <levinsasha928@...il.com>
Subject: [PATCH 1/2] Revert "xattr: mark variable as uninitialized to make both gcc and smatch happy"
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: [<ffffffff81296370>] __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:[<ffffffff81296370>] [<ffffffff81296370>] __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] [<ffffffff812964c3>] simple_xattr_remove+0x13/0x20
[ 19.450225] [<ffffffff81215359>] shmem_removexattr+0x59/0x70
[ 19.450225] [<ffffffff8194b7ed>] ima_inode_post_setattr+0xad/0xc0
[ 19.450225] [<ffffffff8128e47c>] notify_change+0x34c/0x3b0
[ 19.450225] [<ffffffff8126e4d4>] do_truncate+0x64/0xa0
[ 19.450225] [<ffffffff8126e63e>] sys_truncate+0x12e/0x1c0
[ 19.450225] [<ffffffff8374b685>] ? tracesys+0x7e/0xe6
[ 19.450225] [<ffffffff8374b6e8>] 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 [<ffffffff81296370>] __simple_xattr_set+0xf0/0x140
[ 19.450225] RSP <ffff88002742ddb8>
[ 19.450225] CR2: 0000000000000010
Signed-off-by: Sasha Levin <levinsasha928@...il.com>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists