| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 26 Sep 2020 00:01:38 +0200
From: Paolo Bonzini <pbonzini@...hat.com>
To: Alexander Graf <graf@...zon.com>, kvm list <kvm@...r.kernel.org>
Cc: Aaron Lewis <aaronlewis@...gle.com>,
Sean Christopherson <sean.j.christopherson@...el.com>,
Jonathan Corbet <corbet@....net>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
Wanpeng Li <wanpengli@...cent.com>,
Jim Mattson <jmattson@...gle.com>,
Joerg Roedel <joro@...tes.org>,
KarimAllah Raslan <karahmed@...zon.de>,
Dan Carpenter <dan.carpenter@...cle.com>,
Maxim Levitsky <mlevitsk@...hat.com>,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 5/8] KVM: x86: SVM: Prevent MSR passthrough when MSR
access is denied
On 25/09/20 16:34, Alexander Graf wrote:
> We will introduce the concept of MSRs that may not be handled in kernel
> space soon. Some MSRs are directly passed through to the guest, effectively
> making them handled by KVM from user space's point of view.
>
> This patch introduces all logic required to ensure that MSRs that
> user space wants trapped are not marked as direct access for guests.
>
> Signed-off-by: Alexander Graf <graf@...zon.com>
VMX and SVM agree about the awful naming of MSR functions... There is
some confusion between msrs and indexes in msrpm_offsets. I'll revisit
this after looking at Sean's pending series that cleans up VMX.
Paolo
> ---
>
> v7 -> v8:
>
> - s/KVM_MSR_ALLOW/KVM_MSR_FILTER/g
> ---
> arch/x86/kvm/svm/svm.c | 77 +++++++++++++++++++++++++++++++++++++-----
> arch/x86/kvm/svm/svm.h | 7 ++++
> 2 files changed, 76 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 41aaee666751..45b0c180f42c 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -91,7 +91,7 @@ static DEFINE_PER_CPU(u64, current_tsc_ratio);
> static const struct svm_direct_access_msrs {
> u32 index; /* Index of the MSR */
> bool always; /* True if intercept is always on */
> -} direct_access_msrs[] = {
> +} direct_access_msrs[MAX_DIRECT_ACCESS_MSRS] = {
> { .index = MSR_STAR, .always = true },
> { .index = MSR_IA32_SYSENTER_CS, .always = true },
> #ifdef CONFIG_X86_64
> @@ -553,15 +553,41 @@ static int svm_cpu_init(int cpu)
>
> }
>
> -static bool valid_msr_intercept(u32 index)
> +static int direct_access_msr_idx(u32 msr)
> {
> - int i;
> + u32 i;
>
> for (i = 0; direct_access_msrs[i].index != MSR_INVALID; i++)
> - if (direct_access_msrs[i].index == index)
> - return true;
> + if (direct_access_msrs[i].index == msr)
> + return i;
>
> - return false;
> + return -EINVAL;
> +}
> +
> +static void set_shadow_msr_intercept(struct kvm_vcpu *vcpu, u32 msr, int read,
> + int write)
> +{
> + struct vcpu_svm *svm = to_svm(vcpu);
> + int idx = direct_access_msr_idx(msr);
> +
> + if (idx == -EINVAL)
> + return;
> +
> + /* Set the shadow bitmaps to the desired intercept states */
> + if (read)
> + set_bit(idx, svm->shadow_msr_intercept.read);
> + else
> + clear_bit(idx, svm->shadow_msr_intercept.read);
> +
> + if (write)
> + set_bit(idx, svm->shadow_msr_intercept.write);
> + else
> + clear_bit(idx, svm->shadow_msr_intercept.write);
> +}
> +
> +static bool valid_msr_intercept(u32 index)
> +{
> + return direct_access_msr_idx(index) != -EINVAL;
> }
>
> static bool msr_write_intercepted(struct kvm_vcpu *vcpu, u32 msr)
> @@ -583,8 +609,8 @@ static bool msr_write_intercepted(struct kvm_vcpu *vcpu, u32 msr)
> return !!test_bit(bit_write, &tmp);
> }
>
> -static void set_msr_interception(struct kvm_vcpu *vcpu, u32 *msrpm, u32 msr,
> - int read, int write)
> +static void set_msr_interception_nosync(struct kvm_vcpu *vcpu, u32 *msrpm,
> + u32 msr, int read, int write)
> {
> u8 bit_read, bit_write;
> unsigned long tmp;
> @@ -596,6 +622,13 @@ static void set_msr_interception(struct kvm_vcpu *vcpu, u32 *msrpm, u32 msr,
> */
> WARN_ON(!valid_msr_intercept(msr));
>
> + /* Enforce non allowed MSRs to trap */
> + if (read && !kvm_msr_allowed(vcpu, msr, KVM_MSR_FILTER_READ))
> + read = 0;
> +
> + if (write && !kvm_msr_allowed(vcpu, msr, KVM_MSR_FILTER_WRITE))
> + write = 0;
> +
> offset = svm_msrpm_offset(msr);
> bit_read = 2 * (msr & 0x0f);
> bit_write = 2 * (msr & 0x0f) + 1;
> @@ -609,6 +642,13 @@ static void set_msr_interception(struct kvm_vcpu *vcpu, u32 *msrpm, u32 msr,
> msrpm[offset] = tmp;
> }
>
> +static void set_msr_interception(struct kvm_vcpu *vcpu, u32 *msrpm, u32 msr,
> + int read, int write)
> +{
> + set_shadow_msr_intercept(vcpu, msr, read, write);
> + set_msr_interception_nosync(vcpu, msrpm, msr, read, write);
> +}
> +
> static u32 *svm_vcpu_alloc_msrpm(void)
> {
> struct page *pages = alloc_pages(GFP_KERNEL_ACCOUNT, MSRPM_ALLOC_ORDER);
> @@ -639,6 +679,25 @@ static void svm_vcpu_free_msrpm(u32 *msrpm)
> __free_pages(virt_to_page(msrpm), MSRPM_ALLOC_ORDER);
> }
>
> +static void svm_msr_filter_changed(struct kvm_vcpu *vcpu)
> +{
> + struct vcpu_svm *svm = to_svm(vcpu);
> + u32 i;
> +
> + /*
> + * Set intercept permissions for all direct access MSRs again. They
> + * will automatically get filtered through the MSR filter, so we are
> + * back in sync after this.
> + */
> + for (i = 0; direct_access_msrs[i].index != MSR_INVALID; i++) {
> + u32 msr = direct_access_msrs[i].index;
> + u32 read = test_bit(i, svm->shadow_msr_intercept.read);
> + u32 write = test_bit(i, svm->shadow_msr_intercept.write);
> +
> + set_msr_interception_nosync(vcpu, svm->msrpm, msr, read, write);
> + }
> +}
> +
> static void add_msr_offset(u32 offset)
> {
> int i;
> @@ -4212,6 +4271,8 @@ static struct kvm_x86_ops svm_x86_ops __initdata = {
> .need_emulation_on_page_fault = svm_need_emulation_on_page_fault,
>
> .apic_init_signal_blocked = svm_apic_init_signal_blocked,
> +
> + .msr_filter_changed = svm_msr_filter_changed,
> };
>
> static struct kvm_x86_init_ops svm_init_ops __initdata = {
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 45496775f0db..07bec0d5aad4 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -31,6 +31,7 @@ static const u32 host_save_user_msrs[] = {
>
> #define NR_HOST_SAVE_USER_MSRS ARRAY_SIZE(host_save_user_msrs)
>
> +#define MAX_DIRECT_ACCESS_MSRS 15
> #define MSRPM_OFFSETS 16
> extern u32 msrpm_offsets[MSRPM_OFFSETS] __read_mostly;
> extern bool npt_enabled;
> @@ -157,6 +158,12 @@ struct vcpu_svm {
> */
> struct list_head ir_list;
> spinlock_t ir_list_lock;
> +
> + /* Save desired MSR intercept (read: pass-through) state */
> + struct {
> + DECLARE_BITMAP(read, MAX_DIRECT_ACCESS_MSRS);
> + DECLARE_BITMAP(write, MAX_DIRECT_ACCESS_MSRS);
> + } shadow_msr_intercept;
> };
>
> struct svm_cpu_data {
>
Powered by blists - more mailing lists