| Message ID | 1453194241-55201-1-git-send-email-puck.chen@hisilicon.com |
|---|---|
| State | New |
| Headers | show |
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. > > . >
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);