From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tyler Hicks Subject: Re: [PATCH] ecryptfs: initialize private persistent file before dereferencing pointer Date: Fri, 04 Dec 2009 17:23:57 -0600 Message-ID: <4B199A0D.5050807@linux.vnet.ibm.com> References: <200912031835.nB3IZRD3002819@agora.fsl.cs.sunysb.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Dustin Kirkland , linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk, Andrew Morton , Duckjin Kang To: Erez Zadok Return-path: Received: from e38.co.us.ibm.com ([32.97.110.159]:33791 "EHLO e38.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932380AbZLDXX4 (ORCPT ); Fri, 4 Dec 2009 18:23:56 -0500 Received: from d03relay03.boulder.ibm.com (d03relay03.boulder.ibm.com [9.17.195.228]) by e38.co.us.ibm.com (8.14.3/8.13.1) with ESMTP id nB4NJHd3021984 for ; Fri, 4 Dec 2009 16:19:17 -0700 Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d03relay03.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id nB4NNx8o038522 for ; Fri, 4 Dec 2009 16:23:59 -0700 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.3/8.13.1/NCO v10.0 AVout) with ESMTP id nB4NNwLe031771 for ; Fri, 4 Dec 2009 16:23:59 -0700 In-Reply-To: <200912031835.nB3IZRD3002819@agora.fsl.cs.sunysb.edu> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 12/03/2009 12:35 PM, Erez Zadok wrote: > Tyler/Dustin, > > As promised, here's another patch for ecryptfs to fix a problem discovered > by my student Duckjin Kang. I've not personally tested this patch, but it > seems safe to me: it swaps the order of two "if" statements so you first > initialize the lower private file, and then you do something that may oops > dereferencing a NULL ptr. This patch is probably not that urgent to send > out right away, b/c if this NULL ptr was dereferenced easily, you'd have > found out about it more quickly. I think this bug might get triggered more > easily under memory pressure: when an inode holding the lower file is purged > to free up memory, while a concurrent open() takes place. Anyway, you may > have a better fix for this. > > Cheers, > Erez. > > ------------------------------------------------------------------------------ > > Ecryptfs: initialize private persistent file before dereferencing pointer > > Ecryptfs_open dereferences a pointer to the private lower file (the one > stored in the ecryptfs inode), without checking if the pointer is NULL. > Right afterward, it initializes that pointer if it is NULL. Swap order of > statements to first initialize. Bug discovered by Duckjin Kang. > > Signed-off-by: Duckjin Kang > Signed-off-by: Erez Zadok Thanks for the fix! Applied to git://git.kernel.org/pub/scm/linux/kernel/git/ecryptfs/ecryptfs-2.6.git#next > diff --git a/fs/ecryptfs/file.c b/fs/ecryptfs/file.c > index 9e94405..1744f17 100644 > --- a/fs/ecryptfs/file.c > +++ b/fs/ecryptfs/file.c > @@ -191,13 +191,6 @@ static int ecryptfs_open(struct inode *inode, struct file *file) > | ECRYPTFS_ENCRYPTED); > } > mutex_unlock(&crypt_stat->cs_mutex); > - if ((ecryptfs_inode_to_private(inode)->lower_file->f_flags & O_RDONLY) > - && !(file->f_flags & O_RDONLY)) { > - rc = -EPERM; > - printk(KERN_WARNING "%s: Lower persistent file is RO; eCryptfs " > - "file must hence be opened RO\n", __func__); > - goto out; > - } > if (!ecryptfs_inode_to_private(inode)->lower_file) { > rc = ecryptfs_init_persistent_file(ecryptfs_dentry); > if (rc) { > @@ -208,6 +201,13 @@ static int ecryptfs_open(struct inode *inode, struct file *file) > goto out; > } > } > + if ((ecryptfs_inode_to_private(inode)->lower_file->f_flags & O_RDONLY) > + && !(file->f_flags & O_RDONLY)) { > + rc = -EPERM; > + printk(KERN_WARNING "%s: Lower persistent file is RO; eCryptfs " > + "file must hence be opened RO\n", __func__); > + goto out; > + } > ecryptfs_set_file_lower( > file, ecryptfs_inode_to_private(inode)->lower_file); > if (S_ISDIR(ecryptfs_dentry->d_inode->i_mode)) {