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
--
