From a79f41ed9786b75ebe75e52295ad54049b8551b6 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Wed, 15 Aug 2012 18:23:36 -0400 Subject: binder: don't allow mmap() by process other than proc->tsk we really shouldn't do get_files_struct() on a different process and use it to modify the sucker later on. Signed-off-by: Al Viro --- drivers/staging/android/binder.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'drivers/staging') diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c index 574e99210c36..b9a534c46aac 100644 --- a/drivers/staging/android/binder.c +++ b/drivers/staging/android/binder.c @@ -2793,6 +2793,9 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma) const char *failure_string; struct binder_buffer *buffer; + if (proc->tsk != current) + return -EINVAL; + if ((vma->vm_end - vma->vm_start) > SZ_4M) vma->vm_end = vma->vm_start + SZ_4M; @@ -2857,7 +2860,7 @@ static int binder_mmap(struct file *filp, struct vm_area_struct *vma) binder_insert_free_buffer(proc, buffer); proc->free_async_space = proc->buffer_size / 2; barrier(); - proc->files = get_files_struct(proc->tsk); + proc->files = get_files_struct(current); proc->vma = vma; proc->vma_vm_mm = vma->vm_mm; -- cgit v1.2.3 From dcfadfa4ec5a12404a99ad6426871a6b03a62b37 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Sun, 12 Aug 2012 17:27:30 -0400 Subject: new helper: __alloc_fd() Essentially, alloc_fd() in a files_struct we own a reference to. Most of the time wanting to use it is a sign of lousy API design (such as android/binder). It's *not* a general-purpose interface; better that than open-coding its guts, but again, playing with other process' descriptor table is a sign of bad design. Signed-off-by: Al Viro --- drivers/staging/android/binder.c | 59 ++++------------------------------------ 1 file changed, 5 insertions(+), 54 deletions(-) (limited to 'drivers/staging') diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c index b9a534c46aac..4946d282a35c 100644 --- a/drivers/staging/android/binder.c +++ b/drivers/staging/android/binder.c @@ -362,71 +362,22 @@ struct binder_transaction { static void binder_defer_work(struct binder_proc *proc, enum binder_deferred_state defer); -/* - * copied from get_unused_fd_flags - */ int task_get_unused_fd_flags(struct binder_proc *proc, int flags) { struct files_struct *files = proc->files; - int fd, error; - struct fdtable *fdt; unsigned long rlim_cur; unsigned long irqs; if (files == NULL) return -ESRCH; - error = -EMFILE; - spin_lock(&files->file_lock); + if (!lock_task_sighand(proc->tsk, &irqs)) + return -EMFILE; -repeat: - fdt = files_fdtable(files); - fd = find_next_zero_bit(fdt->open_fds, fdt->max_fds, files->next_fd); - - /* - * N.B. For clone tasks sharing a files structure, this test - * will limit the total number of files that can be opened. - */ - rlim_cur = 0; - if (lock_task_sighand(proc->tsk, &irqs)) { - rlim_cur = proc->tsk->signal->rlim[RLIMIT_NOFILE].rlim_cur; - unlock_task_sighand(proc->tsk, &irqs); - } - if (fd >= rlim_cur) - goto out; - - /* Do we need to expand the fd array or fd set? */ - error = expand_files(files, fd); - if (error < 0) - goto out; - - if (error) { - /* - * If we needed to expand the fs array we - * might have blocked - try again. - */ - error = -EMFILE; - goto repeat; - } - - __set_open_fd(fd, fdt); - if (flags & O_CLOEXEC) - __set_close_on_exec(fd, fdt); - else - __clear_close_on_exec(fd, fdt); - files->next_fd = fd + 1; -#if 1 - /* Sanity check */ - if (fdt->fd[fd] != NULL) { - pr_warn("get_unused_fd: slot %d not NULL!\n", fd); - fdt->fd[fd] = NULL; - } -#endif - error = fd; + rlim_cur = task_rlimit(proc->tsk, RLIMIT_NOFILE); + unlock_task_sighand(proc->tsk, &irqs); -out: - spin_unlock(&files->file_lock); - return error; + return __alloc_fd(files, 0, rlim_cur, flags); } /* -- cgit v1.2.3 From f869e8a7f753e3fd43d6483e796774776f645edb Mon Sep 17 00:00:00 2001 From: Al Viro Date: Wed, 15 Aug 2012 21:06:33 -0400 Subject: expose a low-level variant of fd_install() for binder Similar situation to that of __alloc_fd(); do not use unless you really have to. You should not touch any descriptor table other than your own; it's a sure sign of a really bad API design. As with __alloc_fd(), you *must* use a first-class reference to struct files_struct; something obtained by get_files_struct(some task) (let alone direct task->files) will not do. It must be either current->files, or obtained by get_files_struct(current) by the owner of that sucker and given to you. Signed-off-by: Al Viro --- drivers/staging/android/binder.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) (limited to 'drivers/staging') diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c index 4946d282a35c..9e1a98a360d4 100644 --- a/drivers/staging/android/binder.c +++ b/drivers/staging/android/binder.c @@ -386,17 +386,8 @@ int task_get_unused_fd_flags(struct binder_proc *proc, int flags) static void task_fd_install( struct binder_proc *proc, unsigned int fd, struct file *file) { - struct files_struct *files = proc->files; - struct fdtable *fdt; - - if (files == NULL) - return; - - spin_lock(&files->file_lock); - fdt = files_fdtable(files); - BUG_ON(fdt->fd[fd] != NULL); - rcu_assign_pointer(fdt->fd[fd], file); - spin_unlock(&files->file_lock); + if (proc->files) + __fd_install(proc->files, fd, file); } /* -- cgit v1.2.3 From 483ce1d4b8c3b82bc9c9a1dd9dbc44f50b3aaf5a Mon Sep 17 00:00:00 2001 From: Al Viro Date: Sun, 19 Aug 2012 12:04:24 -0400 Subject: take descriptor-related part of close() to file.c Signed-off-by: Al Viro --- drivers/staging/android/binder.c | 34 ++-------------------------------- 1 file changed, 2 insertions(+), 32 deletions(-) (limited to 'drivers/staging') diff --git a/drivers/staging/android/binder.c b/drivers/staging/android/binder.c index 9e1a98a360d4..f71d624995ea 100644 --- a/drivers/staging/android/binder.c +++ b/drivers/staging/android/binder.c @@ -390,43 +390,17 @@ static void task_fd_install( __fd_install(proc->files, fd, file); } -/* - * copied from __put_unused_fd in open.c - */ -static void __put_unused_fd(struct files_struct *files, unsigned int fd) -{ - struct fdtable *fdt = files_fdtable(files); - __clear_open_fd(fd, fdt); - if (fd < files->next_fd) - files->next_fd = fd; -} - /* * copied from sys_close */ static long task_close_fd(struct binder_proc *proc, unsigned int fd) { - struct file *filp; - struct files_struct *files = proc->files; - struct fdtable *fdt; int retval; - if (files == NULL) + if (proc->files == NULL) return -ESRCH; - spin_lock(&files->file_lock); - fdt = files_fdtable(files); - if (fd >= fdt->max_fds) - goto out_unlock; - filp = fdt->fd[fd]; - if (!filp) - goto out_unlock; - rcu_assign_pointer(fdt->fd[fd], NULL); - __clear_close_on_exec(fd, fdt); - __put_unused_fd(files, fd); - spin_unlock(&files->file_lock); - retval = filp_close(filp, files); - + retval = __close_fd(proc->files, fd); /* can't restart close syscall because file table entry was cleared */ if (unlikely(retval == -ERESTARTSYS || retval == -ERESTARTNOINTR || @@ -435,10 +409,6 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd) retval = -EINTR; return retval; - -out_unlock: - spin_unlock(&files->file_lock); - return -EBADF; } static void binder_set_nice(long nice) -- cgit v1.2.3 From cb0942b81249798e15c3f04eee2946ef543e8115 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Mon, 27 Aug 2012 14:48:26 -0400 Subject: make get_file() return its argument simplifies a bunch of callers... Signed-off-by: Al Viro --- drivers/staging/omapdrm/omap_gem.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'drivers/staging') diff --git a/drivers/staging/omapdrm/omap_gem.c b/drivers/staging/omapdrm/omap_gem.c index 3a0d035a9e03..2a6bb7f9ee68 100644 --- a/drivers/staging/omapdrm/omap_gem.c +++ b/drivers/staging/omapdrm/omap_gem.c @@ -566,9 +566,8 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj, * in particular in the case of mmap'd dmabufs) */ fput(vma->vm_file); - get_file(obj->filp); vma->vm_pgoff = 0; - vma->vm_file = obj->filp; + vma->vm_file = get_file(obj->filp); vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); } -- cgit v1.2.3