linux-kernel - Re: [PATCH v1 1/2] mm: Clear uffd-wp PTE/PMD state on mremap()

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]

Message-ID: <738fc4af-cbee-4d14-a9eb-0932ecc3371f@arm.com>
Date: Fri, 24 Jan 2025 09:28:42 +0000
From: Ryan Roberts <ryan.roberts@....com>
To: Peter Xu <peterx@...hat.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
 Muchun Song <muchun.song@...ux.dev>,
 "Liam R. Howlett" <Liam.Howlett@...cle.com>,
 Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
 Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
 Shuah Khan <shuah@...nel.org>, David Hildenbrand <david@...hat.com>,
 Mikołaj Lenczewski <miko.lenczewski@....com>,
 Mark Rutland <mark.rutland@....com>, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org, linux-kselftest@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH v1 1/2] mm: Clear uffd-wp PTE/PMD state on mremap()

On 23/01/2025 17:40, Peter Xu wrote:
> On Thu, Jan 23, 2025 at 02:38:46PM +0000, Ryan Roberts wrote:
>>> @@ -5470,7 +5471,18 @@ static void move_huge_pte(struct vm_area_struct *vma, unsigned long old_addr,
>>>  		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>>>  
>>>  	pte = huge_ptep_get_and_clear(mm, old_addr, src_pte);
>>> -	set_huge_pte_at(mm, new_addr, dst_pte, pte, sz);
>>> +
>>> +	if (need_clear_uffd_wp && pte_marker_uffd_wp(pte))
>>> +		huge_pte_clear(mm, new_addr, dst_pte, sz);
>>
>> This is checking if the source huge_pte is a uffd-wp marker and clearing the
>> destination if so. The destination could have previously held arbitrary valid
>> mappings, I guess?
> 
> I think it should be all cleared.  I didn't check all mremap paths, but for
> MREMAP_FIXED at least there should be:
> 
> 	if (flags & MREMAP_FIXED) {
> 		/*
> 		 * In mremap_to().
> 		 * VMA is moved to dst address, and munmap dst first.
> 		 * do_munmap will check if dst is sealed.
> 		 */
> 		ret = do_munmap(mm, new_addr, new_len, uf_unmap_early);
> 		if (ret)
> 			goto out;
> 	}
> 
> It also doesn't sound right to leave anything in dest range, 

Yes, of course. And the loop over the old ptes actually skips doing anything if
the old pte is none without doing any operations on the new pte; so it must be
none by default. OK panic over! I just need to fix the arm64 issue I raised in
the other email. I'm going to send a bunch of fixes/improvements in that area.

Thanks,
Ryan

> e.g. if there
> can be any leftover dest ptes in move_page_tables(), then it means
> HPAGE_P[MU]D won't work, as they install huge entries directly.  For that I
> do see a hint in the comment too in that path:
> 
> move_normal_pud():
> 	/*
> 	 * The destination pud shouldn't be established, free_pgtables()
> 	 * should have released it.
> 	 */
> 	if (WARN_ON_ONCE(!pud_none(*new_pud)))
> 		return false;
> 
> PMD path has similar implications.
> 
> Thanks,
>