Messages in this thread | | | Subject | Re: [PATCH v4 2/3] KVM: x86: Introduce allow list for MSR emulation | From | Alexander Graf <> | Date | Tue, 1 Sep 2020 21:52:13 +0200 |
| |
On 20.08.20 00:49, Jim Mattson wrote: > > On Mon, Aug 3, 2020 at 2:14 PM Alexander Graf <graf@amazon.com> wrote: > >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -901,6 +901,13 @@ struct kvm_hv { >> struct kvm_hv_syndbg hv_syndbg; >> }; >> >> +struct msr_bitmap_range { >> + u32 flags; >> + u32 nmsrs; >> + u32 base; >> + unsigned long *bitmap; >> +}; >> + >> enum kvm_irqchip_mode { >> KVM_IRQCHIP_NONE, >> KVM_IRQCHIP_KERNEL, /* created with KVM_CREATE_IRQCHIP */ >> @@ -1005,6 +1012,9 @@ struct kvm_arch { >> /* Deflect RDMSR and WRMSR to user space when they trigger a #GP */ >> bool user_space_msr_enabled; >> >> + struct msr_bitmap_range msr_allowlist_ranges[10]; > > Why 10? I think this is the only use of this constant, but a macro > would still be nice, especially since the number appears to be > arbitrary. > >> diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h >> index 0780f97c1850..c33fb1d72d52 100644 >> --- a/arch/x86/include/uapi/asm/kvm.h >> +++ b/arch/x86/include/uapi/asm/kvm.h >> @@ -192,6 +192,21 @@ struct kvm_msr_list { >> __u32 indices[0]; >> }; >> >> +#define KVM_MSR_ALLOW_READ (1 << 0) >> +#define KVM_MSR_ALLOW_WRITE (1 << 1) >> + >> +/* Maximum size of the of the bitmap in bytes */ >> +#define KVM_MSR_ALLOWLIST_MAX_LEN 0x600 > > Wouldn't 0x400 be a more natural size, since both Intel and AMD MSR > permission bitmaps cover ranges of 8192 MSRs?
You can always make your bitmaps 0x400 :). I had to choose something that limits our memory footprint, so that user space can't allocate infinite amounts of memory.
> >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index e1139124350f..25e58ceb19de 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -1472,6 +1472,38 @@ void kvm_enable_efer_bits(u64 mask) >> } >> EXPORT_SYMBOL_GPL(kvm_enable_efer_bits); >> >> +static bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type) > > In another thread, when I suggested that a function should return > bool, you said, "'I'm not a big fan of bool returning APIs unless they > have an "is" in their name.' This function doesn't have "is" in its > name. :-)
I've left this unanswered for way too long :). IMHO, passive is fine too, as it implies an "is" in my brain. Or to put it differently:
bad: bool kvm_get_msr() bad: bool kvm_get_msr_user_space() good: bool kvm_msr_blocked() good: bool kvm_msr_allowed() good: bool is_kvm_msr_allowed()
> >> +{ >> + struct kvm *kvm = vcpu->kvm; >> + struct msr_bitmap_range *ranges = kvm->arch.msr_allowlist_ranges; >> + u32 count = kvm->arch.msr_allowlist_ranges_count; > > Shouldn't the read of kvm->arch.msr_allowlist_ranges_count be guarded > by the mutex, below? > >> + u32 i; >> + bool r = false; >> + >> + /* MSR allowlist not set up, allow everything */ >> + if (!count) >> + return true; >> + >> + /* Prevent collision with clear_msr_allowlist */ >> + mutex_lock(&kvm->lock); >> + >> + for (i = 0; i < count; i++) { >> + u32 start = ranges[i].base; >> + u32 end = start + ranges[i].nmsrs; >> + u32 flags = ranges[i].flags; >> + unsigned long *bitmap = ranges[i].bitmap; >> + >> + if ((index >= start) && (index < end) && (flags & type)) { >> + r = !!test_bit(index - start, bitmap); > > The !! seems gratuitous, since r is of type bool. > >> @@ -1483,6 +1515,9 @@ static int __kvm_set_msr(struct kvm_vcpu *vcpu, u32 index, u64 data, >> { >> struct msr_data msr; >> >> + if (!host_initiated && !kvm_msr_allowed(vcpu, index, KVM_MSR_ALLOW_WRITE)) >> + return -ENOENT; > > Perhaps -EPERM is more appropriate here? > >> switch (index) { >> case MSR_FS_BASE: >> case MSR_GS_BASE: >> @@ -1528,6 +1563,9 @@ int __kvm_get_msr(struct kvm_vcpu *vcpu, u32 index, u64 *data, >> struct msr_data msr; >> int ret; >> >> + if (!host_initiated && !kvm_msr_allowed(vcpu, index, KVM_MSR_ALLOW_READ)) >> + return -ENOENT; > > ...and here? > >> +static bool msr_range_overlaps(struct kvm *kvm, struct msr_bitmap_range *range) > > Another bool function with no "is"? :-) > >> +{ >> + struct msr_bitmap_range *ranges = kvm->arch.msr_allowlist_ranges; >> + u32 i, count = kvm->arch.msr_allowlist_ranges_count; >> + bool r = false; >> + >> + for (i = 0; i < count; i++) { >> + u32 start = max(range->base, ranges[i].base); >> + u32 end = min(range->base + range->nmsrs, >> + ranges[i].base + ranges[i].nmsrs); >> + >> + if ((start < end) && (range->flags & ranges[i].flags)) { >> + r = true; >> + break; >> + } >> + } >> + >> + return r; >> +} > > This seems like an awkward constraint. Would it be possible to allow > overlapping ranges as long as the access types don't clash? So, for > example, could I specify an allow list for READ of MSRs 0-0x1ffff and > an allow list for WRITE of MSRs 0-0x1ffff? Actually, I don't see why > you have to prohibit overlapping ranges at all.
I tend to agree. Now that the order is obvious through the new API, we no longer need to check for overlaps.
> > >> +static int kvm_vm_ioctl_clear_msr_allowlist(struct kvm *kvm) >> +{ >> + int i; > > Nit: In earlier code, you use u32 for this index. (I'm actually a fan > of int, myself.)
I usually use int as well because it's easier to type, but doing signed indexes is just so wrong on so many levels :). I'll fix them up too be all u32.
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
| |