Re: [PATCH v4 3/5] KVM: riscv: Implement KVM_PRE_FAULT_MEMORY

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Returning `-EFAULT` on `KVM_PFN_ERR_HWPOISON` in `kvm_riscv_mmu_map` breaks `KVM_RUN` execution and normal KVM signal delivery.
- [High] Incorrect map advancement calculation in `kvm_arch_vcpu_pre_fault_memory` causes skipped pages and false completion for unaligned and split huge pages.
--

--- Patch [3]: [PATCH v4 3/5] KVM: riscv: Implement KVM_PRE_FAULT_MEMORY ---
commit d1fe5f152fd69a51f4bc7397e89186eff774f433
Author: Jinyu Tang <[email protected]>
Subject: KVM: riscv: Implement KVM_PRE_FAULT_MEMORY

The patch implements the KVM_PRE_FAULT_MEMORY ioctl for RISC-V by adding the
kvm_arch_vcpu_pre_fault_memory() hook. It reuses the existing G-stage fault
mapping path to populate KVM page tables.

> diff --git a/arch/riscv/kvm/gstage.c b/arch/riscv/kvm/gstage.c
> index 54d45addf18f9..dff315dfd24ed 100644
> --- a/arch/riscv/kvm/gstage.c
> +++ b/arch/riscv/kvm/gstage.c
> @@ -286,6 +286,9 @@ int kvm_riscv_gstage_map_page(struct kvm_gstage *gstage,
>  						    out_map->level, true);
>  		} else if (ALIGN_DOWN(PFN_PHYS(pte_pfn(ptep_get(ptep))), page_size) == hpa) {
>  			kvm_riscv_gstage_update_pte_prot(gstage, ptep_level, gpa, ptep, prot);
> +			out_map->addr = ALIGN_DOWN(gpa, page_size);
> +			out_map->level = ptep_level;
> +			out_map->pte = ptep_get(ptep);
>  			return 0;
>  		}
>  	}

[ ... ]

> diff --git a/arch/riscv/kvm/mmu.c b/arch/riscv/kvm/mmu.c
> index 2fabcd4099919..f6ca86da53eb8 100644
> --- a/arch/riscv/kvm/mmu.c
> +++ b/arch/riscv/kvm/mmu.c
> @@ -717,7 +717,7 @@ int kvm_riscv_mmu_map(struct kvm_vcpu *vcpu, struct kvm_memory_slot *memslot,
>  	if (hfn == KVM_PFN_ERR_HWPOISON) {
>  		send_sig_mceerr(BUS_MCEERR_AR, (void __user *)hva,
>  				vma_pageshift, current);
> -		return 0;
> +		return -EFAULT;

[Severity: High]
Does returning -EFAULT here break KVM's standard exit-to-userspace flow on a
signal?

If a guest accesses a poisoned memory page, returning 0 allows KVM to bubble
up 1 to kvm_arch_vcpu_ioctl_run(), which checks signal_pending() (set by
send_sig_mceerr) and correctly exits to userspace with -EINTR.

Returning -EFAULT causes the KVM_RUN ioctl to immediately fail with -EFAULT,
which can cause the VMM to crash instead of gracefully handling the Machine
Check Exception.

[ ... ]

> @@ -782,6 +782,49 @@ int kvm_riscv_mmu_map(struct kvm_vcpu *vcpu, struct kvm_memory_slot *memslot,
[ ... ]
> +	for (;;) {
> +		if (signal_pending(current))
> +			return -EINTR;
> +
> +		if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu))
> +			return -EIO;
> +
> +		cond_resched();
> +		ret = kvm_riscv_mmu_map(vcpu, memslot, range->gpa, hva, false, &out_map);
> +		if (ret)
> +			return ret;
> +
> +		if (!pte_val(out_map.pte))
> +			continue;
> +
> +		map_size = PAGE_SIZE << (out_map.level * kvm_riscv_gstage_index_bits);
> +		end = out_map.addr + map_size;
> +		break;
> +	}
> +
> +	return min_t(u64, range->size, end - range->gpa);
> +}

[Severity: High]
Can end - range->gpa underflow here when handling unaligned offsets or split
huge pages?

In kvm_riscv_gstage_map_page() above, out_map.addr is aligned to the
requested huge page size rather than the actual leaf size installed. 

If out_map.level is set to the 4K leaf level, but out_map.addr is aligned to
the huge page size, and range->gpa is beyond the first 4K of that huge page,
end will be calculated as less than range->gpa.

This would cause end - range->gpa to underflow into a large u64, making
min_t() evaluate to range->size and prematurely terminating the ioctl while
skipping regions of guest physical address space.

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