linux-kernel - Re: [PATCH] kernel/fork: stop playing lockless games for exe

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] [day] [month] [year] [list]

Date:   Mon, 14 Aug 2023 18:09:10 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Mateusz Guzik <mjguzik@...il.com>, linux-kernel@...r.kernel.org,
        torvalds@...ux-foundation.org, brauner@...nel.org,
        ebiederm@...ssion.com, akpm@...ux-foundation.org,
        linux-mm@...ck.org, koct9i@...il.com, dave@...olabs.net
Subject: Re: [PATCH] kernel/fork: stop playing lockless games for exe_file
 replacement

On 14.08.23 17:58, Oleg Nesterov wrote:
> On 08/14, David Hildenbrand wrote:
>>
>>> OK, I seem to understand... without mmap_read_lock() it is possible that
>>>
>>> 	- dup_mm_exe_file() sees mm->exe_file = old_exe_file
>>>
>>> 	- replace_mm_exe_file() does allow_write_access(old_exe_file)
>>>
>>> 	- another process does get_write_access(old_exe_file)
>>>
>>> 	- dup_mm_exe_file()->deny_write_access() fails
>>>
>>> Right?
>>
>>  From what I recall, yes.
> 
> Thanks! but then... David, this all is subjective, feel free to ignore, but
> the current code doesn't look good to me, I mean the purpose of mmap_read_lock()
> is very unclear. To me something like
> 
> 	if (old_exe_file) {
> 		/*
> 		 * Ensure that if we race with dup_mm_exe_file() and it sees
> 		 * mm->exe_file == old_exe_file deny_write_access(old_exe_file)
> 		 * can't fail after we do allow_write_access() and another task
> 		 * does get_write_access(old_exe_file).
> 		 */
> 		mmap_read_lock(mm);
> 		mmap_read_unlock(mm);
> 
> 		allow_write_access(old_exe_file);
> 		fput(old_exe_file);
> 	}
> 
> looks more understandable...

I don't particularly care about that code, and if there are ways to make 
it clearer, great.

As long as we can clarify in the patch description why we decided to go 
again the other direction (write lock) and not do what we did in 2015, 
that would be great.

-- 
Cheers,

David / dhildenb