Re: [PATCH] KVM: VMX: Forbid userspace MSR filters for x2APIC

["Graf (AWS), Alexander" <graf@amazon.de>]

> Am 19.10.2020 um 19:08 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> 
> Allowing userspace to intercept reads to x2APIC MSRs when APICV is
> fully enabled for the guest simply can't work.   But more in general,
> if the LAPIC is in-kernel, allowing accessed by userspace would be very
> confusing.  If userspace wants to intercept x2APIC MSRs, then it should
> first disable in-kernel APIC.
> 
> We could in principle allow userspace to intercept reads and writes to TPR,
> and writes to EOI and SELF_IPI, but while that could be made it work, it
> would still be silly.
> 
> Cc: Alexander Graf <graf@amazon.com>
> Cc: Aaron Lewis <aaronlewis@google.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/kvm/x86.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index c4015a43cc8a..0faf733538f4 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5246,6 +5246,13 @@ static int kvm_add_msr_filter(struct kvm *kvm, struct kvm_msr_filter_range *user
>        return r;
> }
> 
> +static bool range_overlaps_x2apic(struct kvm_msr_filter_range *range)
> +{
> +       u32 start = range->base;
> +       u32 end = start + range->nmsrs;
> +       return start <= 0x8ff && end > 0x800;
> +}
> +
> static int kvm_vm_ioctl_set_msr_filter(struct kvm *kvm, void __user *argp)
> {
>        struct kvm_msr_filter __user *user_msr_filter = argp;
> @@ -5257,6 +5264,16 @@ static int kvm_vm_ioctl_set_msr_filter(struct kvm *kvm, void __user *argp)
>        if (copy_from_user(&filter, user_msr_filter, sizeof(filter)))
>                return -EFAULT;
> 
> +       /*
> +        * In principle it would be possible to trap x2apic ranges
> +        * if !lapic_in_kernel.  This however would be complicated
> +        * because KVM_X86_SET_MSR_FILTER can be called before
> +        * KVM_CREATE_IRQCHIP or KVM_ENABLE_CAP.
> +        */
> +       for (i = 0; i < ARRAY_SIZE(filter.ranges); i++)
> +               if (range_overlaps_x2apic(&filter.ranges[i]))
> +                       return -EINVAL;

What if the default action of the filter is to "deny"? Then only an MSR filter to allow access to x2apic MSRs would make the full filtering logic adhere to the constraints, no?

Also, this really deserves a comment in the API documentation :).

In fact, I think a pure comment in documentation is enough. Just make x2apic && filter on them "undefined behavior".


Alex

> +
>        kvm_clear_msr_filter(kvm);
> 
>        default_allow = !(filter.flags & KVM_MSR_FILTER_DEFAULT_DENY);
> --
> 2.26.2
> 



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879