[PATCH] android: binder: fix dangling pointer comparison

[Jann Horn]

On Sat, Jun 18, 2016 at 11:19 AM, ZhaoJunmin Zhao(Junmin)
<zhaojunmin at huawei.com> wrote:
> 在 2016/6/16 6:39, Jann Horn 写道:
>> On Thu, Jun 16, 2016 at 12:31 AM, Arve Hjønnevåg <arve at android.com> wrote:
>>> On Wed, Jun 15, 2016 at 3:09 PM, Jann Horn <jannh at google.com> 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(&current->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 dangling pointer comparison, for example, potentially allows an
attacker to figure out things about the kernel's heap layout,
potentially aiding in exploitation of other issues.

I'm not saying that this is a big security issue. But given how easy
it is to fix this behavior, I don't see the point in leaving the code
this way. In my opinion, good code should *never* use potentially
freed pointers for *anything*, even if the impact seems low.