Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751248AbaG1GSB (ORCPT ); Mon, 28 Jul 2014 02:18:01 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:58577 "EHLO mailout4.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750840AbaG1GR6 (ORCPT ); Mon, 28 Jul 2014 02:17:58 -0400 X-AuditID: cbfee61b-f79f86d00000144c-d3-53d5eb147c7e From: Chao Yu To: "'Tyler Hicks'" Cc: ecryptfs@vger.kernel.org, linux-kernel@vger.kernel.org References: <000001cfa721$57072880$05157980$@samsung.com> <20140725033353.GC8446@boyd> In-reply-to: <20140725033353.GC8446@boyd> Subject: RE: [PATCH] ecryptfs: avoid to access NULL pointer when write metadata in xattr Date: Mon, 28 Jul 2014 14:16:57 +0800 Message-id: <004001cfaa2b$abb909e0$032b1da0$@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=us-ascii Content-transfer-encoding: 7bit X-Mailer: Microsoft Outlook 14.0 Content-language: zh-cn Thread-index: AQC/UCS5j8IuYAKTDmzJjAgW/WYZCwLtkhZYnb5Av0A= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrNLMWRmVeSWpSXmKPExsVy+t9jAV2R11eDDY6f4rV4e3c5u8XlXXPY LDbdbGFyYPaY1dDL5vF5k1wAUxSXTUpqTmZZapG+XQJXxreTk1gLVupXdH39ztTA+FSti5GD Q0LAROLxW+0uRk4gU0ziwr31bF2MXBxCAosYJSY83sIO4fxglOifcIUdpIpNQEViecd/JhBb REBT4uOkl6wgNrOAlcTx3auZQWwhgQiJZb9eg9VzAtU0dh5hAbGFBaIkXr5ZzghiswioSlzb c5Ad5AheAUuJY2esQcK8AoISPybfY4EYqSWxfudxJghbXmLzmrfMEIcqSOw4+5oRIi4usfHI LRaIc6wk9uw/yzaBUWgWklGzkIyahWTULCTtCxhZVjGKphYkFxQnpeca6RUn5haX5qXrJefn bmIEB/kz6R2MqxosDjEKcDAq8fBaBF8NFmJNLCuuzD3EKMHBrCTC63kOKMSbklhZlVqUH19U mpNafIhRmoNFSZz3YKt1oJBAemJJanZqakFqEUyWiYNTqoFRoHnqxO8fC49Ms/1lxPTl842E VIVbiqENR22Yuv5OumF36cPj7yp/j3tG9D8tabFiFheZW8nR/2XyW7m1EeXBn0/tMuhd1rsq e+ulnw973zXl+77xqbw546Hn+x8896pXCUZb7rApkrKumnbqmBFDuPU0v1ctB47P5T67zU6g SM0lv/Dq+tjdSizFGYmGWsxFxYkAHZmNP24CAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Tyler, Sorry for replying late. > -----Original Message----- > From: Tyler Hicks [mailto:tyhicks@canonical.com] > Sent: Friday, July 25, 2014 11:34 AM > To: Chao Yu > Cc: ecryptfs@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH] ecryptfs: avoid to access NULL pointer when write metadata in xattr > > Hello and thanks for the patch! > > On 2014-07-24 17:25:42, Chao Yu wrote: > > https://bugzilla.kernel.org/show_bug.cgi?id=41692 > > This actually isn't the bug that this patch fixes. It is a different bug > (that I don't think exists anymore) and someone happened to test for the > bug on a newer kernel and hit the oops that you're fixing here. > > > > > Christopher Head 2014-06-28 05:26:20 UTC described: > > "I tried to reproduce this on 3.12.21. Instead, when I do "echo hello > foo" > > in an ecryptfs mount with ecryptfs_xattr specified, I get a kernel crash: > > > > BUG: unable to handle kernel NULL pointer dereference at (null) > > IP: [] fsstack_copy_attr_all+0x2/0x61 > > PGD d7840067 PUD b2c3c067 PMD 0 > > Oops: 0002 [#1] SMP > > Modules linked in: nvidia(PO) > > CPU: 3 PID: 3566 Comm: bash Tainted: P O 3.12.21-gentoo-r1 #2 > > Hardware name: ASUSTek Computer Inc. G60JX/G60JX, BIOS 206 03/15/2010 > > task: ffff8801948944c0 ti: ffff8800bad70000 task.ti: ffff8800bad70000 > > RIP: 0010:[] [] fsstack_copy_attr_all+0x2/0x61 > > RSP: 0018:ffff8800bad71c10 EFLAGS: 00010246 > > RAX: 00000000000181a4 RBX: ffff880198648480 RCX: 0000000000000000 > > RDX: 0000000000000004 RSI: ffff880172010450 RDI: 0000000000000000 > > RBP: ffff880198490e40 R08: 0000000000000000 R09: 0000000000000000 > > R10: ffff880172010450 R11: ffffea0002c51e80 R12: 0000000000002000 > > R13: 000000000000001a R14: 0000000000000000 R15: ffff880198490e40 > > FS: 00007ff224caa700(0000) GS:ffff88019fcc0000(0000) knlGS:0000000000000000 > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > > CR2: 0000000000000000 CR3: 00000000bb07f000 CR4: 00000000000007e0 > > Stack: > > ffffffff811826e8 ffff8800a39d8000 0000000000000000 000000000000001a > > ffff8800a01d0000 ffff8800a39d8000 ffffffff81185fd5 ffffffff81082c2c > > 00000001a39d8000 53d0abbc98490e40 0000000000000037 ffff8800a39d8220 > > Call Trace: > > [] ? ecryptfs_setxattr+0x40/0x52 > > [] ? ecryptfs_write_metadata+0x1b3/0x223 > > [] ? should_resched+0x5/0x23 > > [] ? ecryptfs_initialize_file+0xaf/0xd4 > > [] ? ecryptfs_create+0xf4/0x142 > > [] ? vfs_create+0x48/0x71 > > [] ? do_last.isra.68+0x559/0x952 > > [] ? link_path_walk+0xbd/0x458 > > [] ? path_openat+0x224/0x472 > > [] ? do_filp_open+0x2b/0x6f > > [] ? __alloc_fd+0xd6/0xe7 > > [] ? do_sys_open+0x65/0xe9 > > [] ? system_call_fastpath+0x16/0x1b > > RIP [] fsstack_copy_attr_all+0x2/0x61 > > RSP > > CR2: 0000000000000000 > > ---[ end trace df9dba5f1ddb8565 ]---" > > > > If we create a file when we mount with ecryptfs_xattr_metadata option, we will > > encounter a crash in this path: > > ->ecryptfs_create > > ->ecryptfs_initialize_file > > ->ecryptfs_write_metadata > > ->ecryptfs_write_metadata_to_xattr > > ->ecryptfs_setxattr > > ->fsstack_copy_attr_all > > It's because our dentry->d_inode used in fsstack_copy_attr_all is NULL, and it > > will be initialized when ecryptfs_initialize_file finish. > > > > So we should skip copying attr from lower inode when the value of ->d_inode is > > invalid. > > > > Signed-off-by: Chao Yu > > --- > > I wrote a patch to fix this bug last week, made the changes necessary to > run through all of the eCryptfs tests with the ecryptfs_xattr_metadata > mount option, tested the fix, and then forgot to send out the patch. > (doh!) But it worked out well because I think I like your patch better. > > I moved the innards of ecryptfs_setxattr() into a new function that > accepts the lower_dentry and ecryptfs inode as separate arguments. That > way it can work regardless of the eCryptfs dentry not yet being > d_initialized(). I guess you want to introduce an inner function like __ecryptfs_setxattr which could be invoked by both ecryptfs_write_metadata_to_xattr and ecryptfs_setxattr, right? After that, we could pass the right ecryptfs inode from ecryptfs_write_metadata_to_xattr to __ecryptfs_setxattr. I think it's great way to fix this issue too! > > After looking at your patch, I don't think that's necessary. Skipping > the fsstack_copy_attr_all() should be fine for brand new eCryptfs > inodes since the attr copy just happened in ecryptfs_inode_set(). Yes, that's right. I just want to use this simplest way to fix this issue. I hope it may not cause other problem. Maybe we should add some annotation for readability here. How do you think? > > I'll test your patch and then push it to the eCryptfs -next branch. I'll > also update the bug link to point to the specific comment rather than > the unrelated bug itself. Thanks! Thank you very much! :) Any problem in this patch, please tell me. > > Tyler > > > fs/ecryptfs/inode.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c > > index e67f9f0..436b7d8 100644 > > --- a/fs/ecryptfs/inode.c > > +++ b/fs/ecryptfs/inode.c > > @@ -1037,7 +1037,7 @@ ecryptfs_setxattr(struct dentry *dentry, const char *name, const void > *value, > > } > > > > rc = vfs_setxattr(lower_dentry, name, value, size, flags); > > - if (!rc) > > + if (!rc && dentry->d_inode) > > fsstack_copy_attr_all(dentry->d_inode, lower_dentry->d_inode); > > out: > > return rc; > > -- > > 2.0.1.474.g72c7794 > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/