lkml.org 
[lkml]   [2020]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 2/3] KVM: x86: Introduce allow list for MSR emulation
From
Date


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


\
 
 \ /
  Last update: 2020-09-01 21:53    [W:0.117 / U:1.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site