From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavel Boldin Subject: [PATCH] Fix `eventfd_link' module leakages and races Date: Mon, 16 Mar 2015 18:40:59 +0200 Message-ID: <1426524059-30886-1-git-send-email-pboldin+dpdk@mirantis.com> Cc: Pavel Boldin To: dev-VfR2kkLFssw@public.gmane.org Return-path: List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org Sender: "dev" From: Pavel Boldin The `eventfd_link' module provides an API to "steal" fd from another process had been written with a bug that leaks `struct file' because of the extra reference counter increment and missing `fput' call. The other bug is using another process' `task_struct' without incrementing a reference counter. Fix these bugs and refactor the module. --- examples/vhost/eventfd_link/eventfd_link.c | 236 ++++++++++++++++------------- 1 file changed, 130 insertions(+), 106 deletions(-) diff --git a/examples/vhost/eventfd_link/eventfd_link.c b/examples/vhost/eventfd_link/eventfd_link.c index 69470ba..9f1f8fb 100644 --- a/examples/vhost/eventfd_link/eventfd_link.c +++ b/examples/vhost/eventfd_link/eventfd_link.c @@ -42,15 +42,15 @@ * get_files_struct is copied from fs/file.c */ struct files_struct * -get_files_struct (struct task_struct *task) +get_files_struct(struct task_struct *task) { struct files_struct *files; - task_lock (task); + task_lock(task); files = task->files; if (files) - atomic_inc (&files->count); - task_unlock (task); + atomic_inc(&files->count); + task_unlock(task); return files; } @@ -59,117 +59,142 @@ get_files_struct (struct task_struct *task) * put_files_struct is extracted from fs/file.c */ void -put_files_struct (struct files_struct *files) +put_files_struct(struct files_struct *files) { - if (atomic_dec_and_test (&files->count)) - { + if (atomic_dec_and_test(&files->count)) BUG (); +} + +static struct file * +fget_from_files(struct files_struct *files, unsigned fd) +{ + struct file *file; + + rcu_read_lock(); + file = fcheck_files(files, fd); + if (file) + { + if (file->f_mode & FMODE_PATH + || !atomic_long_inc_not_zero(&file->f_count)) + file = NULL; } + rcu_read_unlock(); + + return file; +} + +static int +close_fd(unsigned fd) +{ + struct file *file; + struct files_struct *files = current->files; + struct fdtable *fdt; + + spin_lock(&files->file_lock); + fdt = files_fdtable(files); + if (fd >= fdt->max_fds) + goto out_unlock; + file = fdt->fd[fd]; + if (!file) + goto out_unlock; + rcu_assign_pointer(fdt->fd[fd], NULL); + __clear_bit(fd, fdt->close_on_exec); + spin_unlock(&files->file_lock); + return filp_close(file, files); + +out_unlock: + spin_unlock(&files->file_lock); + return -EBADF; } static long -eventfd_link_ioctl (struct file *f, unsigned int ioctl, unsigned long arg) +eventfd_link_ioctl_copy(unsigned long arg) { - void __user *argp = (void __user *) arg; + long ret = -EFAULT; struct task_struct *task_target = NULL; - struct file *file; - struct files_struct *files; - struct fdtable *fdt; + struct file *target_file = NULL; + struct files_struct *target_files = NULL; struct eventfd_copy eventfd_copy; + struct pid *pid; + + if (copy_from_user(&eventfd_copy, (void __user*)arg, + sizeof(struct eventfd_copy))) + goto out; + + /* + * Find the task struct for the target pid + */ + pid = find_vpid(eventfd_copy.target_pid); + if (pid == NULL) { + printk(KERN_INFO "Unable to find pid %d\n", + eventfd_copy.target_pid); + goto out; + } + + task_target = get_pid_task(pid, PIDTYPE_PID); + if (task_target == NULL) { + printk(KERN_INFO "Failed to get task for pid %d\n", + eventfd_copy.target_pid); + goto out; + } + + ret = close_fd(eventfd_copy.source_fd); + if (ret) + goto out_task; + ret = -EFAULT; + + /* + * Find the file struct associated with the target fd. + */ + + target_files = get_files_struct(task_target); + if (target_files == NULL) { + printk (KERN_INFO "Failed to get target files struct\n"); + goto out_task; + } + + target_file = fget_from_files(target_files, eventfd_copy.target_fd); + + if (target_file == NULL) { + printk (KERN_INFO "Failed to get file from target pid\n"); + goto out_target_files; + } + + + /* + * Install the file struct from the target process into the + * file desciptor of the source process, + */ + + fd_install(eventfd_copy.source_fd, target_file); + + ret = 0; + +out_target_files: + put_files_struct(target_files); +out_task: + put_task_struct(task_target); +out: + return ret; +} + +static long +eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg) +{ + long ret; switch (ioctl) { case EVENTFD_COPY: - if (copy_from_user (&eventfd_copy, argp, sizeof (struct eventfd_copy))) - return -EFAULT; - - /* - * Find the task struct for the target pid - */ - task_target = - pid_task (find_vpid (eventfd_copy.target_pid), PIDTYPE_PID); - if (task_target == NULL) - { - printk (KERN_DEBUG "Failed to get mem ctx for target pid\n"); - return -EFAULT; - } - - files = get_files_struct (current); - if (files == NULL) - { - printk (KERN_DEBUG "Failed to get files struct\n"); - return -EFAULT; - } - - rcu_read_lock (); - file = fcheck_files (files, eventfd_copy.source_fd); - if (file) - { - if (file->f_mode & FMODE_PATH - || !atomic_long_inc_not_zero (&file->f_count)) - file = NULL; - } - rcu_read_unlock (); - put_files_struct (files); - - if (file == NULL) - { - printk (KERN_DEBUG "Failed to get file from source pid\n"); - return 0; - } - - /* - * Release the existing eventfd in the source process - */ - spin_lock (&files->file_lock); - fput(file); - filp_close (file, files); - fdt = files_fdtable (files); - fdt->fd[eventfd_copy.source_fd] = NULL; - spin_unlock (&files->file_lock); - - /* - * Find the file struct associated with the target fd. - */ - - files = get_files_struct (task_target); - if (files == NULL) - { - printk (KERN_DEBUG "Failed to get files struct\n"); - return -EFAULT; - } - - rcu_read_lock (); - file = fcheck_files (files, eventfd_copy.target_fd); - if (file) - { - if (file->f_mode & FMODE_PATH - || !atomic_long_inc_not_zero (&file->f_count)) - file = NULL; - } - rcu_read_unlock (); - put_files_struct (files); - - if (file == NULL) - { - printk (KERN_DEBUG "Failed to get file from target pid\n"); - return 0; - } - - - /* - * Install the file struct from the target process into the - * file desciptor of the source process, - */ - - fd_install (eventfd_copy.source_fd, file); - - return 0; - + ret = eventfd_link_ioctl_copy(arg); + break; default: - return -ENOIOCTLCMD; + ret = -ENOIOCTLCMD; + break; } + + return ret; } static const struct file_operations eventfd_link_fops = { @@ -184,21 +209,20 @@ static struct miscdevice eventfd_link_misc = { }; static int __init -eventfd_link_init (void) +eventfd_link_init(void) { - printk(KERN_INFO "eventfd_link is broken, use it at your risk\n"); - return misc_register (&eventfd_link_misc); + return misc_register(&eventfd_link_misc); } -module_init (eventfd_link_init); +module_init(eventfd_link_init); static void __exit -eventfd_link_exit (void) +eventfd_link_exit(void) { - misc_deregister (&eventfd_link_misc); + misc_deregister(&eventfd_link_misc); } -module_exit (eventfd_link_exit); +module_exit(eventfd_link_exit); MODULE_VERSION ("0.0.1"); MODULE_LICENSE ("GPL v2"); -- 1.9.1