[dpdk-dev,v2] Fix `eventfd_link' module leakages and races

Message ID 1426684571-14782-1-git-send-email-pboldin@mirantis.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Pavel Boldin March 18, 2015, 1:16 p.m. UTC
  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.
---

Changes since last submission:

* Rebased to the `master' version,
* Corrected error codes returned.

 lib/librte_vhost/eventfd_link/eventfd_link.c | 212 ++++++++++++++++-----------
 1 file changed, 125 insertions(+), 87 deletions(-)
  

Comments

Thomas Monjalon March 23, 2015, 11:15 a.m. UTC | #1
Hi Pavel,

You forgot the Signed-off-by line.

Huawei, Changchun, any comment on this patch?

2015-03-18 15:16, 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.

As there are 2 bugs, you should provide 2 patches.

> Fix these bugs and refactor the module.

Why refactoring along with the bug fixes?
You'd have more chances to have your fixes integrated in 2.0 without
refactoring.

Thanks
  
Pavel Boldin March 23, 2015, 11:36 a.m. UTC | #2
Hi Thomas,

There is by far more than 2 issues, but these are the only ones that
appeared to us.

The list of the issues that motivated the refactoring:

   1. Only one error code for every fault (-EFAULT).
   2. Copy-paste code from the `fget' function.
   3. Ambiguous variable names. The `files' variable mean different things
   under the same block. This ruins readability of the code.
   4. Overall placing of the all the code inside one `switch/case'. Modern
   kernel code style suggests placing these in the inline functions.
   5. Usage of the copy-pasted `close_fd' function code. (I really should
   use `sys_close' here though).


I can rewrite the refactoring in a set of small patches if this will aid
reviewers.

The dangerous bug that definitely must be fixed before the next release is
the `struct file*' leakage. I can provide a simple patch for this as a
separate submission and continue working on the refactoring patches. Is
this plan OK for you?


Pavel

On Mon, Mar 23, 2015 at 1:15 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> Hi Pavel,
>
> You forgot the Signed-off-by line.
>
> Huawei, Changchun, any comment on this patch?
>
> 2015-03-18 15:16, 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.
>
> As there are 2 bugs, you should provide 2 patches.
>
> > Fix these bugs and refactor the module.
>
> Why refactoring along with the bug fixes?
> You'd have more chances to have your fixes integrated in 2.0 without
> refactoring.
>
> Thanks
>
>
>
>
  
Thomas Monjalon March 23, 2015, 11:44 a.m. UTC | #3
2015-03-23 13:36, Pavel Boldin:
> Hi Thomas,
> 
> There is by far more than 2 issues, but these are the only ones that
> appeared to us.
> 
> The list of the issues that motivated the refactoring:
> 
>    1. Only one error code for every fault (-EFAULT).
>    2. Copy-paste code from the `fget' function.
>    3. Ambiguous variable names. The `files' variable mean different things
>    under the same block. This ruins readability of the code.
>    4. Overall placing of the all the code inside one `switch/case'. Modern
>    kernel code style suggests placing these in the inline functions.
>    5. Usage of the copy-pasted `close_fd' function code. (I really should
>    use `sys_close' here though).
> 
> 
> I can rewrite the refactoring in a set of small patches if this will aid
> reviewers.
> 
> The dangerous bug that definitely must be fixed before the next release is
> the `struct file*' leakage. I can provide a simple patch for this as a
> separate submission and continue working on the refactoring patches. Is
> this plan OK for you?

Yes, please separate important fixes and refactoring.
Then we'll be able to merge the fixes in 2.0.

PS: please answer below the question to ease thread reading.

Thanks


> On Mon, Mar 23, 2015 at 1:15 PM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> 
> > Hi Pavel,
> >
> > You forgot the Signed-off-by line.
> >
> > Huawei, Changchun, any comment on this patch?
> >
> > 2015-03-18 15:16, 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.
> >
> > As there are 2 bugs, you should provide 2 patches.
> >
> > > Fix these bugs and refactor the module.
> >
> > Why refactoring along with the bug fixes?
> > You'd have more chances to have your fixes integrated in 2.0 without
> > refactoring.
> >
> > Thanks
  

Patch

diff --git a/lib/librte_vhost/eventfd_link/eventfd_link.c b/lib/librte_vhost/eventfd_link/eventfd_link.c
index 7755dd6..57b0a8a 100644
--- a/lib/librte_vhost/eventfd_link/eventfd_link.c
+++ b/lib/librte_vhost/eventfd_link/eventfd_link.c
@@ -65,100 +65,138 @@  put_files_struct(struct files_struct *files)
 		BUG();
 }
 
+static struct file *
+fget_from_files(struct files_struct *files, unsigned fd)
+{
+	struct file *file;
 
-static long
-eventfd_link_ioctl(struct file *f, unsigned int ioctl, unsigned long arg)
+	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)
 {
-	void __user *argp = (void __user *) arg;
-	struct task_struct *task_target = NULL;
 	struct file *file;
-	struct files_struct *files;
+	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_copy(unsigned long arg)
+{
+	long ret = -EFAULT;
+	struct task_struct *task_target = NULL;
+	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
+	 */
+	ret = -ESRCH;
+
+	pid = find_vpid(eventfd_copy.target_pid);
+	if (pid == NULL) {
+		pr_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) {
+		pr_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 = -ESTALE;
+
+	/*
+	 * Find the file struct associated with the target fd.
+	 */
+
+	target_files = get_files_struct(task_target);
+	if (target_files == NULL) {
+		pr_info("Failed to get target files struct\n");
+		goto out_task;
+	}
+
+	ret = -EBADF;
+	target_file = fget_from_files(target_files, eventfd_copy.target_fd);
+
+	if (target_file == NULL) {
+		pr_info("Failed to get file from target pid\n");
+		goto out_target_files;
+	}
 
-	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) {
-			pr_debug("Failed to get mem ctx for target pid\n");
-			return -EFAULT;
-		}
-
-		files = get_files_struct(current);
-		if (files == NULL) {
-			pr_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) {
-			pr_debug("Failed to get file from source pid\n");
-			return 0;
-		}
-
-		/*
-		 * Release the existing eventfd in the source process
-		 */
-		spin_lock(&files->file_lock);
-		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) {
-			pr_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) {
-			pr_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;
-
-	default:
-		return -ENOIOCTLCMD;
+
+	/*
+	 * 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:
+			ret = eventfd_link_ioctl_copy(arg);
+			break;
+		default:
+			ret = -ENOIOCTLCMD;
+			break;
 	}
+
+	return ret;
 }
 
 static const struct file_operations eventfd_link_fops = {