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

Submitted by Colin King on Aug. 1, 2012, 3:42 p.m.

Details

Message ID 1343835735-10249-2-git-send-email-colin.king@canonical.com
State New
Headers show

Commit Message

Colin King Aug. 1, 2012, 3:42 p.m.
From: Tyler Hicks <tyhicks@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@canonical.com>
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reported-by: Sasha Levin <levinsasha928@gmail.com>
Tested-by: Sasha Levin <levinsasha928@gmail.com>
---
 fs/ecryptfs/miscdev.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Comments

Brad Figg Aug. 1, 2012, 3:56 p.m.
On 08/01/2012 11:42 AM, Colin King wrote:
> From: Tyler Hicks <tyhicks@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@canonical.com>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Reported-by: Sasha Levin <levinsasha928@gmail.com>
> Tested-by: Sasha Levin <levinsasha928@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);
>

Patch hide | download patch | download mbox

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