linux-kernel - Re: [patches] VM-related fixes

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAMP5XgdmjL68qZJ8D0pNz-S5m-BauTzPyQ9Xv1SPgdZVMiZMpg@mail.gmail.com>
Date:	Mon, 5 Mar 2012 21:25:05 -0800
From:	Arve Hjønnevåg <arve@...roid.com>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-kernel@...r.kernel.org, airlied@...ux.ie,
	carsteno@...ibm.com, steiner@....com
Subject: Re: [patches] VM-related fixes

On Mon, Mar 5, 2012 at 8:10 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
> On Mon, Mar 05, 2012 at 07:38:21PM -0800, Arve Hj?nnev?g wrote:
>> > [unsolved] binder is insane, even more than usual for drivers/staging.
>> > It stores a reference to mm at open() time,
>>
>> It stores a reference to the task struct, not the mm.
>
> Equivalent, for our purposes.  OK, almost - strictly speaking that avoids
> some extra nasty scenarios with open/exec/mmap, but the real PITA comes from
> much simpler open/fork/mmap in child anyway...
>
>> > then it stores a reference to
>> > vma at mmap() time, then it cheerfully works with that ->vma assuming that
>> > ->mmap_sem on stored reference to ->mm would suffice. ?Guess what happens
>> > if somebody opens it and then forks and does mmap in child?
>>
>> mmap succeeds, but binder_update_page_range will fail when trying to
>> create a transaction? (assuming you are talking about the current
>> version of the driver)
>
> Sorry, no.
>                down_write(&mm->mmap_sem);
>                vma = proc->vma;
>                if (vma && mm != vma->vm_mm) {
> does *not* do what you seem to describe; there's nothing to protect you
> from proc->vma getting freed under you right between load from proc->vma
> and check of vma->mm.  ->mmap_sem on the right mm would prevent that,
> but this one doesn't guarantee anything.  Get preempted after the second
> line quoted above and by the time you get the timeslice back, you might
> have had munmap() done by another thread, with vma freed, its memory
> recycled, etc.
>

OK, if the memory got freed and then re-used by someone who stored a
value that matched a pointer to the mm struct that was just locked,
this check will fail to catch it. I can check against a cached vm_mm
member from mmap instead, assuming this will not change before
->close() is called. Does that sound reasonable, or is there a better
way to check this?

>> > ?Moreover, if
>> > we fork() and have child exit(), we get ->close() called on each VMA we'd
>> > copied into child.
>>
>> The driver sets VM_DONTCOPY which should avoid this.
>
> Umm...  Point taken - missed that.
>
>> > ?Since they have stored reference to vma invalidated by
>> > ->close(), that has unpleasant side effects, to put it mildly.
>>
>> The side effect of ->close() should be the same as when the process
>> that opened the driver exits. In other words, the process that
>> misbehaved is considered dying. It does not however send death
>> notifications until the driver is closed, so other processes do not
>> notice until they try to talk to it.
>>
>> > ?And yes,
>> > I realize that android userland probably doesn't do anything of that kind;
>> > fat lot of good it does us...
>> >
>>
>> What usage case do you have in mind? The binder driver uses the file
>> pointer as the binder process identifier, so you need to open and mmap
>> the driver in every process that want to use the driver. While it is
>> not common to fork a process in android, the driver should work as
>> expected if you do (assuming each process opens and mmaps the driver
>> on its own and don't share the file).
>
> Except that we can't make that assumption...

I'm not sure what your point is. You cannot expect the driver to
behave as expected if you don't follow its api. However, the driver
should prevent misbehaving processes from affecting other processes or
crashing the kernel. Apart from the situation described above, do you
know of any situation where this is not the case?

-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/