linux-kernel - [PATCH] binderfs: use refcount for binder control devices too

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20200311105309.1742827-1-christian.brauner@ubuntu.com>
Date:   Wed, 11 Mar 2020 11:53:09 +0100
From:   Christian Brauner <christian.brauner@...ntu.com>
To:     naresh.kamboju@...aro.org
Cc:     ard.biesheuvel@...aro.org, ardb@...nel.org,
        christian.brauner@...ntu.com, gregkh@...uxfoundation.org,
        john.stultz@...aro.org, keescook@...omium.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
        lkft-triage@...ts.linaro.org, shuah@...nel.org, tkjos@...gle.com,
        stable@...r.kernel.org
Subject: [PATCH] binderfs: use refcount for binder control devices too

Binderfs binder-control devices are cleaned up via binderfs_evict_inode
too() which will use refcount_dec_and_test(). However, we missed to set
the refcount for binderfs binder-control devices and so we underflowed
when the binderfs instance got unmounted. Pretty obvious oversight and
should have been part of the more general UAF fix. The good news is that
having test cases (suprisingly) helps.

Technically, we could detect that we're about to cleanup the
binder-control dentry in binderfs_evict_inode() and then simply clean it
up. But that makes the assumption that the binder driver itself will
never make use of a binderfs binder-control device after the binderfs
instance it belongs to has been unmounted and the superblock for it been
destroyed. While it is unlikely to ever come to this let's be on the
safe side. Performance-wise this also really doesn't matter since the
binder-control device is only every really when creating the binderfs
filesystem or creating additional binder devices. Both operations are
pretty rare.

Fixes: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II")
Link: https://lore.kernel.org/r/CA+G9fYusdfg7PMfC9Xce-xLT7NiyKSbgojpK35GOm=Pf9jXXrA@mail.gmail.com
Reported-by: Naresh Kamboju <naresh.kamboju@...aro.org>
Cc: stable@...r.kernel.org
Cc: Todd Kjos <tkjos@...gle.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
---
 drivers/android/binderfs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
index 110e41f920c2..f303106b3362 100644
--- a/drivers/android/binderfs.c
+++ b/drivers/android/binderfs.c
@@ -448,6 +448,7 @@ static int binderfs_binder_ctl_create(struct super_block *sb)
 	inode->i_uid = info->root_uid;
 	inode->i_gid = info->root_gid;

+	refcount_set(&device->ref, 1);
 	device->binderfs_inode = inode;
 	device->miscdev.minor = minor;

base-commit: 2c523b344dfa65a3738e7039832044aa133c75fb
-- 
2.25.1