[PATCH 1/2] Revert "xattr: mark variable as uninitialized to make both gcc and smatch happy"

* [PATCH 1/2] Revert "xattr: mark variable as uninitialized to make both gcc and smatch happy"
@ 2012-09-14 19:35 Sasha Levin
  2012-09-14 19:35 ` [PATCH 2/2] xattr: prevent NULL ptr deref warnings in __simple_xattr_set Sasha Levin
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Sasha Levin @ 2012-09-14 19:35 UTC (permalink / raw)
  To: tj; +Cc: aris, dan.carpenter, fengguang.wu, linux-kernel, Sasha Levin

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@gmail.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


^ permalink raw reply related	[flat|nested] 9+ messages in thread