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 <[email protected]>
Cc: Aaron Lewis <[email protected]>
Cc: Peter Xu <[email protected]>
Cc: Sean Christopherson <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
---
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;
+
kvm_clear_msr_filter(kvm);
default_allow = !(filter.flags & KVM_MSR_FILTER_DEFAULT_DENY);
--
2.26.2
On Mon, Oct 19, 2020 at 01:05:19PM -0400, Paolo Bonzini wrote:
> 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 <[email protected]>
> Cc: Aaron Lewis <[email protected]>
> Cc: Peter Xu <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
Acked-by: Peter Xu <[email protected]>
--
Peter Xu
> Am 19.10.2020 um 19:08 schrieb Paolo Bonzini <[email protected]>:
>
> 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 <[email protected]>
> Cc: Aaron Lewis <[email protected]>
> Cc: Peter Xu <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
> ---
> 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
On 19/10/20 19:45, Graf (AWS), Alexander wrote:
>> + * 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?
Right; or more precisely, that is handled by Sean's patch that he had
posted earlier. This patch only makes it impossible to set up such a
filter.
(That said, this is a quick patch that I posted yesterday just before
dinner. I'll send out the real deal with docs fixes and better commit
message).
Paolo
> 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".
On 20.10.20 11:27, Paolo Bonzini wrote:
>
> On 19/10/20 19:45, Graf (AWS), Alexander wrote:
>>> + * 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?
>
> Right; or more precisely, that is handled by Sean's patch that he had
> posted earlier. This patch only makes it impossible to set up such a
> filter.
What I'm saying is that a "filter rule" can either mean "allow" or
"deny". Here you're only checking if there is a rule. What you want to
filter for is whether there is a denying rule (including default
fallback) for x2apic after all rules are in place.
Imagine you add the following filter:
{
count: 1,
default_allow: false,
ranges: [
{
flags: KVM_MSR_FILTER_READ,
nmsrs: 1,
base: MSR_EFER,
bitmap: { 1 },
},
],
}
That filter would set all x2apic registers to "deny", but would not be
caught by the code above. Conversely, a range that explicitly allows
x2apic ranges with default_allow=0 would be rejected by this patch.
Alex
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
On 20/10/20 11:48, Alexander Graf wrote:
>
> count: 1,
> default_allow: false,
> ranges: [
> {
> flags: KVM_MSR_FILTER_READ,
> nmsrs: 1,
> base: MSR_EFER,
> bitmap: { 1 },
> },
> ],
> }
>
> That filter would set all x2apic registers to "deny", but would not be
> caught by the code above. Conversely, a range that explicitly allows
> x2apic ranges with default_allow=0 would be rejected by this patch.
Yes, but the idea is that x2apic registers are always allowed, even
overriding default_allow, and therefore it makes no sense to have them
in a range. The patch is only making things fail early for userspace,
the policy is defined by Sean's patch.
Paolo
On 20.10.20 12:34, Paolo Bonzini wrote:
>
> On 20/10/20 11:48, Alexander Graf wrote:
>>
>> count: 1,
>> default_allow: false,
>> ranges: [
>> {
>> flags: KVM_MSR_FILTER_READ,
>> nmsrs: 1,
>> base: MSR_EFER,
>> bitmap: { 1 },
>> },
>> ],
>> }
>>
>> That filter would set all x2apic registers to "deny", but would not be
>> caught by the code above. Conversely, a range that explicitly allows
>> x2apic ranges with default_allow=0 would be rejected by this patch.
>
> Yes, but the idea is that x2apic registers are always allowed, even
> overriding default_allow, and therefore it makes no sense to have them
> in a range. The patch is only making things fail early for userspace,
> the policy is defined by Sean's patch.
I don't think we should fail on the following:
{
default_allow: false,
ranges: [
{
flags: KVM_MSR_FILTER_READ,
nmsrs: 4096,
base: 0,
bitmap: { 1, 1, 1, 1, [...] },
},
{
flags: KVM_MSR_FILTER_READ,
nmsrs: 4096,
base: 0xc0000000,
bitmap: { 1, 1, 1, 1, [...] },
},
],
}
as a way to say "everything in normal ranges is allowed, the rest please
deflect". Or even just to set default policies with less ranges.
Or to say it differently: Why can't we just check explicitly after
setting up all filter lists whether x2apic MSRs are *denied*? If so,
clear the filter and return -EINVAL.
That still leaves the case where x2apic is not handled in-kernel, but
I'm perfectly happy to ignore that one as "user space should not care" :).
Alex
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
On 20/10/20 12:52, Alexander Graf wrote:
>>
>> Yes, but the idea is that x2apic registers are always allowed, even
>> overriding default_allow, and therefore it makes no sense to have them
>> in a range. The patch is only making things fail early for userspace,
>> the policy is defined by Sean's patch.
>
> I don't think we should fail on the following:
>
> {
> default_allow: false,
> ranges: [
> {
> flags: KVM_MSR_FILTER_READ,
> nmsrs: 4096,
> base: 0,
> bitmap: { 1, 1, 1, 1, [...] },
> },
> {
> flags: KVM_MSR_FILTER_READ,
> nmsrs: 4096,
> base: 0xc0000000,
> bitmap: { 1, 1, 1, 1, [...] },
> },
> ],
> }
>
> as a way to say "everything in normal ranges is allowed, the rest please
> deflect". Or even just to set default policies with less ranges.
>
> Or to say it differently: Why can't we just check explicitly after
> setting up all filter lists whether x2apic MSRs are *denied*? If so,
> clear the filter and return -EINVAL.
Hmm, if you start looking at the bitmaps setting up default-deny
policies correctly is almost impossible :/ because you'd have to ensure
that you have at least one range covering the x2apic MSRs. I'll just
document that x2APIC MSRs ignore the filter.
Paolo