Re: [PATCH v2 4/6] kvm: apply VM_READ/VM_WRITE checks to all VMA types

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Write faults on write-only `VM_IO | VM_PFNMAP` VMAs are incorrectly rejected, breaking guest writes to write-only device memory.
--

--- Patch [4]: [PATCH v2 4/6] kvm: apply VM_READ/VM_WRITE checks to all VMA types ---
commit 254a4090eb1ddc66d1d90350bb0e3ef36a2ee158
Author: Paolo Bonzini <[email protected]>

kvm: apply VM_READ/VM_WRITE checks to all VMA types

This commit moves the VM_READ and VM_WRITE checks to the beginning of the
fallback VMA lookup block in hva_to_pfn(). This standardizes the error
return codes across remapped and regular VMAs, eliminating inconsistencies
in how RO faults are handled.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 45e784462ec6e..576bcb21be3a6 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
[ ... ]
> @@ -3008,20 +2997,29 @@ kvm_pfn_t hva_to_pfn(struct kvm_follow_pfn *kfp)
>  retry:
>  	vma = vma_lookup(current->mm, kfp->hva);
>  
> -	if (vma == NULL)
> +	/*
> +	 * GUP failed.  It could be an inaccessible mapping, a pfnmap one,
> +	 * or the page might be absent.
> +	 */
> +
> +	if (vma == NULL || unlikely(!(vma->vm_flags & VM_READ))) {

[Severity: High]
Does this unconditionally reject write-only mappings? 

If a guest performs a memory write to a guest physical address backed by a
write-only host VMA (e.g. VM_IO or VM_PFNMAP mapped with PROT_WRITE but
without PROT_READ), get_user_pages() will fail and fall back to this lookup.

Since the write-only VMA lacks VM_READ, this check will evaluate to true and
return KVM_PFN_ERR_FAULT, skipping the VM_IO | VM_PFNMAP handler. Could this
prevent guests from writing to legitimate write-only device memory, such as
command ring buffers or write-only PCI BARs?

>  		pfn = KVM_PFN_ERR_FAULT;
> -	else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) {
> +	} else if ((kfp->flags & FOLL_WRITE) && unlikely(!(vma->vm_flags & VM_WRITE))) {
> +		/*
> +		 * Exit to userspace for PROT_READ mappings in a writable
> +		 * memslot, as this is part of the API.
> +		 */
> +		pfn = vma->vm_flags & (VM_IO | VM_PFNMAP) ? KVM_PFN_ERR_RO_FAULT :
> +			KVM_PFN_ERR_FAULT;
> +	} else if (vma->vm_flags & (VM_IO | VM_PFNMAP)) {
>  		r = hva_to_pfn_remapped(vma, kfp, &pfn);
[ ... ]

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