Re: [PATCH v2 1/6] mm: export vmf_insert_pfn_prot_mkwrite(), change variants to inline

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The global shared zero page can be mapped as writable by insert_pfn, leading to memory corruption.
--

--- Patch [1]: [PATCH v2 1/6] mm: export vmf_insert_pfn_prot_mkwrite(), change variants to inline ---
commit 010e09346cc93d215af7040a0cfe7fd891a1bd7d
Author: Paolo Bonzini <[email protected]>

mm: export vmf_insert_pfn_prot_mkwrite(), change variants to inline

This commit introduces vmf_insert_pfn_prot_mkwrite() and
vmf_insert_pfn_mkwrite() to allow drivers to install writable PTEs during
faults when a mapping is under write notification. It refactors the existing
vmf_insert_pfn() family of functions into inline wrappers around this new,
more flexible variant.

> diff --git a/mm/memory.c b/mm/memory.c
> index 6637c5b13c9ba..786b114d9921a 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
[ ... ]
> -vm_fault_t vmf_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
> -			unsigned long pfn, pgprot_t pgprot)
> +vm_fault_t vmf_insert_pfn_prot_mkwrite(struct vm_area_struct *vma,
> +			unsigned long addr, unsigned long pfn, pgprot_t pgprot,
> +			bool mkwrite)
>  {
[ ... ]
>  	pfnmap_setup_cachemode_pfn(pfn, &pgprot);
>  
> -	return insert_pfn(vma, addr, pfn, pgprot, false);
> -}
> -EXPORT_SYMBOL(vmf_insert_pfn_prot);
> -
> -/**
> - * vmf_insert_pfn - insert single pfn into user vma
[ ... ]
> +	return insert_pfn(vma, addr, pfn, pgprot, mkwrite);
>  }
> -EXPORT_SYMBOL(vmf_insert_pfn);
> +EXPORT_SYMBOL(vmf_insert_pfn_prot_mkwrite);

[Severity: Critical]
This is a pre-existing issue, but by allowing mkwrite to be passed directly 
through to insert_pfn(), can this result in the global shared zero page being
mapped writable?

If a driver maps the zero page via vmf_insert_pfn_mkwrite(..., true) into a
shared writable VM_PFNMAP mapping, insert_pfn() in mm/memory.c handles the
new entry like this:

	/* Ok, finally just insert the thing.. */
	entry = pte_mkspecial(pfn_pte(pfn, prot));

	if (mkwrite) {
		entry = pte_mkyoung(entry);
		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
	}

Because there is no check verifying if the passed PFN is the zero page,
maybe_mkwrite() will explicitly make the PTE writable, bypassing the zero
page protections ordinarily afforded by vm_mixed_ok(). 

If the global zero page becomes writable, unprivileged userspace could write
to it, which would cause memory corruption across the system for any process
reading uninitialized memory. Should we add a defense in insert_pfn() to
explicitly reject or write-protect the zero page when mkwrite is true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1