lkml.org 
[lkml]   [2020]   [Sep]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v6 5/7] KVM: x86: VMX: Prevent MSR passthrough when MSR access is denied
From
Date


On 04.09.20 04:18, Aaron Lewis wrote:
>
>> +/*
>> + * List of MSRs that can be directly passed to the guest.
>> + * In addition to these x2apic and PT MSRs are handled specially.
>> + */
>> +static u32 vmx_possible_passthrough_msrs[MAX_POSSIBLE_PASSGHROUGH_MSRS] = {
>
> MAX_POSSIBLE_PASSGHROUGH_MSRS should be MAX_POSSIBLE_PASSTHROUGH_MSRS

Ouch. Thanks :).

>
>> + MSR_IA32_SPEC_CTRL,
>> + MSR_IA32_PRED_CMD,
>> + MSR_IA32_TSC,
>> + MSR_FS_BASE,
>> + MSR_GS_BASE,
>> + MSR_KERNEL_GS_BASE,
>> + MSR_IA32_SYSENTER_CS,
>> + MSR_IA32_SYSENTER_ESP,
>> + MSR_IA32_SYSENTER_EIP,
>> + MSR_CORE_C1_RES,
>> + MSR_CORE_C3_RESIDENCY,
>> + MSR_CORE_C6_RESIDENCY,
>> + MSR_CORE_C7_RESIDENCY,
>> +};
>
> Is there any reason not to construct this list on the fly? That could
> help prevent the list from becoming stale over time if this is missed
> when calls to vmx_disable_intercept_for_msr() are added.

The problem is that we have an upper bound of elements that we can store
in the bitmap. We can either make that number arbitrary and then have
really awkward failure modes or be incredibly picky instead.

I went for incredibly picky. If anything goes out of sync, like when
someone adds an MSR to the list without changing
MAX_POSSIBLE_PASSTHROUGH_MSRS, a call to
vmx_{en,dis}able_intercept_for_msr() is done on an MSR that is not part
of the list, we will abort early and in the former case already through
the compiler.

If you can think of a good way to construct the list dynamically and
still have a working bitmap of "desired" passthrough states, I'm all
ears :).

>
>> +
>> /*
>> * These 2 parameters are used to config the controls for Pause-Loop Exiting:
>> * ple_gap: upper bound on the amount of time between two successive
>> @@ -622,6 +642,41 @@ static inline bool report_flexpriority(void)
>> return flexpriority_enabled;
>> }
>
> One thing that seems to be missing is removing MSRs from the
> permission bitmap or resetting the permission bitmap to its original
> state before adding changes on top of it. This would be needed on
> subsequent calls to kvm_vm_ioctl_set_msr_filter(). When that happens
> the original changes made by KVM_REQ_MSR_FILTER_CHANGED need to be
> backed out before applying the new set.

I'm not sure I follow. Subsequent calls to set_msr_filter() will invoke
the "please reset the whole MSR passthrough bitmap to a consistent
state" which will then reapply the in-kvm desired state through the
bitmap and filter state on top on each of those.


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-16 21:49    [W:0.085 / U:0.624 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site