Re: [PATCH] mm: pull writability check to follow_pfnmap_start()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM fails to return KVM_PFN_ERR_RO_FAULT on write faults to read-only PFNMAP VMAs, breaking MMIO emulation.
--

--- Patch [1]: [PATCH] mm: pull writability check to follow_pfnmap_start() ---
commit 22ed2320251e3d4544366aecf373c7a819e369d9
Author: Paolo Bonzini <[email protected]>

mm: pull writability check to follow_pfnmap_start()

This commit pulls the writability check directly into follow_pfnmap_start()
via a new write_fault parameter. This allows the function to check
permissions and return an error directly if the mapping is not writable
when a write fault is requested.

Link: https://lore.kernel.org/kvm/CAAiTLFU1ALsDoJoKW3d9bUvv990AozAoX=bEHmfnG54qyBAHFg@mail.gmail.com/

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c

[ ... ]

>  static int hva_to_pfn_remapped(struct vm_area_struct *vma,
>  			       struct kvm_follow_pfn *kfp, kvm_pfn_t *p_pfn)
>  {

[ ... ]

>  		bool unlocked = false;
>  		r = fixup_user_fault(current->mm, kfp->hva,
> -				     (write_fault ? FAULT_FLAG_WRITE : 0),
> +				     (args.write_fault ? FAULT_FLAG_WRITE : 0),
>  				     &unlocked);
>  		if (unlocked)
>  			return -EAGAIN;
>  		if (r)
>  			return r;

[Severity: High]
Will this early return bypass the new KVM_PFN_ERR_RO_FAULT fallback logic
added below?

When handling a write fault on a read-only PFNMAP memory region,
args.write_fault is true, causing the first follow_pfnmap_start() to return
-EFAULT. This correctly triggers a call to fixup_user_fault() with
FAULT_FLAG_WRITE.

However, since the underlying VMA lacks VM_WRITE, fixup_user_fault() will
also return -EFAULT. The early error check here will immediately propagate
that -EFAULT back to the caller.

>  
>  		r = follow_pfnmap_start(&args);
> -		if (r)
> +		if (r) {
> +			if (r == -EFAULT) {
> +				*p_pfn = KVM_PFN_ERR_RO_FAULT;
> +				return 0;
> +			}
>  			return r;

Could this cause the function to skip this new check entirely, preventing KVM
from properly returning KVM_PFN_ERR_RO_FAULT? If so, it seems like this
would crash the VM on guest writes to read-only PFNMAP memory regions
instead of emulating them.

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