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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] kvm: nSVM: Stale EXITINFO2 leaked to L1 for Decode Assists and SWINT/ICEBP
- [High] kvm: nSVM: Unconditional CR_VALID injects spurious #GP into 32-bit guests
- [Medium] kvm: nSVM: Missing Decode Assists synthesis for SVM instructions
--

commit 426ea34e024e5747a2d0375a678f4064cd396fdd
Author: Tina Zhang <zhang_wei@open-hieco.net>

KVM: nSVM: Synthesize DecodeAssists EXITINFO for emulated intercepts

This commit adds synthesis of DecodeAssists EXITINFO1 fields for emulated
instruction intercepts such as MOV CR/DR, INTn, and INVLPG when the guest
CPU exposes DecodeAssists capabilities.

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
[ ... ]
> @@ -4846,14 +4847,26 @@ static int svm_check_intercept(struct kvm_vcpu *vcpu,
>  	if (stage != icpt_info.stage)
>  		goto out;
>  
> +	decode_assists = guest_cpu_cap_has(vcpu, X86_FEATURE_DECODEASSISTS);
> +
>  	switch (icpt_info.exit_code) {

[Severity: Medium]
Does this switch statement miss several mandatory intercepts defined in the
hardware specification for Decode Assists?

According to AMD APM Vol 2, Table 15-22, Decode Assists also apply to VMRUN,
VMLOAD, VMSAVE, CLGI, STGI, and PAUSE, requiring EXITINFO1 to be explicitly
cleared to 0.

Since these intercepts are omitted, will they fall through to the default
case and leave uninitialized, stale garbage in EXITINFO1 instead of the
architecturally required 0?

>  	case SVM_EXIT_READ_CR0:
>  		if (info->intercept == x86_intercept_cr_read)
>  			icpt_info.exit_code += info->modrm_reg;
> +
> +		if (decode_assists)
> +			vmcb->control.exit_info_1 =
> +				info->intercept == x86_intercept_cr_read ?
> +				CR_VALID | (info->modrm_rm & SVM_EXITINFO_REG_MASK) : 0;

[Severity: High]
Does setting CR_VALID unconditionally here, and below in SVM_EXIT_WRITE_CR0,
inject spurious #GP faults into 32-bit guests?

AMD APM defines Bit 63 (CR_VALID) of EXITINFO1 for CR accesses under
Decode Assists as "1 = instruction is 64-bit, 0 = instruction is 32-bit".

If a 32-bit L2 guest executes a CR access and its general purpose register
happens to have non-zero upper 32 bits (which is permissible), L1 will see
Bit 63 set.

If L1 treats this as a 64-bit instruction, will it read the full 64-bit
vCPU register and fail safety checks (like cr0 & 0xffffffff00000000UL),
resulting in a spurious #GP being injected into L2?

[Severity: High]
Should exit_info_2 also be unconditionally zeroed here when decode_assists
is true?

AMD APM mandates that when Decode Assists is enabled, EXITINFO2 must contain
NextRIP for INTn and ICEBP, and must be 0 for other intercepts like MOV CRx.

Because KVM does not zero exit_info_2 prior to calling svm_check_intercept(),
could this retain a stale value in the L2 VMCB from a prior exit (such as a
#NPF faulting GPA) and leak it to L1?

>  		break;
>  	case SVM_EXIT_WRITE_CR0: {
>  		unsigned long cr0, val;
>  
> +		if (decode_assists)
> +			vmcb->control.exit_info_1 =
> +				info->intercept == x86_intercept_cr_write ?
> +				CR_VALID | (info->modrm_rm & SVM_EXITINFO_REG_MASK) : 0;
> +
[ ... ]
>  	case SVM_EXIT_SWINT:
> +		if (decode_assists)
> +			vmcb->control.exit_info_1 = info->src_val & 0xff;
> +		break;

[Severity: High]
Does this miss setting exit_info_2 for the SWINT intercept?

AMD APM mandates that EXITINFO2 must contain NextRIP for INTn (SVM_EXIT_SWINT)
and ICEBP (SVM_EXIT_ICEBP) when Decode Assists is enabled.

By not setting exit_info_2 = info->next_rip for SWINT, and completely
omitting handling for SVM_EXIT_ICEBP, does L1 read the stale EXITINFO2 as
NextRIP?

If L1 relies on this for NextRIP, could it cause L2 to resume at a corrupted
instruction pointer and crash the L2 guest?

>  	case SVM_EXIT_INVLPG:
> +		if (decode_assists)
> +			vmcb->control.exit_info_1 = info->invlpg_linear_addr;
> +		break;

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