Re: [Patch v2 2/4] KVM: SVM: Add host support for Enhanced SMT Protection

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: svm: host #DE panic when esmtp_timeout_ns is excessively large
--

commit ec4daa3c1cc021f9f7a5cc29bfa1c4493cc845b8
Author: Pratik R. Sampat <prsampat@amd.com>
Subject: KVM: SVM: Add host support for Enhanced SMT Protection

Enhanced SMT Protection (ESMTP) protects SEV-SNP guests from SMT side
channels by stalling VMRUN until sibling threads are idle or running legal
sibling vCPUs. This commit introduces the hardware enforcement logic,
non-fatal exit handling, and a configurable timeout parameter.

> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 944aaea6501f2..24701f06fce81 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
[ ... ]
> @@ -67,6 +67,11 @@ module_param_named(sev_snp, sev_snp_enabled, bool, 0444);
>  static unsigned int __ro_after_init nr_ciphertext_hiding_asids;
>  module_param_named(ciphertext_hiding_asids, nr_ciphertext_hiding_asids, uint, 0444);
>  
> +static unsigned long esmtp_timeout_ns;
> +module_param(esmtp_timeout_ns, ulong, 0644);
> +MODULE_PARM_DESC(esmtp_timeout_ns,
> +		 "ESMTP VMRUN sibling-wait timeout in nanoseconds (0 disables the timer)");
> +
[ ... ]
> @@ -4708,6 +4750,10 @@ static void sev_es_init_vmcb(struct vcpu_svm *svm, bool init_event)
>  		svm->vmcb->control.allowed_sev_features = sev->vmsa_features |
>  							  VMCB_ALLOWED_SEV_FEATURES_VALID;
>  
> +	if (sev->vmsa_features & SVM_SEV_FEAT_ESMTP)
> +		svm->vmcb->control.esmtp_timeout = mul_u64_u32_div(esmtp_timeout_ns,
> +								   tsc_khz, 1000000);

[Severity: High]
Since esmtp_timeout_ns is writable by root (0644) without bounds validation,
could a very large value here trigger a host kernel panic?

The mul_u64_u32_div() function relies on the x86 divq instruction. If the
resulting 128-by-64 bit division quotient does not fit in 64 bits (for
example, if esmtp_timeout_ns is set near ULONG_MAX), divq unconditionally
raises a #DE (Divide Error) exception. Because there is no exception table
entry for this instruction in mul_u64_add_u64_div_u64(), this leads to an
immediate panic.

Would it be safer to enforce a maximum limit on esmtp_timeout_ns to prevent a
sysfs write followed by a VM start from crashing the hypervisor?

> +
>  	/* Can't intercept CR register access, HV can't modify CR registers */
>  	svm_clr_intercept(svm, INTERCEPT_CR0_READ);
>  	svm_clr_intercept(svm, INTERCEPT_CR4_READ);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1791302373.git.prsampat@amd.com?part=2