Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200902125935.20646-1-graf@amazon.com> <20200902125935.20646-6-graf@amazon.com>
In-Reply-To: <20200902125935.20646-6-graf@amazon.com>
From:   Aaron Lewis <aaronlewis@google.com>
Date:   Thu, 3 Sep 2020 19:18:55 -0700
Message-ID: <CAAAPnDH2D6fANhZzy3fAL2XKO4ROrvbOoqPme2Ww6q5XcVJfog@mail.gmail.com>
Subject: Re: [PATCH v6 5/7] KVM: x86: VMX: Prevent MSR passthrough when MSR
 access is denied
To:     Alexander Graf <graf@amazon.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Sean Christopherson <sean.j.christopherson@intel.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Joerg Roedel <joro@8bytes.org>,
        KarimAllah Raslan <karahmed@amazon.de>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        kvm list <kvm@vger.kernel.org>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> +/*
> + * 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

> +       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.

> +
>  /*
>   * 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.