Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Todd Kjos <tkjos@android.com>
To:     tkjos@google.com, gregkh@linuxfoundation.org, arve@android.com,
        devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
        maco@google.com, joel@joelfernandes.org
Cc:     kernel-team@android.com, Jann Horn <jannh@google.com>,
        Martijn Coenen <maco@android.com>
Subject: [PATCH] binder: fix use-after-free due to fdget() optimization
Date:   Mon,  3 Dec 2018 12:24:57 -0800
Message-Id: <20181203202457.228972-4-tkjos@google.com>
In-Reply-To: <20181203202457.228972-1-tkjos@google.com>
References: <20181203202457.228972-1-tkjos@google.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

44d8047f1d87a ("binder: use standard functions to allocate fds")
exposed a pre-existing issue in the binder driver.

fdget() is used in ksys_ioctl() as a performance optimization.
One of the rules associated with fdget() is that ksys_close() must
not be called between the fdget() and the fdput(). There is a case
where this requirement is not met in the binder driver (and possibly
other drivers) which results in the reference count dropping to 0
when the device is still in use. This can result in use-after-free
or other issues.

This was observed with the following sequence of events:

Task A and task B are connected via binder; task A has /dev/binder open at
file descriptor number X. Both tasks are single-threaded.

1. task B sends a binder message with a file descriptor array
   (BINDER_TYPE_FDA) containing one file descriptor to task A
2. task A reads the binder message with the translated file
   descriptor number Y
3. task A uses dup2(X, Y) to overwrite file descriptor Y with
   the /dev/binder file
4. task A unmaps the userspace binder memory mapping; the reference
   count on task A's /dev/binder is now 2
5. task A closes file descriptor X; the reference count on task
   A's /dev/binder is now 1
6. task A forks off a child, task C, duplicating the file descriptor
   table; the reference count on task A's /dev/binder is now 2
7. task A invokes the BC_FREE_BUFFER command on file descriptor X
   to release the incoming binder message
8. fdget() in ksys_ioctl() suppresses the reference count increment,
   since the file descriptor table is not shared
9. the BC_FREE_BUFFER handler removes the file descriptor table
   entry for X and decrements the reference count of task A's
   /dev/binder file to 1
10.task C calls close(X), which drops the reference count of
   task A's /dev/binder to 0 and frees it
11.task A continues processing of the ioctl and accesses some
   property of e.g. the binder_proc => KASAN-detectable UAF

Fixed by using get_file() / fput() in binder_ioctl().

Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Todd Kjos <tkjos@google.com>
Acked-by: Martijn Coenen <maco@android.com>
---
 drivers/android/binder.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c525b164d39d2..cbaef3b0d3078 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4774,6 +4774,13 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	unsigned int size = _IOC_SIZE(cmd);
 	void __user *ubuf = (void __user *)arg;
 
+	/*
+	 * Need a reference on filp since ksys_close() could
+	 * be called on binder fd and the fdget() used in
+	 * ksys_ioctl() might have optimized out the reference.
+	 */
+	get_file(filp);
+
 	/*pr_info("binder_ioctl: %d:%d %x %lx\n",
 			proc->pid, current->pid, cmd, arg);*/
 
@@ -4885,6 +4892,7 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		pr_info("%d:%d ioctl %x %lx returned %d\n", proc->pid, current->pid, cmd, arg, ret);
 err_unlocked:
 	trace_binder_ioctl_done(ret);
+	fput(filp);
 	return ret;
 }
 
-- 
2.20.0.rc1.387.gf8505762e3-goog