linux-kernel - Re: [PATCH v2] android: binder: Sanity check at binder ioctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.10.1601191436370.7346@chino.kir.corp.google.com>
Date:	Tue, 19 Jan 2016 14:40:59 -0800 (PST)
From:	David Rientjes <rientjes@...gle.com>
To:	Chen Feng <puck.chen@...ilicon.com>
cc:	gregkh@...uxfoundation.org, arve@...roid.com,
	riandrews@...roid.com, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org, yudongbin@...ilicon.com,
	saberlily.xia@...ilicon.com, suzhuangluan@...ilicon.com,
	kong.kongxinwei@...ilicon.com, xuyiping@...ilicon.com,
	z.liuxinliang@...ilicon.com, weidong2@...ilicon.com,
	w.f@...wei.com, puck.chen@...mail.com, shimingxing@...ilicon.com,
	oliver.fu@...ilicon.com, albert.lubing@...ilicon.com,
	chenxiang9@...wei.com, liuzixing@...wei.com,
	haojian.zhuang@...aro.org, zhaojunmin@...wei.com,
	wangjing6@...wei.com, john.stultz@...aro.org,
	qijiwen@...ilicon.com, peter.panshilin@...ilicon.com,
	dan.zhao@...ilicon.com, linuxarm@...wei.com, dev@...ts.96boards.org
Subject: Re: [PATCH v2] android: binder: Sanity check at binder ioctl

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@...ilicon.com>
> Signed-off-by: Wei  Dong <weidong2@...ilicon.com>
> Signed-off-by: Junmin Zhao <zhaojunmin@...wei.com>
> Reviewed-by: Zhuangluan Su <suzhuangluan@...ilicon.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.