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

[Tyler Hicks <tyhicks@linux.vnet.ibm.com>]

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