From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 42BE45A86 for ; Mon, 16 Mar 2015 18:56:04 +0100 (CET) Received: from hmsreliant.think-freely.org ([2001:470:8:a08:7aac:c0ff:fec2:933b] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1YXZF7-0000A6-7O; Mon, 16 Mar 2015 13:56:03 -0400 Date: Mon, 16 Mar 2015 13:55:56 -0400 From: Neil Horman To: Pavel Boldin Message-ID: <20150316175556.GF16238@hmsreliant.think-freely.org> References: <1426524059-30886-1-git-send-email-pboldin+dpdk@mirantis.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1426524059-30886-1-git-send-email-pboldin+dpdk@mirantis.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH] Fix `eventfd_link' module leakages and races X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Mar 2015 17:56:04 -0000 On Mon, Mar 16, 2015 at 06:40:59PM +0200, Pavel Boldin wrote: > 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 > > You need to respin this. This library has since moved to another location in the tree Neil