Ack: [PATCH 1/1] eCryptfs: Gracefully refuse miscdev file ops on inherited/passed files

[Brad Figg]

On 08/01/2012 11:42 AM, Colin King wrote:
> From: Tyler Hicks <tyhicks at canonical.com>
>
> File operations on /dev/ecryptfs would BUG() when the operations were
> performed by processes other than the process that originally opened the
> file. This could happen with open files inherited after fork() or file
> descriptors passed through IPC mechanisms. Rather than calling BUG(), an
> error code can be safely returned in most situations.
>
> In ecryptfs_miscdev_release(), eCryptfs still needs to handle the
> release even if the last file reference is being held by a process that
> didn't originally open the file. ecryptfs_find_daemon_by_euid() will not
> be successful, so a pointer to the daemon is stored in the file's
> private_data. The private_data pointer is initialized when the miscdev
> file is opened and only used when the file is released.
>
> https://launchpad.net/bugs/994247
>
> Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> Reported-by: Sasha Levin <levinsasha928 at gmail.com>
> Tested-by: Sasha Levin <levinsasha928 at gmail.com>
> ---
>   fs/ecryptfs/miscdev.c |   23 ++++++++++++++++-------
>   1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/fs/ecryptfs/miscdev.c b/fs/ecryptfs/miscdev.c
> index 3c632ec..c0038f6 100644
> --- a/fs/ecryptfs/miscdev.c
> +++ b/fs/ecryptfs/miscdev.c
> @@ -49,7 +49,10 @@ ecryptfs_miscdev_poll(struct file *file, poll_table *pt)
>   	mutex_lock(&ecryptfs_daemon_hash_mux);
>   	/* TODO: Just use file->private_data? */
>   	rc = ecryptfs_find_daemon_by_euid(&daemon, euid, current_user_ns());
> -	BUG_ON(rc || !daemon);
> +	if (rc || !daemon) {
> +		mutex_unlock(&ecryptfs_daemon_hash_mux);
> +		return -EINVAL;
> +	}
>   	mutex_lock(&daemon->mux);
>   	mutex_unlock(&ecryptfs_daemon_hash_mux);
>   	if (daemon->flags & ECRYPTFS_DAEMON_ZOMBIE) {
> @@ -122,6 +125,7 @@ ecryptfs_miscdev_open(struct inode *inode, struct file *file)
>   		goto out_unlock_daemon;
>   	}
>   	daemon->flags |= ECRYPTFS_DAEMON_MISCDEV_OPEN;
> +	file->private_data = daemon;
>   	atomic_inc(&ecryptfs_num_miscdev_opens);
>   out_unlock_daemon:
>   	mutex_unlock(&daemon->mux);
> @@ -152,9 +156,9 @@ ecryptfs_miscdev_release(struct inode *inode, struct file *file)
>
>   	mutex_lock(&ecryptfs_daemon_hash_mux);
>   	rc = ecryptfs_find_daemon_by_euid(&daemon, euid, current_user_ns());
> -	BUG_ON(rc || !daemon);
> +	if (rc || !daemon)
> +		daemon = file->private_data;
>   	mutex_lock(&daemon->mux);
> -	BUG_ON(daemon->pid != task_pid(current));
>   	BUG_ON(!(daemon->flags & ECRYPTFS_DAEMON_MISCDEV_OPEN));
>   	daemon->flags &= ~ECRYPTFS_DAEMON_MISCDEV_OPEN;
>   	atomic_dec(&ecryptfs_num_miscdev_opens);
> @@ -270,8 +274,16 @@ ecryptfs_miscdev_read(struct file *file, char __user *buf, size_t count,
>   	mutex_lock(&ecryptfs_daemon_hash_mux);
>   	/* TODO: Just use file->private_data? */
>   	rc = ecryptfs_find_daemon_by_euid(&daemon, euid, current_user_ns());
> -	BUG_ON(rc || !daemon);
> +	if (rc || !daemon) {
> +		mutex_unlock(&ecryptfs_daemon_hash_mux);
> +		return -EINVAL;
> +	}
>   	mutex_lock(&daemon->mux);
> +	if (task_pid(current) != daemon->pid) {
> +		mutex_unlock(&daemon->mux);
> +		mutex_unlock(&ecryptfs_daemon_hash_mux);
> +		return -EPERM;
> +	}
>   	if (daemon->flags & ECRYPTFS_DAEMON_ZOMBIE) {
>   		rc = 0;
>   		mutex_unlock(&ecryptfs_daemon_hash_mux);
> @@ -308,9 +320,6 @@ check_list:
>   		 * message from the queue; try again */
>   		goto check_list;
>   	}
> -	BUG_ON(euid != daemon->euid);
> -	BUG_ON(current_user_ns() != daemon->user_ns);
> -	BUG_ON(task_pid(current) != daemon->pid);
>   	msg_ctx = list_first_entry(&daemon->msg_ctx_out_queue,
>   				   struct ecryptfs_msg_ctx, daemon_out_list);
>   	BUG_ON(!msg_ctx);
>


-- 
Brad Figg brad.figg at canonical.com http://www.canonical.com