[PATCH RESEND] android: binder: Sanity check at binder ioctl

[PATCH RESEND] android: binder: Sanity check at binder ioctl

[Chen Feng]

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.

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

Re: [PATCH RESEND] android: binder: Sanity check at binder ioctl

[Greg KH]

On Tue, Jan 19, 2016 at 11:45:36AM +0800, 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.
> 
> 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.
> 
> 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(+)

Why resend?  What changed from the previous version?

greg k-h

Re: [PATCH RESEND] android: binder: Sanity check at binder ioctl

[chenfeng]

On 2016/1/19 15:33, Greg KH wrote:
> On Tue, Jan 19, 2016 at 11:45:36AM +0800, 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.
>>
>> 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.
>>
>> 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(+)
> 
> Why resend?  What changed from the previous version?
> 
My fault, it's my error commit.
mistake the current->mm with current->tsk->mm.
The robot-compile finds out this error.
> greg k-h
> 
> .
> 

Re: [PATCH RESEND] android: binder: Sanity check at binder ioctl

[Greg KH]

On Tue, Jan 19, 2016 at 03:49:27PM +0800, chenfeng wrote:
> 
> 
> On 2016/1/19 15:33, Greg KH wrote:
> > On Tue, Jan 19, 2016 at 11:45:36AM +0800, 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.
> >>
> >> 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.
> >>
> >> 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(+)
> > 
> > Why resend?  What changed from the previous version?
> > 
> My fault, it's my error commit.
> mistake the current->mm with current->tsk->mm.
> The robot-compile finds out this error.

Then please make it a 'v2' patch, and say what you changed, otherwise
I'll assume it's identical to the first patch you sent in.

And how did you test the first patch if it couldn't even compile?

greg k-h

Re: [PATCH RESEND] android: binder: Sanity check at binder ioctl

[chenfeng]

On 2016/1/19 16:35, Greg KH wrote:
> On Tue, Jan 19, 2016 at 03:49:27PM +0800, chenfeng wrote:
>>
>>
>> On 2016/1/19 15:33, Greg KH wrote:
>>> On Tue, Jan 19, 2016 at 11:45:36AM +0800, 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.
>>>>
>>>> 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.
>>>>
>>>> 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(+)
>>>
>>> Why resend?  What changed from the previous version?
>>>
>> My fault, it's my error commit.
>> mistake the current->mm with current->tsk->mm.
>> The robot-compile finds out this error.
> 
> Then please make it a 'v2' patch, and say what you changed, otherwise
> I'll assume it's identical to the first patch you sent in.
> 
> And how did you test the first patch if it couldn't even compile?
> 
It works well on our platform with hundreds of mobile phone.
Since our working branch is not mainline,and the patch is send for mainline
review.

I made a mistake while making the patch. I will send a new V2 for this patch.
Thanks!

> greg k-h
> 
> .
>