* [PATCH v2] android: binder: Sanity check at binder ioctl @ 2016-01-19 9:04 Chen Feng 2016-01-19 22:40 ` David Rientjes 0 siblings, 1 reply; 3+ messages in thread From: Chen Feng @ 2016-01-19 9:04 UTC (permalink / raw) To: puck.chen, gregkh, arve, riandrews, devel, linux-kernel, yudongbin, saberlily.xia, suzhuangluan, kong.kongxinwei, xuyiping, z.liuxinliang, weidong2, w.f, puck.chen, shimingxing, oliver.fu, albert.lubing, chenxiang9, liuzixing, haojian.zhuang, zhaojunmin, wangjing6, rientjes, john.stultz Cc: qijiwen, peter.panshilin, dan.zhao, linuxarm, dev 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); -- 1.9.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] android: binder: Sanity check at binder ioctl 2016-01-19 9:04 [PATCH v2] android: binder: Sanity check at binder ioctl Chen Feng @ 2016-01-19 22:40 ` David Rientjes 2016-01-28 7:08 ` chenfeng 0 siblings, 1 reply; 3+ messages in thread From: David Rientjes @ 2016-01-19 22:40 UTC (permalink / raw) To: Chen Feng Cc: gregkh, arve, riandrews, devel, linux-kernel, yudongbin, saberlily.xia, suzhuangluan, kong.kongxinwei, xuyiping, z.liuxinliang, weidong2, w.f, puck.chen, shimingxing, oliver.fu, albert.lubing, chenxiang9, liuzixing, haojian.zhuang, zhaojunmin, wangjing6, john.stultz, qijiwen, peter.panshilin, dan.zhao, linuxarm, dev 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. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] android: binder: Sanity check at binder ioctl 2016-01-19 22:40 ` David Rientjes @ 2016-01-28 7:08 ` chenfeng 0 siblings, 0 replies; 3+ messages in thread From: chenfeng @ 2016-01-28 7:08 UTC (permalink / raw) To: David Rientjes Cc: gregkh, arve, riandrews, devel, linux-kernel, yudongbin, saberlily.xia, suzhuangluan, kong.kongxinwei, xuyiping, z.liuxinliang, weidong2, w.f, puck.chen, shimingxing, oliver.fu, albert.lubing, chenxiang9, liuzixing, haojian.zhuang, zhaojunmin, wangjing6, john.stultz, qijiwen, peter.panshilin, dan.zhao, linuxarm, dev 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. > > . > ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-01-28 7:10 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-19 9:04 [PATCH v2] android: binder: Sanity check at binder ioctl Chen Feng 2016-01-19 22:40 ` David Rientjes 2016-01-28 7:08 ` chenfeng
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.