Re: [PATCH v3 04/40] KVM: VMX: Read 32-bit GPR values for ENCLS instructions outside of 64-bit mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect 32-bit mode segment check emulation in `sgx_get_encls_gva` violates architectural bounds, alignment, and type rules.
- [High] KVM fails to enforce the architectural requirement that 64-bit pointer fields in memory-based ENCLS structures must have their upper 32 bits set to zero when executing in 32-bit mode.
--

commit ca9673f76fcf276ab1d3acd4cb535b328a49e842
Author: Sean Christopherson <seanjc@google.com>

KVM: VMX: Read 32-bit GPR values for ENCLS instructions outside of 64-bit mode

When getting register values for ENCLS emulation, use kvm_register_read()
instead of kvm_<reg>_read() so that bits 63:32 of the register are dropped
if the guest is in 32-bit mode.

> diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> index df1d0cf769475..4c61fc33f7648 100644
> --- a/arch/x86/kvm/vmx/sgx.c
> +++ b/arch/x86/kvm/vmx/sgx.c
> @@ -225,8 +225,8 @@ static int handle_encls_ecreate(struct kvm_vcpu *vcpu)
>  	struct x86_exception ex;
>  	int r;
>  
> -	if (sgx_get_encls_gva(vcpu, kvm_rbx_read(vcpu), 32, 32, &pageinfo_gva) ||
> -	    sgx_get_encls_gva(vcpu, kvm_rcx_read(vcpu), 4096, 4096, &secs_gva))
> +	if (sgx_get_encls_gva(vcpu, kvm_register_read(vcpu, VCPU_REGS_RBX), 32, 32, &pageinfo_gva) ||
> +	    sgx_get_encls_gva(vcpu, kvm_register_read(vcpu, VCPU_REGS_RCX), 4096, 4096, &secs_gva))
>  		return 1;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the 32-bit mode
fixes for ENCLS, does the existing 32-bit segment check emulation in
sgx_get_encls_gva() violate architectural bounds, alignment, and type rules?

In sgx_get_encls_gva():

	} else {
		*gva &= 0xffffffff;
		fault = (s.unusable) ||
			(s.type != 2 && s.type != 3) ||
			(*gva > s.limit) ||
			((s.base != 0 || s.limit != 0xffffffff) &&
			(((u64)*gva + size - 1) > s.limit + 1));
	}

Is it correct that this calculates the linear address (*gva = offset + s.base)
and then evaluates the segment limit against it (*gva > s.limit), rather than
checking against the effective address? A non-zero s.base could cause limits
to be evaluated incorrectly.

Also, does the check (((u64)*gva + size - 1) > s.limit + 1) allow an access
to exceed the inclusive segment limit by 1 byte without faulting?

Finally, by requiring s.type == 2 || s.type == 3 (Read/Write data segments),
does this reject valid Read-Only data segments (types 0, 1), even though
ENCLS structures being read (like PAGEINFO) only require read access?

[Severity: High]
This is a pre-existing issue, but does KVM fail to enforce the architectural
requirement that 64-bit pointer fields in memory-based ENCLS structures must
have their upper 32 bits set to zero when executing in 32-bit mode?

According to the Intel SDM for ENCLS, "Any 64-bit pointer field within the
data structures must have its upper 32 bits zero in 32-bit mode, or else
#GP(0) is generated."

Looking at handle_encls_ecreate(), KVM extracts 64-bit pointers from the
guest PAGEINFO structure:

	if (sgx_get_encls_gva(vcpu, pageinfo.metadata, 64, 64, &metadata_gva) ||
	    sgx_get_encls_gva(vcpu, pageinfo.contents, 4096, 4096,
			      &contents_gva))

And passes them to sgx_get_encls_gva(). However, in 32-bit mode,
sgx_get_encls_gva() just silently masks the upper 32 bits:

	} else {
		*gva &= 0xffffffff;

Should this instead validate that the upper 32 bits are zero and inject a
#GP(0) to prevent invalid guest structures from being successfully processed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529222223.870923-1-seanjc@google.com?part=4