tree 7c3e3e84539c86101547f0ca82aac1782e15145e
parent 4ab9e06059b32f94f76e46f5a5f9bcb914be328e
author Christian Brauner <christian.brauner@ubuntu.com> 1583253820 +0100
committer Tim Zimmermann <tim@linux4.de> 1705121288 +0100
gpgsig -----BEGIN PGP SIGNATURE-----
 
 iQIzBAABCgAdFiEEuwYxAyE4TRnuhLgPbcIaY/gZxe8FAmWiFggACgkQbcIaY/gZ
 xe+gTBAAkdNao859ieXn3o+cnu5ExbC4NHULXmT0HkBYVTVuzX9fPjcdQFCJeJNQ
 MjjbObmZ9OvwVCE6aELl8J8XKe9m5ftyygf7pqnCsItFDSUVszECNv6glEqNWOP/
 ogVfPl0EaDXdEcxmTpLRebYv21XfKSkqf1X00IqT5qd2XCYcj3OQ75nFo4+oMyH3
 gf9US3AmcgGxzlURHqxGLeJJQV9Ek4KH3XzD3MRDY6PeoG+bj8CrqEsqjZAd5gQn
 EEYmjUIS4YuZuJK4IIyg8DPGLkp7uCl/OqR398dgWe+RhcUf+v1+1t7gmNdBJbyh
 L5UpqV/BDYn0UgQCbG7TlMC3G4KqCaPzQiFdOKufEqiHiAKHeYqSc+deVdh+h6pR
 L1OV0rSQ8fvoTefq3d86k5EYywQzhIgPS8wJIsxNPRZdOF61RwFgKPtL94AbExaM
 Ibhqe1MYGoxNm4G6Y2vodlj7r/YjCj8gXFxoD3DDqfQ/+owfCMg2FPYYQQ+1DDcj
 PKggT5mDcHJNL3SE/Nvf+gooxmYQogV+6ZpldxJRS4zEyCjoGx4j9txDXGOgmZ7r
 iJZQlFDSGfKWsphkQSXENPm4LXegO0a1QhMKbHwjJ5uOdouvS6536qcOOjlMs3ej
 20Q842pMzsGsIegyf8KecT0F/f5OFMmuF3ZjSqEvQBNLBL6UVmo=
 =IJEO
 -----END PGP SIGNATURE-----

UPSTREAM: binder: prevent UAF for binderfs devices II

This is a necessary follow up to the first fix I proposed and we merged
in 2669b8b0c79 ("binder: prevent UAF for binderfs devices"). I have been
overly optimistic that the simple fix I proposed would work. But alas,
ihold() + iput() won't work since the inodes won't survive the
destruction of the superblock.
So all we get with my prior fix is a different race with a tinier
race-window but it doesn't solve the issue. Fwiw, the problem lies with
generic_shutdown_super(). It even has this cozy Al-style comment:

          if (!list_empty(&sb->s_inodes)) {
                  printk("VFS: Busy inodes after unmount of %s. "
                     "Self-destruct in 5 seconds.  Have a nice day...\n",
                     sb->s_id);
          }

On binder_release(), binder_defer_work(proc, BINDER_DEFERRED_RELEASE) is
called which punts the actual cleanup operation to a workqueue. At some
point, binder_deferred_func() will be called which will end up calling
binder_deferred_release() which will retrieve and cleanup the
binder_context attach to this struct binder_proc.

If we trace back where this binder_context is attached to binder_proc we
see that it is set in binder_open() and is taken from the struct
binder_device it is associated with. This obviously assumes that the
struct binder_device that context is attached to is _never_ freed. While
that might be true for devtmpfs binder devices it is most certainly
wrong for binderfs binder devices.

So, assume binder_open() is called on a binderfs binder devices. We now
stash away the struct binder_context associated with that struct
binder_devices:
	proc->context = &binder_dev->context;
	/* binderfs stashes devices in i_private */
	if (is_binderfs_device(nodp)) {
		binder_dev = nodp->i_private;
		info = nodp->i_sb->s_fs_info;
		binder_binderfs_dir_entry_proc = info->proc_log_dir;
	} else {
	.
	.
	.
	proc->context = &binder_dev->context;

Now let's assume that the binderfs instance for that binder devices is
shutdown via umount() and/or the mount namespace associated with it goes
away. As long as there is still an fd open for that binderfs binder
device things are fine. But let's assume we now close the last fd for
that binderfs binder device. Now binder_release() is called and punts to
the workqueue. Assume that the workqueue has quite a bit of stuff to do
and doesn't get to cleaning up the struct binder_proc and the associated
struct binder_context with it for that binderfs binder device right
away. In the meantime, the VFS is killing the super block and is
ultimately calling sb->evict_inode() which means it will call
binderfs_evict_inode() which does:

static void binderfs_evict_inode(struct inode *inode)
{
	struct binder_device *device = inode->i_private;
	struct binderfs_info *info = BINDERFS_I(inode);

	clear_inode(inode);

	if (!S_ISCHR(inode->i_mode) || !device)
		return;

	mutex_lock(&binderfs_minors_mutex);
	--info->device_count;
	ida_free(&binderfs_minors, device->miscdev.minor);
	mutex_unlock(&binderfs_minors_mutex);

	kfree(device->context.name);
	kfree(device);
}

thereby freeing the struct binder_device including struct
binder_context.

Now the workqueue finally has time to get around to cleaning up struct
binder_proc and is now trying to access the associate struct
binder_context. Since it's already freed it will OOPs.

Fix this by introducing a refounct on binder devices.

This is an alternative fix to 51d8a7eca677 ("binder: prevent UAF read in
print_binder_transaction_log_entry()").

Fixes: 3ad20fe393b3 ("binder: implement binderfs")
Fixes: 2669b8b0c798 ("binder: prevent UAF for binderfs devices")
Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
Related: 51d8a7eca677 ("binder: prevent UAF read in print_binder_transaction_log_entry()")
Cc: stable@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
Acked-by: Todd Kjos <tkjos@google.com>
Link: https://lore.kernel.org/r/20200303164340.670054-1-christian.brauner@ubuntu.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
(cherry picked from commit f0fe2c0f050d31babcad7d65f1d550d462a40064)
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>
Change-Id: I54a6c910002bf1077ba0c34c48fb96f4ffbf012e
Signed-off-by: Greg Kroah-Hartman <gregkh@google.com>