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: [email protected]
Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
Signed-off-by: Jann Horn <[email protected]>
---
sending again because I forgot to CC LKML the first time... sorry about
the spam.
drivers/android/binder.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f50c5f182bb5..cac65ff3a257 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -2982,6 +2982,12 @@ static void binder_transaction(struct binder_proc *proc,
goto err_dead_binder;
}
e->to_node = target_node->debug_id;
+ if (WARN_ON(proc == target_proc)) {
+ return_error = BR_FAILED_REPLY;
+ return_error_param = -EINVAL;
+ return_error_line = __LINE__;
+ goto err_invalid_target_handle;
+ }
if (security_binder_transaction(proc->tsk,
target_proc->tsk) < 0) {
return_error = BR_FAILED_REPLY;
@@ -3635,10 +3641,16 @@ static int binder_thread_write(struct binder_proc *proc,
struct binder_node *ctx_mgr_node;
mutex_lock(&context->context_mgr_node_lock);
ctx_mgr_node = context->binder_context_mgr_node;
- if (ctx_mgr_node)
+ if (ctx_mgr_node) {
+ if (ctx_mgr_node->proc == proc) {
+ binder_user_error("%d:%d context manager tried to acquire desc 0\n");
+ mutex_unlock(&context->context_mgr_node_lock);
+ return -EINVAL;
+ }
ret = binder_inc_ref_for_node(
proc, ctx_mgr_node,
strong, NULL, &rdata);
+ }
mutex_unlock(&context->context_mgr_node_lock);
}
if (ret)
base-commit: 2a89b99f580371b86ae9bafd6cbeccd3bfab524a
--
2.27.0.389.gc38d7665816-goog
On Thu, Jul 9, 2020 at 3:40 PM Jann Horn <[email protected]> wrote:
>
> 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: [email protected]
> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> Signed-off-by: Jann Horn <[email protected]>
Nice catch.
Acked-by: Todd Kjos <[email protected]>
> ---
> sending again because I forgot to CC LKML the first time... sorry about
> the spam.
>
> drivers/android/binder.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f50c5f182bb5..cac65ff3a257 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2982,6 +2982,12 @@ static void binder_transaction(struct binder_proc *proc,
> goto err_dead_binder;
> }
> e->to_node = target_node->debug_id;
> + if (WARN_ON(proc == target_proc)) {
> + return_error = BR_FAILED_REPLY;
> + return_error_param = -EINVAL;
> + return_error_line = __LINE__;
> + goto err_invalid_target_handle;
> + }
> if (security_binder_transaction(proc->tsk,
> target_proc->tsk) < 0) {
> return_error = BR_FAILED_REPLY;
> @@ -3635,10 +3641,16 @@ static int binder_thread_write(struct binder_proc *proc,
> struct binder_node *ctx_mgr_node;
> mutex_lock(&context->context_mgr_node_lock);
> ctx_mgr_node = context->binder_context_mgr_node;
> - if (ctx_mgr_node)
> + if (ctx_mgr_node) {
> + if (ctx_mgr_node->proc == proc) {
> + binder_user_error("%d:%d context manager tried to acquire desc 0\n");
> + mutex_unlock(&context->context_mgr_node_lock);
> + return -EINVAL;
> + }
> ret = binder_inc_ref_for_node(
> proc, ctx_mgr_node,
> strong, NULL, &rdata);
> + }
> mutex_unlock(&context->context_mgr_node_lock);
> }
> if (ret)
>
> base-commit: 2a89b99f580371b86ae9bafd6cbeccd3bfab524a
> --
> 2.27.0.389.gc38d7665816-goog
>
On Fri, Jul 10, 2020 at 12:39:48AM +0200, Jann Horn wrote:
> 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: [email protected]
> Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> Signed-off-by: Jann Horn <[email protected]>
> Acked-by: Todd Kjos <[email protected]>
> ---
> sending again because I forgot to CC LKML the first time... sorry about
> the spam.
This spits out a bunch of warnings when built, how did it work on your
end?
drivers/android/binder.c: In function ‘binder_thread_write’:
./include/linux/kern_levels.h:5:18: warning: format ‘%d’ expects a matching ‘int’ argument [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
./include/linux/printk.h:507:10: note: in definition of macro ‘printk_ratelimited’
507 | printk(fmt, ##__VA_ARGS__); \
| ^~~
./include/linux/kern_levels.h:14:19: note: in expansion of macro ‘KERN_SOH’
14 | #define KERN_INFO KERN_SOH "6" /* informational */
| ^~~~~~~~
./include/linux/printk.h:527:21: note: in expansion of macro ‘KERN_INFO’
527 | printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~
drivers/android/binder.c:147:4: note: in expansion of macro ‘pr_info_ratelimited’
147 | pr_info_ratelimited(x); \
| ^~~~~~~~~~~~~~~~~~~
drivers/android/binder.c:3646:7: note: in expansion of macro ‘binder_user_error’
3646 | binder_user_error("%d:%d context manager tried to acquire desc 0\n");
| ^~~~~~~~~~~~~~~~~
./include/linux/kern_levels.h:5:18: warning: format ‘%d’ expects a matching ‘int’ argument [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
./include/linux/printk.h:507:10: note: in definition of macro ‘printk_ratelimited’
507 | printk(fmt, ##__VA_ARGS__); \
| ^~~
./include/linux/kern_levels.h:14:19: note: in expansion of macro ‘KERN_SOH’
14 | #define KERN_INFO KERN_SOH "6" /* informational */
| ^~~~~~~~
./include/linux/printk.h:527:21: note: in expansion of macro ‘KERN_INFO’
527 | printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~
drivers/android/binder.c:147:4: note: in expansion of macro ‘pr_info_ratelimited’
147 | pr_info_ratelimited(x); \
| ^~~~~~~~~~~~~~~~~~~
drivers/android/binder.c:3646:7: note: in expansion of macro ‘binder_user_error’
3646 | binder_user_error("%d:%d context manager tried to acquire desc 0\n");
| ^~~~~~~~~~~~~~~~~
thanks,
greg k-h
On Fri, Jul 10, 2020 at 8:54 AM Greg Kroah-Hartman
<[email protected]> wrote:
> On Fri, Jul 10, 2020 at 12:39:48AM +0200, Jann Horn wrote:
> > 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: [email protected]
> > Fixes: 457b9a6f09f0 ("Staging: android: add binder driver")
> > Signed-off-by: Jann Horn <[email protected]>
> > Acked-by: Todd Kjos <[email protected]>
> > ---
> > sending again because I forgot to CC LKML the first time... sorry about
> > the spam.
>
> This spits out a bunch of warnings when built, how did it work on your
> end?
... by creating the patch file before fixing the warnings. Sigh. I'll
send the proper patch as v2...