kernel/linux.git/drivers/android, branch v5.4.61

binder: Prevent context manager from incrementing ref 0

2020-08-11T13:33:35+00:00

commit 4b836a1426cb0f1ef2a6e211d7e553221594f8fc upstream. Binder is designed such that a binder_proc never has references to itself. If this rule is violated, memory corruption can occur when a process sends a transaction to itself; see e.g. . There is a remaining edgecase through which such a transaction-to-self can still occur from the context of a task with BINDER_SET_CONTEXT_MGR access: - task A opens /dev/binder twice, creating binder_proc instances P1 and P2 - P1 becomes context manager - P2 calls ACQUIRE on the magic handle 0, allocating index 0 in its handle table - P1 dies (by closing the /dev/binder fd and waiting a bit) - P2 becomes context manager - P2 calls ACQUIRE on the magic handle 0, allocating index 1 in its handle table [this triggers a warning: "binder: 1974:1974 tried to acquire reference to desc 0, got 1 instead"] - task B opens /dev/binder once, creating binder_proc instance P3 - P3 calls P2 (via magic handle 0) with (void*)1 as argument (two-way transaction) - P2 receives the handle and uses it to call P3 (two-way transaction) - P3 calls P2 (via magic handle 0) (two-way transaction) - P2 calls P2 (via handle 1) (two-way transaction) And then, if P2 does *NOT* accept the incoming transaction work, but instead closes the binder fd, we get a crash. Solve it by preventing the context manager from using ACQUIRE on ref 0. There shouldn't be any legitimate reason for the context manager to do that. Additionally, print a warning if someone manages to find another way to trigger a transaction-to-self bug in the future. Cc: stable@vger.kernel.org Fixes: 457b9a6f09f0 ("Staging: android: add binder driver") Acked-by: Todd Kjos Signed-off-by: Jann Horn Reviewed-by: Martijn Coenen Link: https://lore.kernel.org/r/20200727120424.1627555-1-jannh@google.com Signed-off-by: Greg Kroah-Hartman

binder: Don't use mmput() from shrinker function.

2020-07-29T08:18:41+00:00

commit f867c771f98891841c217fa8459244ed0dd28921 upstream. syzbot is reporting that mmput() from shrinker function has a risk of deadlock [1], for delayed_uprobe_add() from update_ref_ctr() calls kzalloc(GFP_KERNEL) with delayed_uprobe_lock held, and uprobe_clear_state() from __mmput() also holds delayed_uprobe_lock. Commit a1b2289cef92ef0e ("android: binder: drop lru lock in isolate callback") replaced mmput() with mmput_async() in order to avoid sleeping with spinlock held. But this patch replaces mmput() with mmput_async() in order not to start __mmput() from shrinker context. [1] https://syzkaller.appspot.com/bug?id=bc9e7303f537c41b2b0cc2dfcea3fc42964c2d45 Reported-by: syzbot Reported-by: syzbot Signed-off-by: Tetsuo Handa Reviewed-by: Michal Hocko Acked-by: Todd Kjos Acked-by: Christian Brauner Cc: stable Link: https://lore.kernel.org/r/4ba9adb2-43f5-2de0-22de-f6075c1fab50@i-love.sakura.ne.jp Signed-off-by: Greg Kroah-Hartman

binder: fix null deref of proc->context

2020-06-30T19:36:48+00:00

commit d35d3660e065b69fdb8bf512f3d899f350afce52 upstream. The binder driver makes the assumption proc->context pointer is invariant after initialization (as documented in the kerneldoc header for struct proc). However, in commit f0fe2c0f050d ("binder: prevent UAF for binderfs devices II") proc->context is set to NULL during binder_deferred_release(). Another proc was in the middle of setting up a transaction to the dying process and crashed on a NULL pointer deref on "context" which is a local set to &proc->context: new_ref->data.desc = (node == context->binder_context_mgr_node) ? 0 : 1; Here's the stack: [ 5237.855435] Call trace: [ 5237.855441] binder_get_ref_for_node_olocked+0x100/0x2ec [ 5237.855446] binder_inc_ref_for_node+0x140/0x280 [ 5237.855451] binder_translate_binder+0x1d0/0x388 [ 5237.855456] binder_transaction+0x2228/0x3730 [ 5237.855461] binder_thread_write+0x640/0x25bc [ 5237.855466] binder_ioctl_write_read+0xb0/0x464 [ 5237.855471] binder_ioctl+0x30c/0x96c [ 5237.855477] do_vfs_ioctl+0x3e0/0x700 [ 5237.855482] __arm64_sys_ioctl+0x78/0xa4 [ 5237.855488] el0_svc_common+0xb4/0x194 [ 5237.855493] el0_svc_handler+0x74/0x98 [ 5237.855497] el0_svc+0x8/0xc The fix is to move the kfree of the binder_device to binder_free_proc() so the binder_device is freed when we know there are no references remaining on the binder_proc. Fixes: f0fe2c0f050d ("binder: prevent UAF for binderfs devices II") Acked-by: Christian Brauner Signed-off-by: Todd Kjos Cc: stable Link: https://lore.kernel.org/r/20200622200715.114382-1-tkjos@google.com Signed-off-by: Greg Kroah-Hartman

binderfs: use refcount for binder control devices too

2020-03-25T07:25:50+00:00

[ Upstream commit 211b64e4b5b6bd5fdc19cd525c2cc9a90e6b0ec9 ] 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 Cc: stable@vger.kernel.org Signed-off-by: Christian Brauner Acked-by: Todd Kjos Link: https://lore.kernel.org/r/20200311105309.1742827-1-christian.brauner@ubuntu.com Signed-off-by: Greg Kroah-Hartman Signed-off-by: Sasha Levin

binder: prevent UAF for binderfs devices II

2020-03-12T12:00:17+00:00

commit f0fe2c0f050d31babcad7d65f1d550d462a40064 upstream. 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 Acked-by: Todd Kjos Link: https://lore.kernel.org/r/20200303164340.670054-1-christian.brauner@ubuntu.com Signed-off-by: Greg Kroah-Hartman Signed-off-by: Greg Kroah-Hartman

binder: prevent UAF for binderfs devices

2020-03-12T12:00:17+00:00

commit 2669b8b0c798fbe1a31d49e07aa33233d469ad9b upstream. 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 holding an additional reference to the inode that is only released once the workqueue is done cleaning up struct binder_proc. This is an easy alternative to introducing separate refcounting on struct binder_device which we can always do later if it becomes necessary. This is an alternative fix to 51d8a7eca677 ("binder: prevent UAF read in print_binder_transaction_log_entry()"). Fixes: 3ad20fe393b3 ("binder: implement binderfs") 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 Acked-by: Todd Kjos Signed-off-by: Greg Kroah-Hartman Signed-off-by: Greg Kroah-Hartman

binder: fix log spam for existing debugfs file creation.

2020-02-01T09:34:35+00:00

commit eb143f8756e77c8fcfc4d574922ae9efd3a43ca9 upstream. Since commit 43e23b6c0b01 ("debugfs: log errors when something goes wrong") debugfs logs attempts to create existing files. However binder attempts to create multiple debugfs files with the same name when a single PID has multiple contexts, this leads to log spamming during an Android boot (17 such messages during boot on my system). Fix this by checking if we already know the PID and only create the debugfs entry for the first context per PID. Do the same thing for binderfs for symmetry. Signed-off-by: Martin Fuzzey Acked-by: Todd Kjos Fixes: 43e23b6c0b01 ("debugfs: log errors when something goes wrong") Cc: stable Link: https://lore.kernel.org/r/1578671054-5982-1-git-send-email-martin.fuzzey@flowbird.group Signed-off-by: Greg Kroah-Hartman

binder: fix incorrect calculation for num_valid

2019-12-17T18:55:33+00:00

commit 16981742717b04644a41052570fb502682a315d2 upstream. For BINDER_TYPE_PTR and BINDER_TYPE_FDA transactions, the num_valid local was calculated incorrectly causing the range check in binder_validate_ptr() to miss out-of-bounds offsets. Fixes: bde4a19fc04f ("binder: use userspace pointer as base of buffer space") Signed-off-by: Todd Kjos Cc: stable Link: https://lore.kernel.org/r/20191213202531.55010-1-tkjos@google.com Signed-off-by: Greg Kroah-Hartman

binder: Handle start==NULL in binder_update_page_range()

2019-12-13T07:43:25+00:00

commit 2a9edd056ed4fbf9d2e797c3fc06335af35bccc4 upstream. The old loop wouldn't stop when reaching `start` if `start==NULL`, instead continuing backwards to index -1 and crashing. Luckily you need to be highly privileged to map things at NULL, so it's not a big problem. Fix it by adjusting the loop so that the loop variable is always in bounds. This patch is deliberately minimal to simplify backporting, but IMO this function could use a refactor. The jump labels in the second loop body are horrible (the error gotos should be jumping to free_range instead), and both loops would look nicer if they just iterated upwards through indices. And the up_read()+mmput() shouldn't be duplicated like that. Cc: stable@vger.kernel.org Fixes: 457b9a6f09f0 ("Staging: android: add binder driver") Signed-off-by: Jann Horn Acked-by: Christian Brauner Link: https://lore.kernel.org/r/20191018205631.248274-3-jannh@google.com Signed-off-by: Greg Kroah-Hartman

binder: Prevent repeated use of ->mmap() via NULL mapping

2019-12-13T07:43:25+00:00

commit a7a74d7ff55a0c657bc46238b050460b9eacea95 upstream. binder_alloc_mmap_handler() attempts to detect the use of ->mmap() on a binder_proc whose binder_alloc has already been initialized by checking whether alloc->buffer is non-zero. Before commit 880211667b20 ("binder: remove kernel vm_area for buffer space"), alloc->buffer was a kernel mapping address, which is always non-zero, but since that commit, it is a userspace mapping address. A sufficiently privileged user can map /dev/binder at NULL, tricking binder_alloc_mmap_handler() into assuming that the binder_proc has not been mapped yet. This leads to memory unsafety. Luckily, no context on Android has such privileges, and on a typical Linux desktop system, you need to be root to do that. Fix it by using the mapping size instead of the mapping address to distinguish the mapped case. A valid VMA can't have size zero. Fixes: 880211667b20 ("binder: remove kernel vm_area for buffer space") Cc: stable@vger.kernel.org Signed-off-by: Jann Horn Acked-by: Christian Brauner Link: https://lore.kernel.org/r/20191018205631.248274-2-jannh@google.com Signed-off-by: Greg Kroah-Hartman