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: <Z5J_FLry1C2d3BKv@x1n>
Date: Thu, 23 Jan 2025 12:40:36 -0500
From: Peter Xu <peterx@...hat.com>
To: Ryan Roberts <ryan.roberts@....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 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, 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,

-- 
Peter Xu