[PATCH] KVM: nVMX: store L2's TSC frame in the VM-exit IA32_TSC MSR-store list

[PATCH] KVM: nVMX: store L2's TSC frame in the VM-exit IA32_TSC MSR-store list

[Amirmohammad Eftekhar]

When L1 lists IA32_TSC (0x10) in vmcs12's VM-exit MSR-store area, the
value KVM auto-stores on a nested VM-exit is computed with
kvm_read_l1_tsc(), which evaluates the TSC in L1's frame
(l1_tsc_offset + host_tsc scaled by l1_tsc_scaling_ratio).  But the
guest that just exited is L2, and RDMSR(IA32_TSC) in L2 returns L2's
frame, (host_tsc scaled by the composed ratio) + the composed offset
(SDM Vol 3C 27.4).  The stored value is therefore short by exactly the
L2 offset O2 (more under scaling), so an L1 that uses this list to bound
the highest TSC L2 could have observed gets a bound that is too low.  A
subsequent L1 re-base can then legitimately place L2's TSC below a value
L2 already read, i.e. drive L2's clock backward -- the exact hazard the
feature added in commit 662f1d1d1931 ("KVM: nVMX: Add support for
capturing highest observable L2 TSC") was meant to prevent.

The bug is wrong by single-level equivalence: a non-nested hypervisor
running a guest with offset O that lists IA32_TSC in its VM-exit
MSR-store area receives the guest's frame; the nested emulation of the
identical setup must produce the identical value.

nested_vmx_vmexit() restores the composed TSC state to L1's before the
MSR-store list is processed, which is why the code reached for
kvm_read_l1_tsc() -- whose contract is genuinely raw input everywhere
else it's called (lapic.c and x86.c's pvclock path both pass it
straight from rdtsc()), consistent with treating the autostored value
the same way.  It just stops one step short: O2/M2 remain readable
from vmcs12 via the vmx_get_l2_tsc_* accessors, so recompose L2's
frame there.  Export kvm_scale_tsc() so nested.c can scale the host
TSC by the composed ratio.

Signed-off-by: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>
---
 arch/x86/kvm/vmx/nested.c | 20 +++++++++++++++++++-
 arch/x86/kvm/x86.c        |  1 +
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 6957bb6f5cf7..610b58933d6b 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -1111,8 +1111,26 @@ static bool nested_vmx_get_vmexit_msr_value(struct kvm_vcpu *vcpu,
 	if (msr_index == MSR_IA32_TSC && vmx->nested.tsc_autostore_slot >= 0) {
 		int slot = vmx->nested.tsc_autostore_slot;
 		u64 host_tsc = vmx->msr_autostore.val[slot].value;
+		u64 l2_multiplier = vmx_get_l2_tsc_multiplier(vcpu);
+		u64 l2_offset, l2_ratio;
 
-		*data = kvm_read_l1_tsc(vcpu, host_tsc);
+		/*
+		 * The guest that just exited is L2, so the value auto-stored on
+		 * its behalf must be L2's frame: RDMSR(IA32_TSC) in L2 reads
+		 * (host_tsc scaled by the composed ratio) + the composed offset
+		 * (SDM Vol 3C 27.4).  nested_vmx_vmexit() has already restored
+		 * the composed TSC state to L1's by the time the MSR-store list
+		 * is processed, so recompose L2's frame from L1 state and the
+		 * vmcs12 TSC controls.  kvm_read_l1_tsc() would instead record
+		 * L1's frame, dropping L2's offset (and scaling) entirely.
+		 */
+		l2_offset = kvm_calc_nested_tsc_offset(vcpu->arch.l1_tsc_offset,
+						       vmx_get_l2_tsc_offset(vcpu),
+						       l2_multiplier);
+		l2_ratio = kvm_calc_nested_tsc_multiplier(vcpu->arch.l1_tsc_scaling_ratio,
+							  l2_multiplier);
+
+		*data = kvm_scale_tsc(host_tsc, l2_ratio) + l2_offset;
 		return true;
 	}
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index afcac1042947..0ce0d30e8825 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2641,6 +2641,7 @@ u64 kvm_scale_tsc(u64 tsc, u64 ratio)
 
 	return _tsc;
 }
+EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_scale_tsc);
 
 static u64 kvm_compute_l1_tsc_offset(struct kvm_vcpu *vcpu, u64 target_tsc)
 {
-- 
2.34.1

Re: [PATCH] KVM: nVMX: store L2's TSC frame in the VM-exit IA32_TSC MSR-store list

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch incorrectly returns L2's TSC frame instead of L1's TSC frame for IA32_TSC in the VM-exit MSR-store list, directly violating the Intel SDM.
- [Medium] The patch introduces an inconsistency between the MSR_IA32_TSC autostore path and the fallback read path.
--

--- Patch [1]: [PATCH] KVM: nVMX: store L2's TSC frame in the VM-exit IA32_TSC MSR-store list ---
commit 9a886b64a3ad54e2783daef1b8de78fa7019e1c2
Author: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>

KVM: nVMX: store L2's TSC frame in the VM-exit IA32_TSC MSR-store list

This commit updates KVM to store L2's TSC frame in the VM-exit IA32_TSC
MSR-store list. It recomposes L2's frame using L1 state and vmcs12 TSC
controls.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 6957bb6f5cf7e..610b58933d6b4 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -1111,8 +1111,26 @@ static bool nested_vmx_get_vmexit_msr_value(struct kvm_vcpu *vcpu,
>  	if (msr_index == MSR_IA32_TSC && vmx->nested.tsc_autostore_slot >= 0) {
>  		int slot = vmx->nested.tsc_autostore_slot;
>  		u64 host_tsc = vmx->msr_autostore.val[slot].value;
> +		u64 l2_multiplier = vmx_get_l2_tsc_multiplier(vcpu);
> +		u64 l2_offset, l2_ratio;
>  
> -		*data = kvm_read_l1_tsc(vcpu, host_tsc);
> +		/*
> +		 * The guest that just exited is L2, so the value auto-stored on
> +		 * its behalf must be L2's frame: RDMSR(IA32_TSC) in L2 reads
> +		 * (host_tsc scaled by the composed ratio) + the composed offset
> +		 * (SDM Vol 3C 27.4).  nested_vmx_vmexit() has already restored
> +		 * the composed TSC state to L1's by the time the MSR-store list
> +		 * is processed, so recompose L2's frame from L1 state and the
> +		 * vmcs12 TSC controls.  kvm_read_l1_tsc() would instead record
> +		 * L1's frame, dropping L2's offset (and scaling) entirely.
> +		 */
> +		l2_offset = kvm_calc_nested_tsc_offset(vcpu->arch.l1_tsc_offset,
> +						       vmx_get_l2_tsc_offset(vcpu),
> +						       l2_multiplier);
> +		l2_ratio = kvm_calc_nested_tsc_multiplier(vcpu->arch.l1_tsc_scaling_ratio,
> +							  l2_multiplier);
> +
> +		*data = kvm_scale_tsc(host_tsc, l2_ratio) + l2_offset;

[Severity: High]
Does this calculation violate the architectural requirements for the VM-exit
MSR-store list?

According to the Intel SDM (Vol 3C, 27.4), reads of MSR_IA32_TSC for the
VM-exit MSR-store list are not subject to TSC offsetting or TSC scaling,
which requires the hardware to save the bare host TSC.

From L1's perspective, the bare host TSC is L1's TSC frame. By applying L2's
offset and multiplier here, this returns L2's TSC frame instead. A
conforming L1 hypervisor that correctly applies its guest's offset and
multiplier could inadvertently apply them a second time, which could
corrupt the L2 guest's clock.

>  		return true;
>  	}
>  

[Severity: Medium]
Is there an inconsistency introduced between the autostore path and the
fallback MSR read path?

If the autostore slot was not successfully allocated, the code falls back to
calling kvm_emulate_msr_read() right below this block. Because the vcpu
state is already reverted to L1's configuration before processing the
MSR-store list, the fallback path continues to evaluate and return L1's TSC
frame.

This divergence causes the returned value to unexpectedly toggle between L2's
frame and L1's frame depending on whether the autostore array was full.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713144605.2773845-1-amirmohammad.eftekhar@cispa.de?part=1

Re: [PATCH] KVM: nVMX: store L2's TSC frame in the VM-exit IA32_TSC MSR-store list

[Amirmohammad Eftekhar]

The bot's review is correct -- I'd like to withdraw this patch.

I tested the underlying assumption on real hardware: a single-level (non-nested)
VMX hypervisor that configures a TSC offset for a guest and lists IA32_TSC in
its own VM-exit MSR-store area gets back its *own* TSC frame on exit, not the
guest's. That contradicts the "single-level equivalence" reasoning this patch
was based on. The original code was already correct.

Sorry for the noise -- please disregard this patch.

Amirmohammad