When a process fork a child process, we should not allow the child process use the binder which opened by parent process.
But if the binder-object creater is a thread of one process who exit, the other thread can also use this binder-object normally. We can distinguish this by the member proc->tsk->mm. If the thread exit the tsk->mm will be NULL.
proc->tsk->mm != current->mm && proc->tsk->mm
So only allow the shared mm_struct to use the same binder-object and check the existence of mm_struct.
V2: Fix compile error for error commit
Signed-off-by: Chen Feng puck.chen@hisilicon.com Signed-off-by: Wei Dong weidong2@hisilicon.com Signed-off-by: Junmin Zhao zhaojunmin@huawei.com Reviewed-by: Zhuangluan Su suzhuangluan@hisilicon.com --- drivers/android/binder.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index a39e85f..279063c 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2736,6 +2736,8 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
/*pr_info("binder_ioctl: %d:%d %x %lx\n", proc->pid, current->pid, cmd, arg);*/ + if (unlikely(proc->tsk->mm != current->mm && proc->tsk->mm)) + return -EINVAL;
trace_binder_ioctl(cmd, arg);
On Tue, 19 Jan 2016, Chen Feng wrote:
When a process fork a child process, we should not allow the child process use the binder which opened by parent process.
But if the binder-object creater is a thread of one process who exit, the other thread can also use this binder-object normally. We can distinguish this by the member proc->tsk->mm. If the thread exit the tsk->mm will be NULL.
Why does exit_mm(), the point where tsk->mm == NULL, signify the point at which the binder can now be used by other threads?
proc->tsk->mm != current->mm && proc->tsk->mm
So only allow the shared mm_struct to use the same binder-object and check the existence of mm_struct.
V2: Fix compile error for error commit
Signed-off-by: Chen Feng puck.chen@hisilicon.com Signed-off-by: Wei Dong weidong2@hisilicon.com Signed-off-by: Junmin Zhao zhaojunmin@huawei.com Reviewed-by: Zhuangluan Su suzhuangluan@hisilicon.com
drivers/android/binder.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index a39e85f..279063c 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2736,6 +2736,8 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) /*pr_info("binder_ioctl: %d:%d %x %lx\n", proc->pid, current->pid, cmd, arg);*/
- if (unlikely(proc->tsk->mm != current->mm && proc->tsk->mm))
return -EINVAL;trace_binder_ioctl(cmd, arg);
I would imagine that you would want to do READ_ONCE(proc->tsk->mm) so you are guaranteed to be testing the same value.
Hi,
On 2016/1/20 6:40, David Rientjes wrote:
On Tue, 19 Jan 2016, Chen Feng wrote:
When a process fork a child process, we should not allow the child process use the binder which opened by parent process.
But if the binder-object creater is a thread of one process who exit, the other thread can also use this binder-object normally. We can distinguish this by the member proc->tsk->mm. If the thread exit the tsk->mm will be NULL.
Why does exit_mm(), the point where tsk->mm == NULL, signify the point at which the binder can now be used by other threads?
Yes,the other threads used this shared mm for communication. A lots of APPs have this issue.
For example: system_server process use many libs,libstagefright, Libdrmframeworketc, In these lib, binder global object exist like static sp<DeathNotifier> sDeathNotifier, static sp<IDrmManagerService> sDrmManagerService; When system_sever process fork failed, the new child process call exit, which will call hook destruction function. In destruction, the parent process *binder reference count will be wrong*. To protect these exception, we need to add the code in binder.
For v2 patch, I agree with your opinion, some race condition exist in some special case. I found that the task->mm already cached at binder_mm. proc->vma_vm_mm = vma->vm_mm; I will change the condition to if (unlikely(current->mm != proc->tsk->mm)). But in some scenes, the thread do communication before the mmap called. So I add proc->vma_vm_mm = current->mm where the binder_open function.
Since the binder init process in libbinder, the fix won't produce side effect. If these are accepted, I will send a new V3 version.
proc->tsk->mm != current->mm && proc->tsk->mm
So only allow the shared mm_struct to use the same binder-object and check the existence of mm_struct.
V2: Fix compile error for error commit
Signed-off-by: Chen Feng puck.chen@hisilicon.com Signed-off-by: Wei Dong weidong2@hisilicon.com Signed-off-by: Junmin Zhao zhaojunmin@huawei.com Reviewed-by: Zhuangluan Su suzhuangluan@hisilicon.com
drivers/android/binder.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c index a39e85f..279063c 100644 --- a/drivers/android/binder.c +++ b/drivers/android/binder.c @@ -2736,6 +2736,8 @@ static long binder_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) /*pr_info("binder_ioctl: %d:%d %x %lx\n", proc->pid, current->pid, cmd, arg);*/
- if (unlikely(proc->tsk->mm != current->mm && proc->tsk->mm))
return -EINVAL;trace_binder_ioctl(cmd, arg);
I would imagine that you would want to do READ_ONCE(proc->tsk->mm) so you are guaranteed to be testing the same value.
.
-
Chen Feng -
chenfeng
-
David Rientjes