Re: [PATCH v2 2/8] KVM: nSVM: Synthesize DecodeAssists EXITINFO for emulated intercepts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditionally setting bit 63 in EXITINFO1 for CR read intercepts.
--

--- Patch [2]: [PATCH v2 2/8] KVM: nSVM: Synthesize DecodeAssists EXITINFO for emulated intercepts ---
commit 12fdeb07951db9a20a07afc60e302f9b9e4fcaca
Author: Tina Zhang <zhang_wei@open-hieco.net>

KVM: nSVM: Synthesize DecodeAssists EXITINFO for emulated intercepts

This commit populates the architectural EXITINFO fields when DecodeAssists is 
exposed to L1 and an emulated intercept occurs. It generates EXITINFO1 
depending on the intercept type, such as CR/DR accesses and INTn.

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index e02a38da5296e..cc35afb59623f 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
[ ... ]
> @@ -4754,6 +4754,52 @@ static const struct __x86_intercept {
>  #undef POST_EX
>  #undef POST_MEM
>  
> +static void svm_prepare_decode_assist_exit_info(struct kvm_vcpu *vcpu,
> +						const struct x86_instruction_info *info)
> +{
> +	struct vmcb *vmcb = to_svm(vcpu)->vmcb;
> +	u64 exit_info_1;
> +
> +	if (!guest_cpu_cap_has(vcpu, X86_FEATURE_DECODEASSISTS))
> +		return;
> +
> +	switch (info->intercept) {
> +	case x86_intercept_cr_read:
> +	case x86_intercept_cr_write:
> +		/* MOV CRx: bit 63 set, GPR number in bits 3:0. */
> +		exit_info_1 = BIT_ULL(63) | (info->modrm_rm & 0xf);
                ^^^^^^^^^^^
[Severity: High]
Does this code incorrectly set bit 63 of EXITINFO1 for CR reads?

According to the AMD Architecture Programmer's Manual (APM), bit 63 of
EXITINFO1 for a CR intercept indicates the operation type, where 1 means a
write to the CR and 0 means a read from the CR. By unconditionally setting
BIT_ULL(63) for both x86_intercept_cr_read and x86_intercept_cr_write,
wouldn't KVM report CR reads as CR writes to the L1 hypervisor?

If an L2 guest executes a MOV from CR instruction, this could cause the L1
hypervisor to corrupt its shadow CPU state or misbehave, because it will
be provided a malformed EXITINFO1 and handle the event as a write operation
rather than a read.

> +		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783999988.git.zhang_wei@open-hieco.net?part=2