Re: [PATCH v2] android: binder: fix dangling pointer comparison

[Arve Hjønnevåg]

On Wed, Aug 31, 2016 at 6:22 AM, Greg Kroah-Hartman
 wrote:
> On Thu, Aug 18, 2016 at 08:00:59PM -0700, Arve Hjønnevåg wrote:
>> On Thu, Aug 18, 2016 at 7:30 PM, ZhaoJunmin Zhao(Junmin)
>>  wrote:
>> >
>> >
>> > 在 2016/8/18 23:23, Greg Kroah-Hartman 写道:
>> >>
>> >> On Tue, Aug 16, 2016 at 07:44:59PM -0700, Arve Hjønnevåg wrote:
>> >>>
>> >>> On Mon, Aug 15, 2016 at 7:58 AM, Greg Kroah-Hartman
>> >>>  wrote:
>> 
>>  On Thu, Jun 16, 2016 at 12:45:33AM +0200, Jann Horn wrote:
>> >
>> > If /dev/binder is opened and the opener process then e.g. calls execve,
>> > proc->vma_vm_mm will still point to the location of the now-freed
>> > mm_struct. If the process then calls ioctl(binder_fd, ...), the
>> > dangling
>> > proc->vma_vm_mm pointer will be compared to current->mm.
>> >
>> > Let the binder take a reference to the mm_struct to avoid this.
>> >
>> > v2: use the right refcounter
>> >
>> > Fixes: a906d6931f3ccaf7de805643190765ddd7378e27
>> 
>> 
>>  Nit, the proper way to do this is:
>> 
>>  Fixes: a906d6931f3c ("android: binder: Sanity check at binder ioctl")
>> 
>>  You can get that by doing:
>>   git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h
>>  (\"%s\")%n" a906d6931f3ccaf7de805643190765ddd7378e27
>> 
>>  Also, I'm guessing this should go to the stable kernels that include the
>>  above patch?
>> 
>> >>>
>> >>> Has this patch been tested?
>> >
>> >
>> > Hi:
>> >   The patch have tested in huawei device.
>> >
>> >>>
>> >>> Does it actually fix anything?
>> >>>
>> >>> Changes to vma_vm_mm are normally protected by the mmap_sem it
>> >>> contains, and is set to NULL in the vma->vm_ops->close call on the
>> >>> binder vma. The vma_vm_mm pointer is only used to check if the mm
>> >>> struct that the binder driver was mmapped into is the same mm struct
>> >>> that the driver just locked (in binder_update_page_range). I don't
>> >>> think change description here is accurate. If a process calls execve,
>> >>> the old vma should be closed (and vma_vm_mm cleared) before the
>> >>> mm_struct that it belongs too is freed.
>> >>
>> >>
>> >> Yeah, this whole thing feels wrong...
>> >
>> >
>> >  In huawei device,
>> >  For example: system_server process use many libs,libstagefright,
>> > Libdrmframework etc.
>> >  In these lib, binder global object exist like static sp
>> > sDeathNotifier, static sp 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.
>>
>> I not sure I follow this.
>>
>> I see now that there is another patch that got merged that reused the
>> vma_vm_mm in the ioctl itself, without holding the lock that protects
>> it in the existing code. It would probably be best to revert that
>> change too.
>
> So what other patch should I revert?  Have a git commit id?
>
> thanks,
>

a906d6931f3ccaf7de805643190765ddd7378e27

That patch appears to fix a user-space bug in the kernel, so I don't
think it is needed.

-- 
Arve Hjønnevåg
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] android: binder: fix dangling pointer comparison

[Greg Kroah-Hartman]

On Thu, Aug 18, 2016 at 08:00:59PM -0700, Arve Hjønnevåg wrote:
> On Thu, Aug 18, 2016 at 7:30 PM, ZhaoJunmin Zhao(Junmin)
>  wrote:
> >
> >
> > 在 2016/8/18 23:23, Greg Kroah-Hartman 写道:
> >>
> >> On Tue, Aug 16, 2016 at 07:44:59PM -0700, Arve Hjønnevåg wrote:
> >>>
> >>> On Mon, Aug 15, 2016 at 7:58 AM, Greg Kroah-Hartman
> >>>  wrote:
> 
>  On Thu, Jun 16, 2016 at 12:45:33AM +0200, Jann Horn wrote:
> >
> > If /dev/binder is opened and the opener process then e.g. calls execve,
> > proc->vma_vm_mm will still point to the location of the now-freed
> > mm_struct. If the process then calls ioctl(binder_fd, ...), the
> > dangling
> > proc->vma_vm_mm pointer will be compared to current->mm.
> >
> > Let the binder take a reference to the mm_struct to avoid this.
> >
> > v2: use the right refcounter
> >
> > Fixes: a906d6931f3ccaf7de805643190765ddd7378e27
> 
> 
>  Nit, the proper way to do this is:
> 
>  Fixes: a906d6931f3c ("android: binder: Sanity check at binder ioctl")
> 
>  You can get that by doing:
>   git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h
>  (\"%s\")%n" a906d6931f3ccaf7de805643190765ddd7378e27
> 
>  Also, I'm guessing this should go to the stable kernels that include the
>  above patch?
> 
> >>>
> >>> Has this patch been tested?
> >
> >
> > Hi:
> >   The patch have tested in huawei device.
> >
> >>>
> >>> Does it actually fix anything?
> >>>
> >>> Changes to vma_vm_mm are normally protected by the mmap_sem it
> >>> contains, and is set to NULL in the vma->vm_ops->close call on the
> >>> binder vma. The vma_vm_mm pointer is only used to check if the mm
> >>> struct that the binder driver was mmapped into is the same mm struct
> >>> that the driver just locked (in binder_update_page_range). I don't
> >>> think change description here is accurate. If a process calls execve,
> >>> the old vma should be closed (and vma_vm_mm cleared) before the
> >>> mm_struct that it belongs too is freed.
> >>
> >>
> >> Yeah, this whole thing feels wrong...
> >
> >
> >  In huawei device,
> >  For example: system_server process use many libs,libstagefright,
> > Libdrmframework etc.
> >  In these lib, binder global object exist like static sp
> > sDeathNotifier, static sp 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.
> 
> I not sure I follow this.
> 
> I see now that there is another patch that got merged that reused the
> vma_vm_mm in the ioctl itself, without holding the lock that protects
> it in the existing code. It would probably be best to revert that
> change too.

So what other patch should I revert?  Have a git commit id?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] android: binder: fix dangling pointer comparison

[Arve Hjønnevåg]

On Thu, Aug 18, 2016 at 7:30 PM, ZhaoJunmin Zhao(Junmin)
 wrote:
>
>
> 在 2016/8/18 23:23, Greg Kroah-Hartman 写道:
>>
>> On Tue, Aug 16, 2016 at 07:44:59PM -0700, Arve Hjønnevåg wrote:
>>>
>>> On Mon, Aug 15, 2016 at 7:58 AM, Greg Kroah-Hartman
>>>  wrote:

 On Thu, Jun 16, 2016 at 12:45:33AM +0200, Jann Horn wrote:
>
> If /dev/binder is opened and the opener process then e.g. calls execve,
> proc->vma_vm_mm will still point to the location of the now-freed
> mm_struct. If the process then calls ioctl(binder_fd, ...), the
> dangling
> proc->vma_vm_mm pointer will be compared to current->mm.
>
> Let the binder take a reference to the mm_struct to avoid this.
>
> v2: use the right refcounter
>
> Fixes: a906d6931f3ccaf7de805643190765ddd7378e27


 Nit, the proper way to do this is:

 Fixes: a906d6931f3c ("android: binder: Sanity check at binder ioctl")

 You can get that by doing:
  git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h
 (\"%s\")%n" a906d6931f3ccaf7de805643190765ddd7378e27

 Also, I'm guessing this should go to the stable kernels that include the
 above patch?

>>>
>>> Has this patch been tested?
>
>
> Hi:
>   The patch have tested in huawei device.
>
>>>
>>> Does it actually fix anything?
>>>
>>> Changes to vma_vm_mm are normally protected by the mmap_sem it
>>> contains, and is set to NULL in the vma->vm_ops->close call on the
>>> binder vma. The vma_vm_mm pointer is only used to check if the mm
>>> struct that the binder driver was mmapped into is the same mm struct
>>> that the driver just locked (in binder_update_page_range). I don't
>>> think change description here is accurate. If a process calls execve,
>>> the old vma should be closed (and vma_vm_mm cleared) before the
>>> mm_struct that it belongs too is freed.
>>
>>
>> Yeah, this whole thing feels wrong...
>
>
>  In huawei device,
>  For example: system_server process use many libs,libstagefright,
> Libdrmframework etc.
>  In these lib, binder global object exist like static sp
> sDeathNotifier, static sp 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.

I not sure I follow this.

I see now that there is another patch that got merged that reused the
vma_vm_mm in the ioctl itself, without holding the lock that protects
it in the existing code. It would probably be best to revert that
change too.

>>
>>
>> Combined with the original author's email address now bouncing, so I've
>> reverted this patch from the tree.
>>
>> thanks,
>>
>> greg k-h
>>
>> .
>>
>



-- 
Arve Hjønnevåg
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] android: binder: fix dangling pointer comparison

[ZhaoJunmin Zhao(Junmin)]

在 2016/8/18 23:23, Greg Kroah-Hartman 写道:

On Tue, Aug 16, 2016 at 07:44:59PM -0700, Arve Hjønnevåg wrote:

On Mon, Aug 15, 2016 at 7:58 AM, Greg Kroah-Hartman
 wrote:

On Thu, Jun 16, 2016 at 12:45:33AM +0200, Jann Horn wrote:

If /dev/binder is opened and the opener process then e.g. calls execve,
proc->vma_vm_mm will still point to the location of the now-freed
mm_struct. If the process then calls ioctl(binder_fd, ...), the dangling
proc->vma_vm_mm pointer will be compared to current->mm.

Let the binder take a reference to the mm_struct to avoid this.

v2: use the right refcounter

Fixes: a906d6931f3ccaf7de805643190765ddd7378e27


Nit, the proper way to do this is:

Fixes: a906d6931f3c ("android: binder: Sanity check at binder ioctl")

You can get that by doing:
 git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h 
(\"%s\")%n" a906d6931f3ccaf7de805643190765ddd7378e27

Also, I'm guessing this should go to the stable kernels that include the
above patch?



Has this patch been tested?


Hi:
  The patch have tested in huawei device.



Does it actually fix anything?

Changes to vma_vm_mm are normally protected by the mmap_sem it
contains, and is set to NULL in the vma->vm_ops->close call on the
binder vma. The vma_vm_mm pointer is only used to check if the mm
struct that the binder driver was mmapped into is the same mm struct
that the driver just locked (in binder_update_page_range). I don't
think change description here is accurate. If a process calls execve,
the old vma should be closed (and vma_vm_mm cleared) before the
mm_struct that it belongs too is freed.


Yeah, this whole thing feels wrong...


 In huawei device,
 For example: system_server process use many libs,libstagefright, 
Libdrmframework etc.
 In these lib, binder global object exist like static sp 
sDeathNotifier, static sp 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.


Combined with the original author's email address now bouncing, so I've
reverted this patch from the tree.

thanks,

greg k-h

.



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] android: binder: fix dangling pointer comparison

[Greg Kroah-Hartman]

On Tue, Aug 16, 2016 at 07:44:59PM -0700, Arve Hjønnevåg wrote:
> On Mon, Aug 15, 2016 at 7:58 AM, Greg Kroah-Hartman
>  wrote:
> > On Thu, Jun 16, 2016 at 12:45:33AM +0200, Jann Horn wrote:
> >> If /dev/binder is opened and the opener process then e.g. calls execve,
> >> proc->vma_vm_mm will still point to the location of the now-freed
> >> mm_struct. If the process then calls ioctl(binder_fd, ...), the dangling
> >> proc->vma_vm_mm pointer will be compared to current->mm.
> >>
> >> Let the binder take a reference to the mm_struct to avoid this.
> >>
> >> v2: use the right refcounter
> >>
> >> Fixes: a906d6931f3ccaf7de805643190765ddd7378e27
> >
> > Nit, the proper way to do this is:
> >
> > Fixes: a906d6931f3c ("android: binder: Sanity check at binder ioctl")
> >
> > You can get that by doing:
> > git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h 
> > (\"%s\")%n" a906d6931f3ccaf7de805643190765ddd7378e27
> >
> > Also, I'm guessing this should go to the stable kernels that include the
> > above patch?
> >
> 
> Has this patch been tested?
> 
> Does it actually fix anything?
> 
> Changes to vma_vm_mm are normally protected by the mmap_sem it
> contains, and is set to NULL in the vma->vm_ops->close call on the
> binder vma. The vma_vm_mm pointer is only used to check if the mm
> struct that the binder driver was mmapped into is the same mm struct
> that the driver just locked (in binder_update_page_range). I don't
> think change description here is accurate. If a process calls execve,
> the old vma should be closed (and vma_vm_mm cleared) before the
> mm_struct that it belongs too is freed.

Yeah, this whole thing feels wrong...

Combined with the original author's email address now bouncing, so I've
reverted this patch from the tree.

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] android: binder: fix dangling pointer comparison

[Arve Hjønnevåg]

On Mon, Aug 15, 2016 at 7:58 AM, Greg Kroah-Hartman
 wrote:
> On Thu, Jun 16, 2016 at 12:45:33AM +0200, Jann Horn wrote:
>> If /dev/binder is opened and the opener process then e.g. calls execve,
>> proc->vma_vm_mm will still point to the location of the now-freed
>> mm_struct. If the process then calls ioctl(binder_fd, ...), the dangling
>> proc->vma_vm_mm pointer will be compared to current->mm.
>>
>> Let the binder take a reference to the mm_struct to avoid this.
>>
>> v2: use the right refcounter
>>
>> Fixes: a906d6931f3ccaf7de805643190765ddd7378e27
>
> Nit, the proper way to do this is:
>
> Fixes: a906d6931f3c ("android: binder: Sanity check at binder ioctl")
>
> You can get that by doing:
> git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h 
> (\"%s\")%n" a906d6931f3ccaf7de805643190765ddd7378e27
>
> Also, I'm guessing this should go to the stable kernels that include the
> above patch?
>

Has this patch been tested?

Does it actually fix anything?

Changes to vma_vm_mm are normally protected by the mmap_sem it
contains, and is set to NULL in the vma->vm_ops->close call on the
binder vma. The vma_vm_mm pointer is only used to check if the mm
struct that the binder driver was mmapped into is the same mm struct
that the driver just locked (in binder_update_page_range). I don't
think change description here is accurate. If a process calls execve,
the old vma should be closed (and vma_vm_mm cleared) before the
mm_struct that it belongs too is freed.

-- 
Arve Hjønnevåg
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] android: binder: fix dangling pointer comparison

[Greg Kroah-Hartman]

On Thu, Jun 16, 2016 at 12:45:33AM +0200, Jann Horn wrote:
> If /dev/binder is opened and the opener process then e.g. calls execve,
> proc->vma_vm_mm will still point to the location of the now-freed
> mm_struct. If the process then calls ioctl(binder_fd, ...), the dangling
> proc->vma_vm_mm pointer will be compared to current->mm.
> 
> Let the binder take a reference to the mm_struct to avoid this.
> 
> v2: use the right refcounter
> 
> Fixes: a906d6931f3ccaf7de805643190765ddd7378e27

Nit, the proper way to do this is:

Fixes: a906d6931f3c ("android: binder: Sanity check at binder ioctl")

You can get that by doing:
git show -s --abbrev-commit --abbrev=12 --pretty=format:"%h (\"%s\")%n" 
a906d6931f3ccaf7de805643190765ddd7378e27

Also, I'm guessing this should go to the stable kernels that include the
above patch?

thanks,

greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v2] android: binder: fix dangling pointer comparison

[Chen Feng]

looks good to me.

On 2016/6/16 6:45, Jann Horn wrote:
> If /dev/binder is opened and the opener process then e.g. calls execve,
> proc->vma_vm_mm will still point to the location of the now-freed
> mm_struct. If the process then calls ioctl(binder_fd, ...), the dangling
> proc->vma_vm_mm pointer will be compared to current->mm.
> 
> Let the binder take a reference to the mm_struct to avoid this.
> 
> v2: use the right refcounter
> 
> Fixes: a906d6931f3ccaf7de805643190765ddd7378e27
> Signed-off-by: Jann Horn 

Reviewed-by: Chen Feng 
> ---
>  drivers/android/binder.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 16288e7..09fdb42 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -2962,6 +2962,7 @@ static int binder_open(struct inode *nodp, struct file 
> *filp)
>   return -ENOMEM;
>   get_task_struct(current);
>   proc->tsk = current;
> + atomic_inc(>mm->mm_count);
>   proc->vma_vm_mm = current->mm;
>   INIT_LIST_HEAD(>todo);
>   init_waitqueue_head(>wait);
> @@ -3167,6 +3168,7 @@ static void binder_deferred_release(struct binder_proc 
> *proc)
>   vfree(proc->buffer);
>   }
>  
> + mmdrop(proc->vma_vm_mm);
>   put_task_struct(proc->tsk);
>  
>   binder_debug(BINDER_DEBUG_OPEN_CLOSE,
> 

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel