Re: [bug report] KVM: x86: Do not block APIC write for non ICR registers

[Sean Christopherson <seanjc@google.com>]

On Thu, Aug 04, 2022, Dan Carpenter wrote:
> Hello Suravee Suthikulpanit,
> 
> The patch 1bd9dfec9fd4: "KVM: x86: Do not block APIC write for non
> ICR registers" from Jul 25, 2022, leads to the following Smatch
> static checker warning:
> 
> 	arch/x86/kvm/lapic.c:2302 kvm_apic_write_nodecode()
> 	error: uninitialized symbol 'val'.
> 
> arch/x86/kvm/lapic.c
>   2282  void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
>   2283  {
>   2284          struct kvm_lapic *apic = vcpu->arch.apic;
>   2285          u64 val;
>   2286  
>   2287          if (apic_x2apic_mode(apic))
>   2288                  kvm_lapic_msr_read(apic, offset, &val);
> 
> Originally, this was only called when "offset == APIC_ICR", but the
> patch removed that condition.  Now, if kvm_lapic_msr_read() returns 1
> then "val" isn't initialized.
> 
>   2289          else
>   2290                  val = kvm_lapic_get_reg(apic, offset);
>   2291  
>   2292          /*
>   2293           * ICR is a single 64-bit register when x2APIC is enabled.  For legacy
>   2294           * xAPIC, ICR writes need to go down the common (slightly slower) path
>   2295           * to get the upper half from ICR2.
>   2296           */
>   2297          if (apic_x2apic_mode(apic) && offset == APIC_ICR) {
>   2298                  kvm_apic_send_ipi(apic, (u32)val, (u32)(val >> 32));
>   2299                  trace_kvm_apic_write(APIC_ICR, val);
>   2300          } else {
>   2301                  /* TODO: optimize to just emulate side effect w/o one more write */
>   2302                  kvm_lapic_reg_write(apic, offset, (u32)val);
> 
> The warning here is for when apic_x2apic_mode() is true but
> "offset != APIC_ICR" and kvm_lapic_msr_read() returns 1.

In theory this can't happen because kvm_apic_write_nodecode() is called if and only
if hardware successful wrote the vAPIC register, i.e. kvm_lapic_msr_read() should
never fail.  But I 100% agree that not guarding against a hardware/ucode/KVM bug is
unnecessarily dangerous.

There are more succinct ways to handle this, but they're rather gross and it's
probably best to bug the VM and bail, e.g. as opposed to zeroing out val and
continuing on.

---
 arch/x86/kvm/lapic.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index e2ce3556915e..9dda989a1cf0 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -2284,10 +2284,12 @@ void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
 	struct kvm_lapic *apic = vcpu->arch.apic;
 	u64 val;

-	if (apic_x2apic_mode(apic))
-		kvm_lapic_msr_read(apic, offset, &val);
-	else
+	if (apic_x2apic_mode(apic)) {
+		if (KVM_BUG_ON(kvm_lapic_msr_read(apic, offset, &val), vcpu->kvm))
+			return;
+	} else {
 		val = kvm_lapic_get_reg(apic, offset);
+	}

 	/*
 	 * ICR is a single 64-bit register when x2APIC is enabled.  For legacy

base-commit: e91e4455d92a5f2908eeb295d3512bd1a31e1558
--