On Sat, Jun 18, 2016 at 02:12:32PM +0200, Jann Horn wrote:
> On Sat, Jun 18, 2016 at 11:19 AM, ZhaoJunmin Zhao(Junmin)
> <zhaojunmin@xxxxxxxxxx> wrote:
> > 在 2016/6/16 6:39, Jann Horn 写道:
> >> On Thu, Jun 16, 2016 at 12:31 AM, Arve Hjønnevåg <arve@xxxxxxxxxxx> wrote:
> >>> On Wed, Jun 15, 2016 at 3:09 PM, Jann Horn <jannh@xxxxxxxxxx> 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.
> >>>>
> >>>> In the current code, the BUG_ON() will never trigger; it's just there
> >>>> in case someone changes the conditions under which get_task_mm() can
> >>>> fail. Just WARN_ON() wouldn't work because of the mmput(), and I don't
> >>>> want to complicate the code with a dead error handling code path.
> >>>> (If the BUG_ON() is unacceptable, I'd replace the get_task_mm() with
> >>>> a direct refcount increment or just omit the BUG_ON().)
> >>>>
> >>>> This shouldn't cause additional memory usage in a well-behaved system
> >>>> because you're not supposed to keep binder fds open over fork() or
> >>>> execve().
> >>>>
> [...]
> >>> Does this work? In the past, holding a reference to a task's mm while
> >>> the driver is open would prevent vma close which in turn would prevent
> >>> release of the fd.
> >> Ah, right, mm_struct has two refcounters. I think that should be
> >> atomic_inc(¤t->mm->mm_count) / mmdrop() instead.
> > Hi Jann Horn:
> > In android platform, the libbinder use the O_CLOEXEC flag:
> > static int open_driver()
> > {
> > int fd = open("/dev/binder", O_RDWR | O_CLOEXEC);
> > ...
> > return fd;
> > }
> > So I think you don't need to consider the case:
> > "If the process then calls ioctl(binder_fd, ...), the dangling
> > proc->vma_vm_mm pointer will be compared to current->mm."
>
> It is the kernel's job to behave in a reasonable way even if
> unprivileged userland code does weird things it isn't supposed to do.
The binder userspace code is "privileged" and "trusted". If it were
not, bad bad things would happen to your machine. That's why you should
never run binder on a machine that is not an Android machine.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
