| Message ID | 20211011182853.978640-1-venkateshs@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: Inject #GP on invalid writes to x2APIC registers | expand |
Be kind to case-sensitive greppers and capitalize the "KVM" in the shortlog ;-) On Mon, Oct 11, 2021, Venkatesh Srinivas wrote: Assuming Marc is the original author, this needs an explicit From: Marc Orr <marcorr@google.com> so that he's accredited as the author by hit. A few ways to do this are described here: https://lkml.kernel.org/r/YUNu2npJv2LPBRop@google.com > The upper 7 bytes pf the x2APIC self IPI register and the upper 4 > bytes of any 32-bit x2APIC register are reserved. Inject a #GP into the > guest if any of these reserved bits are set. Hmm, I'd split this into two patches. It's a bit silly for such a small change, but if for some reason _one_ of the checks turns out to be wrong or cause problems, it's easy to pinpoint and fix/revert that individual change. > Signed-off-by: Marc Orr <marcorr@google.com> > Signed-off-by: Venkatesh Srinivas <venkateshs@chromium.org> > --- > arch/x86/kvm/lapic.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 76fb00921203..96e300acf70a 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -2126,13 +2126,15 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) > ret = 1; > break; > > - case APIC_SELF_IPI: > - if (apic_x2apic_mode(apic)) { > + case APIC_SELF_IPI: { Braces on the case statement are unnecessary (and confusing). > + /* Top 7 bytes of val are reserved in x2apic mode */ Comment says 7 bytes, code checks 3 :-) It's arguably even technically wrong to say 7 bytes are reserved, because the SDM defines Self-IPI as a 32-bit register. Doesn't reaaaally matter, but maybe dodge the issue by using slightly less specific language? (sample below) > + if (apic_x2apic_mode(apic) && !(val & GENMASK(31, 8))) { Hmm, rather than a custom GENMASK, what about ~APIC_VECTOR_MASK? I think that would fold in nicely with an updated comment? (again, see below) Not your (Marc's?) code, but the braces here are technically unnecessary as well. Some subsystems prefer braces if the code splits multiple lines, but KVM usually omits the braces if there's a single statement, even if the statement spans more than one line. > kvm_lapic_reg_write(apic, APIC_ICR, > APIC_DEST_SELF | (val & APIC_VECTOR_MASK)); > } else If you do keep the braces, please opportunistically add braces for the else path as well. That's a more rigid kernel style guideline :-) Actually, to avoid a somewhat confusing comment since Self-IPI doesn't even exist outside of x2APIC, what about: /* * Self-IPI exists only when x2APIC is enabled. Bits 7:0 hold * the vector, everything else is reserved. */ if (!apic_x2apic_mode(apic) || (val & ~APIC_VECTOR_MASK)) ret = 1; break; } kvm_lapic_reg_write(apic, APIC_ICR, APIC_DEST_SELF | (val & APIC_VECTOR_MASK)); break; That avoids the braces debate, avoids a taken branch in the happy case, and eliminates the >80 chars line. > ret = 1; > break; > + } > default: > ret = 1; > break; > @@ -2797,6 +2799,9 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data) > /* if this is ICR write vector before command */ > if (reg == APIC_ICR) > kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32)); > + else if (data & GENMASK_ULL(63, 32)) I think we usually write this as else if (data >> 32) in KVM? Not sure if we have an official policy :-) > + return 1; > + > return kvm_lapic_reg_write(apic, reg, (u32)data); > } > > -- > 2.33.0.882.g93a45727a2-goog >
> > arch/x86/kvm/lapic.c | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > > index 76fb00921203..96e300acf70a 100644 > > --- a/arch/x86/kvm/lapic.c > > +++ b/arch/x86/kvm/lapic.c > > @@ -2126,13 +2126,15 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) > > ret = 1; > > break; > > > > - case APIC_SELF_IPI: > > - if (apic_x2apic_mode(apic)) { > > + case APIC_SELF_IPI: { > > Braces on the case statement are unnecessary (and confusing). I guess the original patch defined an intermediate case-local var. I agree, the case braces can be removed in this version. Also, thanks for upstreaming this, Venkatesh! I read through Sean's feedback, and it is all fine with me.
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 76fb00921203..96e300acf70a 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -2126,13 +2126,15 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val) ret = 1; break; - case APIC_SELF_IPI: - if (apic_x2apic_mode(apic)) { + case APIC_SELF_IPI: { + /* Top 7 bytes of val are reserved in x2apic mode */ + if (apic_x2apic_mode(apic) && !(val & GENMASK(31, 8))) { kvm_lapic_reg_write(apic, APIC_ICR, APIC_DEST_SELF | (val & APIC_VECTOR_MASK)); } else ret = 1; break; + } default: ret = 1; break; @@ -2797,6 +2799,9 @@ int kvm_x2apic_msr_write(struct kvm_vcpu *vcpu, u32 msr, u64 data) /* if this is ICR write vector before command */ if (reg == APIC_ICR) kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(data >> 32)); + else if (data & GENMASK_ULL(63, 32)) + return 1; + return kvm_lapic_reg_write(apic, reg, (u32)data); }