- Dec 21, 2021
-
-
Todd Kjos authored
In 4.13, commit 74310e06 ("android: binder: Move buffer out of area shared with user space") fixed a kernel structure visibility issue. As part of that patch, sizeof(void *) was used as the buffer size for 0-length data payloads so the driver could detect abusive clients sending 0-length asynchronous transactions to a server by enforcing limits on async_free_size. Unfortunately, on the "free" side, the accounting of async_free_space did not add the sizeof(void *) back. The result was that up to 8-bytes of async_free_space were leaked on every async transaction of 8-bytes or less. These small transactions are uncommon, so this accounting issue has gone undetected for several years. The fix is to use "buffer_size" (the allocated buffer size) instead of "size" (the logical buffer size) when updating the async_free_space during the free operation. These are the same except for this corner case of asynchronous transactions with payloads < 8 bytes. Fixes: 74310e06 ("android: binder: Move buffer out of area shared with user space") Signed-off-by:
Todd Kjos <tkjos@google.com> Cc: stable@vger.kernel.org # 4.14+ Link: https://lore.kernel.org/r/20211220190150.2107077-1-tkjos@google.com Signed-off-by:
Greg Kroah-Hartman <gregkh@linuxfoundation.org>
-
- Dec 09, 2021
-
-
Eric Biggers authored
wake_up_poll() uses nr_exclusive=1, so it's not guaranteed to wake up all exclusive waiters. Yet, POLLFREE *must* wake up all waiters. epoll and aio poll are fortunately not affected by this, but it's very fragile. Thus, the new function wake_up_pollfree() has been introduced. Convert binder to use wake_up_pollfree(). Reported-by:
Linus Torvalds <torvalds@linux-foundation.org> Fixes: f5cb779b ("ANDROID: binder: remove waitqueue when thread exits.") Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/20211209010455.42744-3-ebiggers@kernel.org Signed-off-by:
Eric Biggers <ebiggers@google.com>
-
- Nov 17, 2021
-
-
Todd Kjos authored
This is a partial revert of commit 29bc22ac ("binder: use euid from cred instead of using task"). Setting sender_euid using proc->cred caused some Android system test regressions that need further investigation. It is a partial reversion because subsequent patches rely on proc->cred. Fixes: 29bc22ac ("binder: use euid from cred instead of using task") Cc: stable@vger.kernel.org # 4.4+ Acked-by:
Christian Brauner <christian.brauner@ubuntu.com> Signed-off-by:
-
- Oct 19, 2021
-
-
Todd Kjos authored
When freeing txn buffers, binder_transaction_buffer_release() attempts to detect whether the current context is the target by comparing current->group_leader to proc->tsk. This is an unreliable test. Instead explicitly pass an 'is_failure' boolean. Detecting the sender was being used as a way to tell if the transaction failed to be sent. When cleaning up after failing to send a transaction, there is no need to close the fds associated with a BINDER_TYPE_FDA object. Now 'is_failure' can be used to accurately detect this case. Fixes: 44d8047f ("binder: use standard functions to allocate fds") Cc: stable <stable@vger.kernel.org> Acked-by:
-
- Oct 15, 2021
-
-
Todd Kjos authored
Use the 'struct cred' saved at binder_open() to lookup the security ID via security_cred_getsecid(). This ensures that the security context that opened binder is the one used to generate the secctx. Cc: stable@vger.kernel.org # 5.4+ Fixes: ec74136d ("binder: create node flag to request sender's security context") Signed-off-by:
Stephen Smalley <stephen.smalley.work@gmail.com> Reported-by:
kernel test robot <lkp@intel.com> Acked-by:
Casey Schaufler <casey@schaufler-ca.com> Signed-off-by:
Paul Moore <paul@paul-moore.com>
-
Todd Kjos authored
Since binder was integrated with selinux, it has passed 'struct task_struct' associated with the binder_proc to represent the source and target of transactions. The conversion of task to SID was then done in the hook implementations. It turns out that there are race conditions which can result in an incorrect security context being used. Fix by using the 'struct cred' saved during binder_open and pass it to the selinux subsystem. Cc: stable@vger.kernel.org # 5.14 (need backport for earlier stables) Fixes: 79af7307 ("Add security hooks to binder and implement the hooks for SELinux.") Suggested-by:
Jann Horn <jannh@google.com> Signed-off-by:
-
- Oct 14, 2021
-
-
Todd Kjos authored
Save the 'struct cred' associated with a binder process at initial open to avoid potential race conditions when converting to an euid. Set a transaction's sender_euid from the 'struct cred' saved at binder_open() instead of looking up the euid from the binder proc's 'struct task'. This ensures the euid is associated with the security context that of the task that opened binder. Cc: stable@vger.kernel.org # 4.4+ Fixes: 457b9a6f ("Staging: android: add binder driver") Signed-off-by:
-
- Sep 14, 2021
-
-
Todd Kjos authored
During BC_FREE_BUFFER processing, the BINDER_TYPE_FDA object cleanup may close 1 or more fds. The close operations are completed using the task work mechanism -- which means the thread needs to return to userspace or the file object may never be dereferenced -- which can lead to hung processes. Force the binder thread back to userspace if an fd is closed during BC_FREE_BUFFER handling. Fixes: 80cd7956 ("binder: fix use-after-free due to ksys_close() during fdget()") Cc: stable <stable@vger.kernel.org> Reviewed-by:
Martijn Coenen <maco@android.com> Acked-by:
-
Li Li authored
Currently cgroup freezer is used to freeze the application threads, and BINDER_FREEZE is used to freeze the corresponding binder interface. There's already a mechanism in ioctl(BINDER_FREEZE) to wait for any existing transactions to drain out before actually freezing the binder interface. But freezing an app requires 2 steps, freezing the binder interface with ioctl(BINDER_FREEZE) and then freezing the application main threads with cgroupfs. This is not an atomic operation. The following race issue might happen. 1) Binder interface is frozen by ioctl(BINDER_FREEZE); 2) Main thread A initiates a new sync binder transaction to process B; 3) Main thread A is frozen by "echo 1 > cgroup.freeze"; 4) The response from process B reaches the frozen thread, which will unexpectedly fail. This patch provides a mechanism to check if there's any new pending transaction happening between ioctl(BINDER_FREEZE) and freezing the main thread. If there's any, the main thread freezing operation can be rolled back to finish the pending transaction. Furthermore, the response might reach the binder driver before the rollback actually happens. That will still cause failed transaction. As the other process doesn't wait for another response of the response, the response transaction failure can be fixed by treating the response transaction like an oneway/async one, allowing it to reach the frozen thread. And it will be consumed when the thread gets unfrozen later. NOTE: This patch reuses the existing definition of struct binder_frozen_status_info but expands the bit assignments of __u32 member sync_recv. To ensure backward compatibility, bit 0 of sync_recv still indicates there's an outstanding sync binder transaction. This patch adds new information to bit 1 of sync_recv, indicating the binder transaction happens exactly when there's a race. If an existing userspace app runs on a new kernel, a sync binder call will set bit 0 of sync_recv so ioctl(BINDER_GET_FROZEN_INFO) still return the expected value (true). The app just doesn't check bit 1 intentionally so it doesn't have the ability to tell if there's a race. This behavior is aligned with what happens on an old kernel which doesn't set bit 1 at all. A new userspace app can 1) check bit 0 to know if there's a sync binder transaction happened when being frozen - same as before; and 2) check bit 1 to know if that sync binder transaction happened exactly when there's a race - a new information for rollback decision. the same time, confirmed the pending transactions succeeded. Fixes: 432ff1e9 ("binder: BINDER_FREEZE ioctl") Acked-by:
Li Li <dualli@google.com> Test: stress test with apps being frozen and initiating binder calls at Link: https://lore.kernel.org/r/20210910164210.2282716-2-dualli@chromium.org Signed-off-by:
-
- Aug 03, 2021
-
-
Ramji Jiyani authored
In the case of a failed transaction, only the thread and process id are logged. Add the handle info for the reference to the target node in user error log to aid debugging. Acked-by:
Ramji Jiyani <ramjiyani@google.com> Link: https://lore.kernel.org/r/20210802220446.1938347-1-ramjiyani@google.com Signed-off-by:
-
- Jul 21, 2021
-
-
Carlos Llamas authored
Provide userspace with a mechanism to discover features supported by the binder driver to refrain from using any unsupported ones in the first place. Starting with "oneway_spam_detection" only new features are to be listed under binderfs and all previous ones are assumed to be supported. Assuming an instance of binderfs has been mounted at /dev/binderfs, binder feature files can be found under /dev/binderfs/features/. Usage example: $ mkdir /dev/binderfs $ mount -t binder binder /dev/binderfs $ cat /dev/binderfs/features/oneway_spam_detection 1 Acked-by:
Carlos Llamas <cmllamas@google.com> Link: https://lore.kernel.org/r/20210715031805.1725878-1-cmllamas@google.com Signed-off-by:
-
- May 13, 2021
-
-
luca020400 authored
All the other ioctl paths return EFAULT in case the copy_from_user/copy_to_user call fails, make oneway spam detection follow the same paradigm. Fixes: a7dc1e6f ("binder: tell userspace to dump current backtrace when detected oneway spamming") Acked-by:
Luca Stefani <luca.stefani.ge1@gmail.com> Link: https://lore.kernel.org/r/20210506193726.45118-1-luca.stefani.ge1@gmail.com Signed-off-by:
-
- Apr 10, 2021
-
-
Hang Lu authored
When async binder buffer got exhausted, some normal oneway transactions will also be discarded and may cause system or application failures. By that time, the binder debug information we dump may not be relevant to the root cause. And this issue is difficult to debug if without the backtrace of the thread sending spam. This change will send BR_ONEWAY_SPAM_SUSPECT to userspace when oneway spamming is detected, request to dump current backtrace. Oneway spamming will be reported only once when exceeding the threshold (target process dips below 80% of its oneway space, and current process is responsible for either more than 50 transactions, or more than 50% of the oneway space). And the detection will restart when the async buffer has returned to a healthy state. Acked-by:
Hang Lu <hangl@codeaurora.org> Link: https://lore.kernel.org/r/1617961246-4502-3-git-send-email-hangl@codeaurora.org Signed-off-by:
-
Hang Lu authored
Add BR_FROZEN_REPLY in binder_return_strings to support stat function. Fixes: ae28c1be ("binder: BINDER_GET_FROZEN_INFO ioctl") Acked-by:
-
- Mar 24, 2021
-
-
Marco Ballesio authored
User space needs to know if binder transactions occurred to frozen processes. Introduce a new BINDER_GET_FROZEN ioctl and keep track of transactions occurring to frozen proceses. Signed-off-by:
Marco Ballesio <balejs@google.com> Signed-off-by:
-
Marco Ballesio authored
when interrupted by a signal, binder_wait_for_work currently returns -ERESTARTSYS. This error code isn't propagated to user space, but a way to handle interruption due to signals must be provided to code using this API. Replace this instance of -ERESTARTSYS with -EINTR, which is propagated to user space. binder_wait_for_work Signed-off-by:
-
Marco Ballesio authored
Frozen tasks can't process binder transactions, so a way is required to inform transmitting ends of communication failures due to the frozen state of their receiving counterparts. Additionally, races are possible between transitions to frozen state and binder transactions enqueued to a specific process. Implement BINDER_FREEZE ioctl for user space to inform the binder driver about the intention to freeze or unfreeze a process. When the ioctl is called, block the caller until any pending binder transactions toward the target process are flushed. Return an error to transactions to processes marked as frozen. Co-developed-by:
-
- Mar 22, 2021
-
-
Paul Moore authored
Of the three LSMs that implement the security_task_getsecid() LSM hook, all three LSMs provide the task's objective security credentials. This turns out to be unfortunate as most of the hook's callers seem to expect the task's subjective credentials, although a small handful of callers do correctly expect the objective credentials. This patch is the first step towards fixing the problem: it splits the existing security_task_getsecid() hook into two variants, one for the subjective creds, one for the objective creds. void security_task_getsecid_subj(struct task_struct *p, u32 *secid); void security_task_getsecid_obj(struct task_struct *p, u32 *secid); While this patch does fix all of the callers to use the correct variant, in order to keep this patch focused on the callers and to ease review, the LSMs continue to use the same implementation for both hooks. The net effect is that this patch should not change the behavior of the kernel in any way, it will be up to the latter LSM specific patches in this series to change the hook implementations and return the correct credentials. Acked-by: Mimi Zohar <zohar@linux.ibm.com> (IMA) Acked-by:
Richard Guy Briggs <rgb@redhat.com> Signed-off-by:
-
- Jan 24, 2021
-
-
Christian Brauner authored
Extend some inode methods with an additional user namespace argument. A filesystem that is aware of idmapped mounts will receive the user namespace the mount has been marked with. This can be used for additional permission checking and also to enable filesystems to translate between uids and gids if they need to. We have implemented all relevant helpers in earlier patches. As requested we simply extend the exisiting inode method instead of introducing new ones. This is a little more code churn but it's mostly mechanical and doesnt't leave us with additional inode methods. Link: https://lore.kernel.org/r/20210121131959.646623-25-christian.brauner@ubuntu.com Cc: Christoph Hellwig <hch@lst.de> Cc: David Howells <dhowells@redhat.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: linux-fsdevel@vger.kernel.org Reviewed-by:
Christoph Hellwig <hch@lst.de> Signed-off-by:
-
- Dec 10, 2020
-
-
Eric W. Biederman authored
The function close_fd_get_file is explicitly a variant of __close_fd[1]. Now that __close_fd has been renamed close_fd, rename close_fd_get_file to be consistent with close_fd. When __alloc_fd, __close_fd and __fd_install were introduced the double underscore indicated that the function took a struct files_struct parameter. The function __close_fd_get_file never has so the naming has always been inconsistent. This just cleans things up so there are not any lingering mentions or references __close_fd left in the code. [1] 80cd7956 ("binder: fix use-after-free due to ksys_close() during fdget()") Link: https://lkml.kernel.org/r/20201120231441.29911-23-ebiederm@xmission.com Signed-off-by:
Eric W. Biederman <ebiederm@xmission.com>
-
- Dec 09, 2020
-
-
Todd Kjos authored
Add a per-transaction flag to indicate that the buffer must be cleared when the transaction is complete to prevent copies of sensitive data from being preserved in memory. Signed-off-by:
-
- Nov 11, 2020
-
-
Frankie.Chang authored
Since the original trace_binder_transaction_received cannot precisely present the real finished time of transaction, adding a trace_binder_txn_latency_free at the point of free transaction may be more close to it. Signed-off-by:
Frankie.Chang <Frankie.Chang@mediatek.com> Acked-by:
-
Frankie.Chang authored
Moving all structs to header file makes module more extendable, and makes all these structs to be defined in the same file. Signed-off-by:
-
- Nov 09, 2020
-
-
Zhang Qilong authored
Depending on the context, the error return value here (extra_buffers_size < added_size) should be negative. Acked-by:
Zhang Qilong <zhangqilong3@huawei.com> Link: https://lore.kernel.org/r/20201026110314.135481-1-zhangqilong3@huawei.com Signed-off-by:
-
Andrew Bridges authored
Fixed a coding style issue. Signed-off-by:
Andrew Bridges <andrew@slova.app> Link: https://lore.kernel.org/r/20201027225655.650922-1-andrew@slova.app Signed-off-by:
-
- Oct 17, 2020
-
-
Jens Axboe authored
A previous commit changed the notification mode from true/false to an int, allowing notify-no, notify-yes, or signal-notify. This was backwards compatible in the sense that any existing true/false user would translate to either 0 (on notification sent) or 1, the latter which mapped to TWA_RESUME. TWA_SIGNAL was assigned a value of 2. Clean this up properly, and define a proper enum for the notification mode. Now we have: - TWA_NONE. This is 0, same as before the original change, meaning no notification requested. - TWA_RESUME. This is 1, same as before the original change, meaning that we use TIF_NOTIFY_RESUME. - TWA_SIGNAL. This uses TIF_SIGPENDING/JOBCTL_TASK_WORK for the notification. Clean up all the callers, switching their 0/1/false/true to using the appropriate TWA_* mode for notifications. Fixes: e91b4816 ("task_work: teach task_work_add() to do signal_wake_up()") Reviewed-by:
Thomas Gleixner <tglx@linutronix.de> Signed-off-by:
Jens Axboe <axboe@kernel.dk>
-
- Oct 10, 2020
-
-
Todd Kjos authored
When releasing a thread todo list when tearing down a binder_proc, the following race was possible which could result in a use-after-free: 1. Thread 1: enter binder_release_work from binder_thread_release 2. Thread 2: binder_update_ref_for_handle() -> binder_dec_node_ilocked() 3. Thread 2: dec nodeA --> 0 (will free node) 4. Thread 1: ACQ inner_proc_lock 5. Thread 2: block on inner_proc_lock 6. Thread 1: dequeue work (BINDER_WORK_NODE, part of nodeA) 7. Thread 1: REL inner_proc_lock 8. Thread 2: ACQ inner_proc_lock 9. Thread 2: todo list cleanup, but work was already dequeued 10. Thread 2: free node 11. Thread 2: REL inner_proc_lock 12. Thread 1: deref w->type (UAF) The problem was that for a BINDER_WORK_NODE, the binder_work element must not be accessed after releasing the inner_proc_lock while processing the todo list elements since another thread might be handling a deref on the node containing the binder_work element leading to the node being freed. Signed-off-by:
-
- Oct 05, 2020
-
-
Liu Shixin authored
Simplify the return expression. Acked-by:
Liu Shixin <liushixin2@huawei.com> Link: https://lore.kernel.org/r/20200929015216.1829946-1-liushixin2@huawei.com Signed-off-by:
-
- Sep 16, 2020
-
-
Colin Ian King authored
The pointer n is being initialized with a value that is never read and it is being updated later with a new value. The initialization is redundant and can be removed. Acked-by:
Colin Ian King <colin.king@canonical.com> Link: https://lore.kernel.org/r/20200910151221.751464-1-colin.king@canonical.com Signed-off-by:
-
- Sep 03, 2020
-
-
Martijn Coenen authored
The most common cause of the binder transaction buffer filling up is a client rapidly firing oneway transactions into a process, before it has a chance to handle them. Yet the root cause of this is often hard to debug, because either the system or the app will stop, and by that time binder debug information we dump in bugreports is no longer relevant. This change warns as soon as a process dips below 80% of its oneway space (less than 100kB available in the configuration), when any one process is responsible for either more than 50 transactions, or more than 50% of the oneway space. Signed-off-by:
-
Wei Yongjun authored
The sparse tool complains as follows: drivers/android/binderfs.c:66:32: warning: symbol 'binderfs_fs_parameters' was not declared. Should it be static? This variable is not used outside of binderfs.c, so this commit marks it static. Fixes: 095cf502 ("binderfs: port to new mount api") Reported-by:
Hulk Robot <hulkci@huawei.com> Signed-off-by:
Wei Yongjun <weiyongjun1@huawei.com> Acked-by:
-
YangHui authored
The function name should is binder_alloc_new_buf() Signed-off-by:
YangHui <yanghui.def@gmail.com> Reviewed-by:
-
Jann Horn authored
While binder transactions with the same binder_proc as sender and recipient are forbidden, transactions with the same task_struct as sender and recipient are possible (even though currently there is a weird check in binder_transaction() that rejects them in the target==0 case). Therefore, task_struct identities can't be used to distinguish whether the caller is running in the context of the sender or the recipient. Since I see no easy way to make this WARN_ON() useful and correct, let's just remove it. Fixes: 44d8047f ("binder: use standard functions to allocate fds") Reported-by:
<syzbot+e113a0b970b7b3f394ba@syzkaller.appspotmail.com> Acked-by:
-
- Jul 29, 2020
-
-
Mrinal Pandey authored
C source files should have `//` as SPDX comment and not `/**/`. Fix this by running checkpatch on the file. Signed-off-by:
Mrinal Pandey <mrinalmni@gmail.com> Link: https://lore.kernel.org/r/20200724131449.zvjutbemg3vqhrzh@mrinalpandey Signed-off-by:
-
Mrinal Pandey authored
Add a blank line after variable declarations as suggested by checkpatch. Signed-off-by:
-
Mrinal Pandey authored
Remove braces for both if and else block as suggested by checkpatch. Signed-off-by:
-
Mrinal Pandey authored
Remove the unnecessary else branch after return statement as suggested by checkpatch. Signed-off-by:
-
Mrinal Pandey authored
Add a blank line after variable declarations as suggested by checkpatch. Signed-off-by:
-
Jann Horn authored
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. <https://syzkaller.appspot.com/bug?extid=09e05aba06723a94d43d >. 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: 457b9a6f ("Staging: android: add binder driver") Acked-by:
-
- Jul 23, 2020
-
-
Tetsuo Handa authored
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 a1b2289c ("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 <syzbot+1068f09c44d151250c33@syzkaller.appspotmail.com> Reported-by:
syzbot <syzbot+e5344baa319c9a96edec@syzkaller.appspotmail.com> Signed-off-by:
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Reviewed-by:
Michal Hocko <mhocko@suse.com> Acked-by:
-
