* Re: [PATCH] kvm: Inject #GP on invalid writes to x2APIC registers
2021-10-11 18:28 [PATCH] kvm: Inject #GP on invalid writes to x2APIC registers Venkatesh Srinivas
@ 2021-10-11 21:48 ` Sean Christopherson
2021-10-11 22:12 ` Marc Orr
0 siblings, 1 reply; 3+ messages in thread
From: Sean Christopherson @ 2021-10-11 21:48 UTC (permalink / raw)
To: Venkatesh Srinivas; +Cc: kvm, marcorr, pbonzini
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread