Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Graf (AWS), Alexander" <graf@amazon.de>
To:     Paolo Bonzini <pbonzini@redhat.com>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        Aaron Lewis <aaronlewis@google.com>,
        Peter Xu <peterx@redhat.com>,
        Sean Christopherson <sean.j.christopherson@intel.com>
Subject: Re: [PATCH] KVM: VMX: Forbid userspace MSR filters for x2APIC
Thread-Topic: [PATCH] KVM: VMX: Forbid userspace MSR filters for x2APIC
Thread-Index: AQHWpj+x3+JT+Cl9gkCqCDyMCFO5/w==
Date:   Mon, 19 Oct 2020 17:45:54 +0000
Message-ID: <618E2129-7AB5-4F0D-A6C9-E782937FE935@amazon.de>
References: <20201019170519.1855564-1-pbonzini@redhat.com>
In-Reply-To: <20201019170519.1855564-1-pbonzini@redhat.com>
Accept-Language: en-US
Content-Language: de-DE
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


> 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 TP=
R,
> 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, str=
uct kvm_msr_filter_range *user
>        return r;
> }
> =

> +static bool range_overlaps_x2apic(struct kvm_msr_filter_range *range)
> +{
> +       u32 start =3D range->base;
> +       u32 end =3D start + range->nmsrs;
> +       return start <=3D 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 =3D 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 =3D 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 fil=
ter to allow access to x2apic MSRs would make the full filtering logic adhe=
re 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 x2api=
c && filter on them "undefined behavior".


Alex

> +
>        kvm_clear_msr_filter(kvm);
> =

>        default_allow =3D !(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