Re: [PATCH v4 4/7] KVM: VMX: Implement LASS violation check

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] LASS enforcement for supervisor-mode instruction fetches incorrectly ignores whether SMEP is enabled.
- [High] LASS is incorrectly enforced in compatibility mode, leading to spurious #GP faults for supervisor accesses.
--

--- Patch [4]: [PATCH v4 4/7] KVM: VMX: Implement LASS violation check ---
commit 7357ad469a8f5e7cd4a7e4ed10750c1068c7cc69
Author: Zeng Guang <[email protected]>

KVM: VMX: Implement LASS violation check

This commit adds a VMX implementation for detecting Linear Address Space
Separation (LASS) violations during instruction emulation. It enforces
mode-based protections on emulated instruction fetches, data accesses,
and VMX/SGX ENCLS instructions. LASS enforcement relies on CR4 and RFLAGS
settings, applying restrictions selectively based on execution context.

> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
[ ... ]
> @@ -8604,6 +8604,53 @@ gva_t vmx_get_untagged_addr(struct kvm_vcpu *vcpu, gva_t gva, unsigned int flags
>  	return (sign_extend64(gva, lam_bit) & ~BIT_ULL(63)) | (gva & BIT_ULL(63));
>  }
>  
> +bool vmx_is_lass_violation(struct kvm_vcpu *vcpu, gva_t gva,
> +			   unsigned int size, unsigned int flags)
> +{
> +	const bool is_supervisor_address = !!(gva & BIT_ULL(63));
> +	const bool implicit_supervisor = !!(flags & X86EMUL_F_IMPLICIT);
> +	const bool fetch = !!(flags & X86EMUL_F_FETCH);
> +
> +	if (!kvm_is_cr4_bit_set(vcpu, X86_CR4_LASS) || !is_long_mode(vcpu))
> +		return false;

[Severity: High]
Should this check use is_64_bit_mode() instead of is_long_mode()?

If a guest is in compatibility mode with LASS and SMAP enabled, a compatibility
mode application might trigger an interrupt resulting in an implicit supervisor
access to the IDT or TSS. Since compatibility-mode linear addresses are 32-bit,
bit 63 is always 0.

Does using is_long_mode() cause the hypervisor to erroneously flag these as
LASS violations for supervisor mode, breaking 32-bit execution?

> +
> +	/*
> +	 * INVLPG isn't subject to LASS, e.g. to allow invalidating userspace
> +	 * addresses without toggling RFLAGS.AC.  Branch targets aren't subject
> +	 * to LASS in order to simplify far control transfers (the subsequent
> +	 * fetch will enforce LASS as appropriate).
> +	 */
> +	if (flags & (X86EMUL_F_BRANCH | X86EMUL_F_INVLPG))
> +		return false;
> +
> +	if (!implicit_supervisor && vmx_get_cpl(vcpu) == 3)
> +		return is_supervisor_address;
> +
> +	/*
> +	 * LASS enforcement for supervisor-mode data accesses depends on SMAP
> +	 * being enabled, and like SMAP ignores explicit accesses if RFLAGS.AC=1.
> +	 */
> +	if (!fetch) {
> +		if (!kvm_is_cr4_bit_set(vcpu, X86_CR4_SMAP))
> +			return false;
> +
> +		if (!implicit_supervisor && (kvm_get_rflags(vcpu) & X86_EFLAGS_AC))
> +			return false;
> +	}

[Severity: High]
Does LASS enforcement for supervisor-mode instruction fetches require a check
for SMEP being enabled here?

According to the architecture specification, LASS applies to supervisor
instruction fetches only if SMEP is enabled, similar to how it requires
SMAP for data accesses.

If a guest OS running in 64-bit mode with CR4.LASS = 1 and CR4.SMEP = 0
performs a supervisor-mode instruction fetch from a user-mode address,
will omitting a kvm_is_cr4_bit_set(vcpu, X86_CR4_SMEP) check when fetch
is true cause a spurious #GP fault to be injected?

> +
> +	/*
> +	 * The entire access must be in the appropriate address space.  Note,
> +	 * if LAM is supported, @gva has already been untagged, so barring a
> +	 * massive architecture change to expand the canonical address range,
> +	 * it's impossible for a user access to straddle user and supervisor
> +	 * address spaces.
> +	 */
> +	if (size && !((gva + size - 1) & BIT_ULL(63)))
> +		return true;
> +
> +	return !is_supervisor_address;
> +}

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