Re: [PATCH] KVM: x86/mmu: Cope with the SS bit being set in #NPF exit codes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] kvm: x86/mmu: WARN_ON_ONCE triggered by PFERR_GUEST_RMP_MASK in permission_fault()
- [Low] build: unused variable user_fault in walk_addr_generic
--

commit 41b1ccbf9309dd89dc82f560f60a35e3cca23942
Author: Paolo Bonzini <pbonzini@redhat.com>
Subject: KVM: x86/mmu: Cope with the SS bit being set in #NPF exit codes

This commit modifies kvm's handling of page fault error codes to drop the SS
bit and warn on unexpected bits. It intends to fix out-of-bounds accesses in
permission_fault() seen when running Hyper-V from Windows 11. The SS bit is
also preserved when reflecting the fault to L1.

> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 2ae7f9ed4cf86..8e991e02c954e 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -275,8 +275,18 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_pagewalk *w,
>  				  unsigned pte_access, unsigned pte_pkey,
>  				  u64 access)
>  {
> -	/* strip nested paging fault error codes */
> -	unsigned int pfec = access;
> +	/*
> +	 * Of the error codes that do not contribute to the index in
> +	 * fmt->permissions, PK is not included in EPT violation bits and
> +	 * not supported by shadow paging, and RSVD faults are handled
> +	 * elsewhere.  SS however is included in the NPT exit code.
> +	 */
> +	unsigned int pfec = access & ~PFERR_SS_MASK;
> +	if (WARN_ON_ONCE(pfec & ~(PFERR_PRESENT_MASK | PFERR_WRITE_MASK |
> +				  PFERR_USER_MASK | PFERR_FETCH_MASK)))

[Severity: High]
Does this strict whitelist introduce a way for unprivileged nested guests
to panic the host?

When running nested VMs on amd hardware with sev-snp support, a guest can
intentionally trigger an rmp violation, for example by attempting an
unauthorized access to an rmp-protected page. The hardware generates a nested
page fault and sets PFERR_GUEST_RMP_MASK (bit 31) in the exitinfo1 error
code.

The npf_interception() function intercepts the nested page fault and passes
the full error code into kvm_mmu_page_fault(). During shadow page table
walking, walk_addr_generic() propagates the error code down as the access
argument to permission_fault().

Because pfec is defined as an unsigned int, bit 31 survives the initial
bitwise truncation. Since bit 31 is not in this whitelist, the check fails
and triggers the WARN_ON_ONCE, which allows an unprivileged nested guest to
consistently trigger a host kernel panic on panic_on_warn hosts.

> +		pfec &= (PFERR_PRESENT_MASK | PFERR_WRITE_MASK |
> +			 PFERR_USER_MASK | PFERR_FETCH_MASK);
> +
>  	unsigned long rflags = kvm_x86_call(get_rflags)(vcpu);

[ ... ]

> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> index 8e350095508c5..54b3172e84bff 100644
> --- a/arch/x86/kvm/mmu/paging_tmpl.h
> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
> @@ -489,7 +489,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
>  	return 1;
>  
>  error:
> -	errcode |= write_fault | user_fault;
> +	errcode |= access & (PFERR_WRITE_MASK | PFERR_USER_MASK | PFERR_SS_MASK);

[Severity: Low]
Does this change leave user_fault unused?

Because user_fault is declared at the top of FNAME(walk_addr_generic):

    const int user_fault  = access & PFERR_USER_MASK;

Removing its only usage here seems to trigger a -Wunused-variable compiler
warning, which breaks the build under CONFIG_WERROR. Since this file is a
template included multiple times, the error repeats for each instantiation.

>  	if (fetch_fault && has_pferr_fetch(w))
>  		errcode |= PFERR_FETCH_MASK;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009130450.361690-1-pbonzini@redhat.com?part=1