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@xxxxxxxxx>
> Signed-off-by: Erez Zadok <ezk@xxxxxxxxxxxxx>
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)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
