From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f181.google.com (mail-wi0-f181.google.com [209.85.212.181]) by dpdk.org (Postfix) with ESMTP id E665058E4 for ; Wed, 18 Mar 2015 14:15:58 +0100 (CET) Received: by wixw10 with SMTP id w10so39559741wix.0 for ; Wed, 18 Mar 2015 06:15:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mirantis.com; s=google; h=from:to:subject:date:message-id:in-reply-to:references; bh=vkwkSAn1S3SCih2nr2bCXRW9NCQS1x3ZYL8vWLwnwWI=; b=FzlxeU8fJZJDDIkO2nl4J8nQxLcB2T/3BnFehg5gn6xo1gz7eTXOwWRGhpE1YElvEf BfZK9J5bJdN0r/mGOIwf9u7UUf08eNFKPmenUQzI6iNpoKRH986FXN5oSDZNTTaXaXuJ M3i0oPjvPuiYfvncT6Y5ZT4K692lsKiZwv1yI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:subject:date:message-id:in-reply-to :references; bh=vkwkSAn1S3SCih2nr2bCXRW9NCQS1x3ZYL8vWLwnwWI=; b=WophJA7QrHgjJNRsNuLpAGO8I4TLUn77mBsaszLDMfTeMLqGFi/VaMpZk2wBeg+TE9 Hkf5EuBCR/OMxEaVAl5Z6bnzqnIeMJaN1k9+2lRuDXHC69L9rFGv8BJ0MwbwhYqXjN46 qa6cdHNCmPnLWvimGUA2l/5kAXadlgh2XStq4tLPv7IxsJIeHGIo0UnVv949ESuIKXFw G62mF6HttN+VhyX8H6r/zR6w6WWuN3W1kbf9/qDcRv7/cJTHhquSES+A7fPmTW0i9hVc PQ+mXHkOR8+ygO4QcKG5IzIt+BiEq3151qtvNiDM2MZP/eiswEToYMz8Z1NvvCbfjmIh 8Uig== X-Gm-Message-State: ALoCoQn0Yn8K+ndjaO8ze7NUluCUSYBAgvcEB8MTfKlMiucu7Pb6/WyARYtH84IlNshX8/m3cbpZ X-Received: by 10.194.85.46 with SMTP id e14mr137777872wjz.110.1426684558831; Wed, 18 Mar 2015 06:15:58 -0700 (PDT) Received: from pboldin-pc.kha.mirantis.net ([194.213.110.67]) by mx.google.com with ESMTPSA id l9sm3165344wij.16.2015.03.18.06.15.58 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 18 Mar 2015 06:15:58 -0700 (PDT) From: Pavel Boldin To: dev@dpdk.org Date: Wed, 18 Mar 2015 15:16:11 +0200 Message-Id: <1426684571-14782-1-git-send-email-pboldin@mirantis.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1426524059-30886-1-git-send-email-pboldin+dpdk@mirantis.com> References: <1426524059-30886-1-git-send-email-pboldin+dpdk@mirantis.com> Subject: [dpdk-dev] [PATCH v2] 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: Wed, 18 Mar 2015 13:15:59 -0000 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(-) 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 = { -- 1.9.1