Ack: [PATCH] eCryptfs: Prevent file create race condition

Stefan Bader stefan.bader at canonical.com
Thu Nov 24 14:33:22 UTC 2011


On 24.11.2011 09:00, Tyler Hicks wrote:
> The file creation path prematurely called d_instantiate() and
> unlock_new_inode() before the eCryptfs inode info was fully
> allocated and initialized and before the eCryptfs metadata was written
> to the lower file.
> 
> This could result in race conditions in subsequent file and inode
> operations leading to unexpected error conditions or a null pointer
> dereference while attempting to use the unallocated memory.
> 
> https://launchpad.net/bugs/813146
> 
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> ---
> 
> Upstream: b59db43ad4434519feb338eacb01d77eb50825c5
> 
> This is the upstream commit backported to ubuntu-oneiric/master-next. There is
> a minor change from the upstream commit to work around the lack of
> bf6c7f6c7bd0ea779757d35b5fdc9f9157f056b3 in Oneiric.
> 
>  fs/ecryptfs/crypto.c          |   22 +++++++++--------
>  fs/ecryptfs/ecryptfs_kernel.h |    5 ++-
>  fs/ecryptfs/inode.c           |   52 ++++++++++++++++++++++++----------------
>  3 files changed, 46 insertions(+), 33 deletions(-)
> 
> diff --git a/fs/ecryptfs/crypto.c b/fs/ecryptfs/crypto.c
> index 58609bd..203a1fd 100644
> --- a/fs/ecryptfs/crypto.c
> +++ b/fs/ecryptfs/crypto.c
> @@ -967,7 +967,7 @@ static void ecryptfs_set_default_crypt_stat_vals(
>  
>  /**
>   * ecryptfs_new_file_context
> - * @ecryptfs_dentry: The eCryptfs dentry
> + * @ecryptfs_inode: The eCryptfs inode
>   *
>   * If the crypto context for the file has not yet been established,
>   * this is where we do that.  Establishing a new crypto context
> @@ -984,13 +984,13 @@ static void ecryptfs_set_default_crypt_stat_vals(
>   *
>   * Returns zero on success; non-zero otherwise
>   */
> -int ecryptfs_new_file_context(struct dentry *ecryptfs_dentry)
> +int ecryptfs_new_file_context(struct inode *ecryptfs_inode)
>  {
>  	struct ecryptfs_crypt_stat *crypt_stat =
> -	    &ecryptfs_inode_to_private(ecryptfs_dentry->d_inode)->crypt_stat;
> +	    &ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat;
>  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat =
>  	    &ecryptfs_superblock_to_private(
> -		    ecryptfs_dentry->d_sb)->mount_crypt_stat;
> +		    ecryptfs_inode->i_sb)->mount_crypt_stat;
>  	int cipher_name_len;
>  	int rc = 0;
>  
> @@ -1299,12 +1299,12 @@ static int ecryptfs_write_headers_virt(char *page_virt, size_t max,
>  }
>  
>  static int
> -ecryptfs_write_metadata_to_contents(struct dentry *ecryptfs_dentry,
> +ecryptfs_write_metadata_to_contents(struct inode *ecryptfs_inode,
>  				    char *virt, size_t virt_len)
>  {
>  	int rc;
>  
> -	rc = ecryptfs_write_lower(ecryptfs_dentry->d_inode, virt,
> +	rc = ecryptfs_write_lower(ecryptfs_inode, virt,
>  				  0, virt_len);
>  	if (rc < 0)
>  		printk(KERN_ERR "%s: Error attempting to write header "
> @@ -1338,7 +1338,8 @@ static unsigned long ecryptfs_get_zeroed_pages(gfp_t gfp_mask,
>  
>  /**
>   * ecryptfs_write_metadata
> - * @ecryptfs_dentry: The eCryptfs dentry
> + * @ecryptfs_dentry: The eCryptfs dentry, which should be negative
> + * @ecryptfs_inode: The newly created eCryptfs inode
>   *
>   * Write the file headers out.  This will likely involve a userspace
>   * callout, in which the session key is encrypted with one or more
> @@ -1348,10 +1349,11 @@ static unsigned long ecryptfs_get_zeroed_pages(gfp_t gfp_mask,
>   *
>   * Returns zero on success; non-zero on error
>   */
> -int ecryptfs_write_metadata(struct dentry *ecryptfs_dentry)
> +int ecryptfs_write_metadata(struct dentry *ecryptfs_dentry,
> +			    struct inode *ecryptfs_inode)
>  {
>  	struct ecryptfs_crypt_stat *crypt_stat =
> -		&ecryptfs_inode_to_private(ecryptfs_dentry->d_inode)->crypt_stat;
> +		&ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat;
>  	unsigned int order;
>  	char *virt;
>  	size_t virt_len;
> @@ -1391,7 +1393,7 @@ int ecryptfs_write_metadata(struct dentry *ecryptfs_dentry)
>  		rc = ecryptfs_write_metadata_to_xattr(ecryptfs_dentry, virt,
>  						      size);
>  	else
> -		rc = ecryptfs_write_metadata_to_contents(ecryptfs_dentry, virt,
> +		rc = ecryptfs_write_metadata_to_contents(ecryptfs_inode, virt,
>  							 virt_len);
>  	if (rc) {
>  		printk(KERN_ERR "%s: Error writing metadata out to lower file; "
> diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> index 43c7c43..be92836 100644
> --- a/fs/ecryptfs/ecryptfs_kernel.h
> +++ b/fs/ecryptfs/ecryptfs_kernel.h
> @@ -654,9 +654,10 @@ int ecryptfs_init_crypt_ctx(struct ecryptfs_crypt_stat *crypt_stat);
>  int ecryptfs_write_inode_size_to_metadata(struct inode *ecryptfs_inode);
>  int ecryptfs_encrypt_page(struct page *page);
>  int ecryptfs_decrypt_page(struct page *page);
> -int ecryptfs_write_metadata(struct dentry *ecryptfs_dentry);
> +int ecryptfs_write_metadata(struct dentry *ecryptfs_dentry,
> +			    struct inode *ecryptfs_inode);
>  int ecryptfs_read_metadata(struct dentry *ecryptfs_dentry);
> -int ecryptfs_new_file_context(struct dentry *ecryptfs_dentry);
> +int ecryptfs_new_file_context(struct inode *ecryptfs_inode);
>  void ecryptfs_write_crypt_stat_flags(char *page_virt,
>  				     struct ecryptfs_crypt_stat *crypt_stat,
>  				     size_t *written);
> diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
> index 4a4fad7..f0a4300 100644
> --- a/fs/ecryptfs/inode.c
> +++ b/fs/ecryptfs/inode.c
> @@ -194,9 +194,9 @@ ecryptfs_create_underlying_file(struct inode *lower_dir_inode,
>   * it. It will also update the eCryptfs directory inode to mimic the
>   * stat of the lower directory inode.
>   *
> - * Returns zero on success; non-zero on error condition
> + * Returns the new eCryptfs inode on success; an ERR_PTR on error condition
>   */
> -static int
> +static struct inode *
>  ecryptfs_do_create(struct inode *directory_inode,
>  		   struct dentry *ecryptfs_dentry, int mode,
>  		   struct nameidata *nd)
> @@ -204,13 +204,14 @@ ecryptfs_do_create(struct inode *directory_inode,
>  	int rc;
>  	struct dentry *lower_dentry;
>  	struct dentry *lower_dir_dentry;
> +	struct inode *inode;
>  
>  	lower_dentry = ecryptfs_dentry_to_lower(ecryptfs_dentry);
>  	lower_dir_dentry = lock_parent(lower_dentry);
>  	if (IS_ERR(lower_dir_dentry)) {
>  		ecryptfs_printk(KERN_ERR, "Error locking directory of "
>  				"dentry\n");
> -		rc = PTR_ERR(lower_dir_dentry);
> +		inode = ERR_CAST(lower_dir_dentry);
>  		goto out;
>  	}
>  	rc = ecryptfs_create_underlying_file(lower_dir_dentry->d_inode,
> @@ -218,20 +219,19 @@ ecryptfs_do_create(struct inode *directory_inode,
>  	if (rc) {
>  		printk(KERN_ERR "%s: Failure to create dentry in lower fs; "
>  		       "rc = [%d]\n", __func__, rc);
> +		inode = ERR_PTR(rc);
>  		goto out_lock;
>  	}
> -	rc = ecryptfs_interpose(lower_dentry, ecryptfs_dentry,
> -				directory_inode->i_sb);
> -	if (rc) {
> -		ecryptfs_printk(KERN_ERR, "Failure in ecryptfs_interpose\n");
> +	inode = __ecryptfs_get_inode(lower_dentry->d_inode,
> +				     directory_inode->i_sb);
> +	if (IS_ERR(inode))
>  		goto out_lock;
> -	}
>  	fsstack_copy_attr_times(directory_inode, lower_dir_dentry->d_inode);
>  	fsstack_copy_inode_size(directory_inode, lower_dir_dentry->d_inode);
>  out_lock:
>  	unlock_dir(lower_dir_dentry);
>  out:
> -	return rc;
> +	return inode;
>  }
>  
>  /**
> @@ -242,26 +242,26 @@ out:
>   *
>   * Returns zero on success
>   */
> -static int ecryptfs_initialize_file(struct dentry *ecryptfs_dentry)
> +static int ecryptfs_initialize_file(struct dentry *ecryptfs_dentry,
> +				    struct inode *ecryptfs_inode)
>  {
>  	struct ecryptfs_crypt_stat *crypt_stat =
> -		&ecryptfs_inode_to_private(ecryptfs_dentry->d_inode)->crypt_stat;
> +		&ecryptfs_inode_to_private(ecryptfs_inode)->crypt_stat;
>  	int rc = 0;
>  
> -	if (S_ISDIR(ecryptfs_dentry->d_inode->i_mode)) {
> +	if (S_ISDIR(ecryptfs_inode->i_mode)) {
>  		ecryptfs_printk(KERN_DEBUG, "This is a directory\n");
>  		crypt_stat->flags &= ~(ECRYPTFS_ENCRYPTED);
>  		goto out;
>  	}
>  	ecryptfs_printk(KERN_DEBUG, "Initializing crypto context\n");
> -	rc = ecryptfs_new_file_context(ecryptfs_dentry);
> +	rc = ecryptfs_new_file_context(ecryptfs_inode);
>  	if (rc) {
>  		ecryptfs_printk(KERN_ERR, "Error creating new file "
>  				"context; rc = [%d]\n", rc);
>  		goto out;
>  	}
> -	rc = ecryptfs_get_lower_file(ecryptfs_dentry,
> -				     ecryptfs_dentry->d_inode);
> +	rc = ecryptfs_get_lower_file(ecryptfs_dentry, ecryptfs_inode);
>  	if (rc) {
>  		printk(KERN_ERR "%s: Error attempting to initialize "
>  			"the lower file for the dentry with name "
> @@ -269,10 +269,10 @@ static int ecryptfs_initialize_file(struct dentry *ecryptfs_dentry)
>  			ecryptfs_dentry->d_name.name, rc);
>  		goto out;
>  	}
> -	rc = ecryptfs_write_metadata(ecryptfs_dentry);
> +	rc = ecryptfs_write_metadata(ecryptfs_dentry, ecryptfs_inode);
>  	if (rc)
>  		printk(KERN_ERR "Error writing headers; rc = [%d]\n", rc);
> -	ecryptfs_put_lower_file(ecryptfs_dentry->d_inode);
> +	ecryptfs_put_lower_file(ecryptfs_inode);
>  out:
>  	return rc;
>  }
> @@ -292,18 +292,28 @@ static int
>  ecryptfs_create(struct inode *directory_inode, struct dentry *ecryptfs_dentry,
>  		int mode, struct nameidata *nd)
>  {
> +	struct inode *ecryptfs_inode;
>  	int rc;
>  
> -	/* ecryptfs_do_create() calls ecryptfs_interpose() */
> -	rc = ecryptfs_do_create(directory_inode, ecryptfs_dentry, mode, nd);
> -	if (unlikely(rc)) {
> +	ecryptfs_inode = ecryptfs_do_create(directory_inode, ecryptfs_dentry,
> +					    mode, nd);
> +	if (unlikely(IS_ERR(ecryptfs_inode))) {
>  		ecryptfs_printk(KERN_WARNING, "Failed to create file in"
>  				"lower filesystem\n");
> +		rc = PTR_ERR(ecryptfs_inode);
>  		goto out;
>  	}
>  	/* At this point, a file exists on "disk"; we need to make sure
>  	 * that this on disk file is prepared to be an ecryptfs file */
> -	rc = ecryptfs_initialize_file(ecryptfs_dentry);
> +	rc = ecryptfs_initialize_file(ecryptfs_dentry, ecryptfs_inode);
> +	if (rc) {
> +		drop_nlink(ecryptfs_inode);
> +		unlock_new_inode(ecryptfs_inode);
> +		iput(ecryptfs_inode);
> +		goto out;
> +	}
> +	d_instantiate(ecryptfs_dentry, ecryptfs_inode);
> +	unlock_new_inode(ecryptfs_inode);
>  out:
>  	return rc;
>  }

Like Herton said, the commit body needs a bit maintenance...




More information about the kernel-team mailing list