[PATCH] ecryptfs: initialize private persistent file before dereferencing pointer

[PATCH] ecryptfs: initialize private persistent file before dereferencing pointer

[Erez Zadok]

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 <fromdj2k@gmail.com>
Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
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)) {

Re: [PATCH] ecryptfs: initialize private persistent file before dereferencing pointer

[Tyler Hicks]

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 <fromdj2k@gmail.com>
> Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

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)) {