Fix a goof where KVM fails to re-initialize the set of supported VM types,
resulting in KVM overreporting the set of supported types when a vendor
module is reloaded with incompatible settings. E.g. unload kvm-intel.ko,
reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as
supported.
Fix a similar long-standing bug with supported_mce_cap that is much less
benign, and then harden against us making the same mistake in the future.
Sean Christopherson (3):
KVM: x86: Fully re-initialize supported_vm_types on vendor module load
KVM: x86: Fully re-initialize supported_mce_cap on vendor module load
KVM: x86: Explicitly zero kvm_caps during vendor module load
arch/x86/kvm/x86.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
base-commit: a96cb3bf390eebfead5fc7a2092f8452a7997d1b
--
2.44.0.769.g3c40516874-goog
Recompute the entire set of supported VM types when a vendor module is
loaded, as preserving supported_vm_types across vendor module unload and
reload can result in VM types being incorrectly treated as supported.
E.g. if a vendor module is loaded with TDP enabled, unloaded, and then
reloaded with TDP disabled, KVM_X86_SW_PROTECTED_VM will be incorrectly
retained. Ditto for SEV_VM and SEV_ES_VM and their respective module
params in kvm-amd.ko.
Fixes: 2a955c4db1dd ("KVM: x86: Add supported_vm_types to kvm_caps")
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/x86.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2d2619d3eee4..a65a1012d878 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -94,7 +94,6 @@
struct kvm_caps kvm_caps __read_mostly = {
.supported_mce_cap = MCG_CTL_P | MCG_SER_P,
- .supported_vm_types = BIT(KVM_X86_DEFAULT_VM),
};
EXPORT_SYMBOL_GPL(kvm_caps);
@@ -9776,6 +9775,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
if (r)
goto out_free_percpu;
+ kvm_caps.supported_vm_types = BIT(KVM_X86_DEFAULT_VM);
+
if (boot_cpu_has(X86_FEATURE_XSAVE)) {
host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
kvm_caps.supported_xcr0 = host_xcr0 & KVM_SUPPORTED_XCR0;
--
2.44.0.769.g3c40516874-goog
Zero out all of kvm_caps when loading a new vendor module to ensure that
KVM can't inadvertently rely on global initialization of a field, and add
a comment above the definition of kvm_caps to call out that all fields
needs to be explicitly computed during vendor module load.
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/x86.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 44ce187bad89..8f3979d5fc80 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -92,6 +92,11 @@
#define MAX_IO_MSRS 256
#define KVM_MAX_MCE_BANKS 32
+/*
+ * Note, kvm_caps fields should *never* have default values, all fields must be
+ * recomputed from scratch during vendor module load, e.g. to account for a
+ * vendor module being reloaded with different module parameters.
+ */
struct kvm_caps kvm_caps __read_mostly;
EXPORT_SYMBOL_GPL(kvm_caps);
@@ -9755,6 +9760,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
return -EIO;
}
+ memset(&kvm_caps, 0, sizeof(kvm_caps));
+
x86_emulator_cache = kvm_alloc_emulator_cache();
if (!x86_emulator_cache) {
pr_err("failed to allocate cache for x86 emulator\n");
--
2.44.0.769.g3c40516874-goog
Effectively reset supported_mce_cap on vendor module load to ensure that
capabilities aren't unintentionally preserved across module reload, e.g.
if kvm-intel.ko added a module param to control LMCE support, or if
someone somehow managed to load a vendor module that doesn't support LMCE
after loading and unloading kvm-intel.ko.
Practically speaking, this bug is a non-issue as kvm-intel.ko doesn't have
a module param for LMCE, and there is no system in the world that supports
both kvm-intel.ko and kvm-amd.ko.
Fixes: c45dcc71b794 ("KVM: VMX: enable guest access to LMCE related MSRs")
Signed-off-by: Sean Christopherson <[email protected]>
---
arch/x86/kvm/x86.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a65a1012d878..44ce187bad89 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -92,9 +92,7 @@
#define MAX_IO_MSRS 256
#define KVM_MAX_MCE_BANKS 32
-struct kvm_caps kvm_caps __read_mostly = {
- .supported_mce_cap = MCG_CTL_P | MCG_SER_P,
-};
+struct kvm_caps kvm_caps __read_mostly;
EXPORT_SYMBOL_GPL(kvm_caps);
#define ERR_PTR_USR(e) ((void __user *)ERR_PTR(e))
@@ -9776,6 +9774,7 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
goto out_free_percpu;
kvm_caps.supported_vm_types = BIT(KVM_X86_DEFAULT_VM);
+ kvm_caps.supported_mce_cap = MCG_CTL_P | MCG_SER_P;
if (boot_cpu_has(X86_FEATURE_XSAVE)) {
host_xcr0 = xgetbv(XCR_XFEATURE_ENABLED_MASK);
--
2.44.0.769.g3c40516874-goog
On Tue, 2024-04-23 at 09:53 -0700, Sean Christopherson wrote:
> Zero out all of kvm_caps when loading a new vendor module to ensure that
> KVM can't inadvertently rely on global initialization of a field, and add
> a comment above the definition of kvm_caps to call out that all fields
> needs to be explicitly computed during vendor module load.
>
> Signed-off-by: Sean Christopherson <[email protected]>
> ---
> arch/x86/kvm/x86.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 44ce187bad89..8f3979d5fc80 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -92,6 +92,11 @@
> #define MAX_IO_MSRS 256
> #define KVM_MAX_MCE_BANKS 32
>
> +/*
> + * Note, kvm_caps fields should *never* have default values, all fields must be
> + * recomputed from scratch during vendor module load, e.g. to account for a
> + * vendor module being reloaded with different module parameters.
> + */
> struct kvm_caps kvm_caps __read_mostly;
> EXPORT_SYMBOL_GPL(kvm_caps);
>
> @@ -9755,6 +9760,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
> return -EIO;
> }
>
> + memset(&kvm_caps, 0, sizeof(kvm_caps));
> +
> x86_emulator_cache = kvm_alloc_emulator_cache();
> if (!x86_emulator_cache) {
> pr_err("failed to allocate cache for x86 emulator\n");
Why do the memset() here particularly?
Isn't it better to put ...
memset(&kvm_caps, 0, sizeof(kvm_caps));
kvm_caps.supported_vm_types = BIT(KVM_X86_DEFAULT_VM);
kvm_caps.supported_mce_cap = MCG_CTL_P | MCG_SER_P;
... together so it can be easily seen?
We can even have a helper to do above to "reset kvm_caps to default
values" I think.
On Wed, Apr 24, 2024, Kai Huang wrote:
> On Tue, 2024-04-23 at 09:53 -0700, Sean Christopherson wrote:
> > Zero out all of kvm_caps when loading a new vendor module to ensure that
> > KVM can't inadvertently rely on global initialization of a field, and add
> > a comment above the definition of kvm_caps to call out that all fields
> > needs to be explicitly computed during vendor module load.
> >
> > Signed-off-by: Sean Christopherson <[email protected]>
> > ---
> > arch/x86/kvm/x86.c | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 44ce187bad89..8f3979d5fc80 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -92,6 +92,11 @@
> > #define MAX_IO_MSRS 256
> > #define KVM_MAX_MCE_BANKS 32
> >
> > +/*
> > + * Note, kvm_caps fields should *never* have default values, all fields must be
> > + * recomputed from scratch during vendor module load, e.g. to account for a
> > + * vendor module being reloaded with different module parameters.
> > + */
> > struct kvm_caps kvm_caps __read_mostly;
> > EXPORT_SYMBOL_GPL(kvm_caps);
> >
> > @@ -9755,6 +9760,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
> > return -EIO;
> > }
> >
> > + memset(&kvm_caps, 0, sizeof(kvm_caps));
> > +
> > x86_emulator_cache = kvm_alloc_emulator_cache();
> > if (!x86_emulator_cache) {
> > pr_err("failed to allocate cache for x86 emulator\n");
>
> Why do the memset() here particularly?
So that it happens as early as possible, e.g. in case kvm_mmu_vendor_module_init()
or some other function comes along and modifies kvm_caps.
> Isn't it better to put ...
>
> memset(&kvm_caps, 0, sizeof(kvm_caps));
> kvm_caps.supported_vm_types = BIT(KVM_X86_DEFAULT_VM);
> kvm_caps.supported_mce_cap = MCG_CTL_P | MCG_SER_P;
>
> ... together so it can be easily seen?
>
> We can even have a helper to do above to "reset kvm_caps to default
> values" I think.
Hmm, I don't think a helper is necessary, but I do agree that having all of the
explicit initialization in one place would be better. The alternative would be
to use |= for BIT(KVM_X86_DEFAULT_VM), and MCG_CTL_P | MCG_SER_P, but I don't
think that would be an improvement. I'll tweak the first two patches to set the
hardcoded caps earlier.
The main reason I don't want to add a helper is that coming up with a name would
be tricky. E.g. "kvm_reset_caps()" isn't a great fit because the caps are "reset"
throughout module loading. "kvm_set_default_caps()" kinda fits, but they aren't
so much that they are KVM's defaults, rather they are the caps that KVM can always
support regardless of hardware support, e.g. supported_xcr0 isn't optional, it
just depends on hardware.
On Wed, 2024-04-24 at 08:33 -0700, Sean Christopherson wrote:
> On Wed, Apr 24, 2024, Kai Huang wrote:
> > On Tue, 2024-04-23 at 09:53 -0700, Sean Christopherson wrote:
> > > Zero out all of kvm_caps when loading a new vendor module to ensure that
> > > KVM can't inadvertently rely on global initialization of a field, and add
> > > a comment above the definition of kvm_caps to call out that all fields
> > > needs to be explicitly computed during vendor module load.
> > >
> > > Signed-off-by: Sean Christopherson <[email protected]>
> > > ---
> > > arch/x86/kvm/x86.c | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 44ce187bad89..8f3979d5fc80 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -92,6 +92,11 @@
> > > #define MAX_IO_MSRS 256
> > > #define KVM_MAX_MCE_BANKS 32
> > >
> > > +/*
> > > + * Note, kvm_caps fields should *never* have default values, all fields must be
> > > + * recomputed from scratch during vendor module load, e.g. to account for a
> > > + * vendor module being reloaded with different module parameters.
> > > + */
> > > struct kvm_caps kvm_caps __read_mostly;
> > > EXPORT_SYMBOL_GPL(kvm_caps);
> > >
> > > @@ -9755,6 +9760,8 @@ int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops)
> > > return -EIO;
> > > }
> > >
> > > + memset(&kvm_caps, 0, sizeof(kvm_caps));
> > > +
> > > x86_emulator_cache = kvm_alloc_emulator_cache();
> > > if (!x86_emulator_cache) {
> > > pr_err("failed to allocate cache for x86 emulator\n");
> >
> > Why do the memset() here particularly?
>
> So that it happens as early as possible, e.g. in case kvm_mmu_vendor_module_init()
> or some other function comes along and modifies kvm_caps.
>
> > Isn't it better to put ...
> >
> > memset(&kvm_caps, 0, sizeof(kvm_caps));
> > kvm_caps.supported_vm_types = BIT(KVM_X86_DEFAULT_VM);
> > kvm_caps.supported_mce_cap = MCG_CTL_P | MCG_SER_P;
> >
> > ... together so it can be easily seen?
> >
> > We can even have a helper to do above to "reset kvm_caps to default
> > values" I think.
>
> Hmm, I don't think a helper is necessary, but I do agree that having all of the
> explicit initialization in one place would be better. The alternative would be
> to use |= for BIT(KVM_X86_DEFAULT_VM), and MCG_CTL_P | MCG_SER_P, but I don't
> think that would be an improvement. I'll tweak the first two patches to set the
> hardcoded caps earlier.
>
> The main reason I don't want to add a helper is that coming up with a name would
> be tricky. E.g. "kvm_reset_caps()" isn't a great fit because the caps are "reset"
> throughout module loading. "kvm_set_default_caps()" kinda fits, but they aren't
> so much that they are KVM's defaults, rather they are the caps that KVM can always
> support regardless of hardware support, e.g. supported_xcr0 isn't optional, it
> just depends on hardware.
Personally I am fine with kvm_set_default_caps(), but obviously no strong
opinion here. :-)
On 4/24/2024 12:53 AM, Sean Christopherson wrote:
> Fix a goof where KVM fails to re-initialize the set of supported VM types,
> resulting in KVM overreporting the set of supported types when a vendor
> module is reloaded with incompatible settings. E.g. unload kvm-intel.ko,
> reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as
> supported.
Hah, this reminds me of the bug of msrs_to_save[] and etc.
7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists")
The series looks good to me.
With v2 to move the reset of kvm_cap and set the
hardcoded caps earlier,
Reviewed-by: Xiaoyao Li <[email protected]>
> Fix a similar long-standing bug with supported_mce_cap that is much less
> benign, and then harden against us making the same mistake in the future.
>
> Sean Christopherson (3):
> KVM: x86: Fully re-initialize supported_vm_types on vendor module load
> KVM: x86: Fully re-initialize supported_mce_cap on vendor module load
> KVM: x86: Explicitly zero kvm_caps during vendor module load
>
> arch/x86/kvm/x86.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
>
> base-commit: a96cb3bf390eebfead5fc7a2092f8452a7997d1b
On Thu, Apr 25, 2024, Xiaoyao Li wrote:
> On 4/24/2024 12:53 AM, Sean Christopherson wrote:
> > Fix a goof where KVM fails to re-initialize the set of supported VM types,
> > resulting in KVM overreporting the set of supported types when a vendor
> > module is reloaded with incompatible settings. E.g. unload kvm-intel.ko,
> > reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as
> > supported.
>
> Hah, this reminds me of the bug of msrs_to_save[] and etc.
>
> 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists")
Yeah, and we had the same bug with allow_smaller_maxphyaddr
88213da23514 ("kvm: x86: disable the narrow guest module parameter on unload")
If the side effects of linking kvm.ko into kvm-{amd,intel}.ko weren't so painful
for userspace, I would more seriously consider pursuing that in advance of
multi-KVM[*]. Because having KVM be fully self-contained has some *really* nice
properties, e.g. eliminates this entire class of bugs, eliminates a huge pile of
exports, etc.
: > Since the symbols in the new module are invisible outside, I recommend:
: > new kvm_intel.ko = kvm_intel.ko + kvm.ko
: > new kvm_amd.ko = kvm_amd.ko + kvm.ko
:
: Yeah, Paolo also suggested this at LPC.
[*] https://lore.kernel.org/all/[email protected]
On Thu, 2024-04-25 at 07:30 -0700, Sean Christopherson wrote:
> On Thu, Apr 25, 2024, Xiaoyao Li wrote:
> > On 4/24/2024 12:53 AM, Sean Christopherson wrote:
> > > Fix a goof where KVM fails to re-initialize the set of supported VM types,
> > > resulting in KVM overreporting the set of supported types when a vendor
> > > module is reloaded with incompatible settings. E.g. unload kvm-intel.ko,
> > > reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as
> > > supported.
> >
> > Hah, this reminds me of the bug of msrs_to_save[] and etc.
> >
> > 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists")
>
> Yeah, and we had the same bug with allow_smaller_maxphyaddr
>
> 88213da23514 ("kvm: x86: disable the narrow guest module parameter on unload")
>
> If the side effects of linking kvm.ko into kvm-{amd,intel}.ko weren't so painful
> for userspace,
>
Do we have any real side effects for _userspace_ here?
> I would more seriously consider pursuing that in advance of
> multi-KVM[*]. Because having KVM be fully self-contained has some *really* nice
> properties, e.g. eliminates this entire class of bugs, eliminates a huge pile of
> exports, etc.
>
> : > Since the symbols in the new module are invisible outside, I recommend:
> : > new kvm_intel.ko = kvm_intel.ko + kvm.ko
> : > new kvm_amd.ko = kvm_amd.ko + kvm.ko
> :
> : Yeah, Paolo also suggested this at LPC.
>
> [*] https://lore.kernel.org/all/[email protected]
>
+1. This makes life a lot easier.
On Fri, Apr 26, 2024, Kai Huang wrote:
> On Thu, 2024-04-25 at 07:30 -0700, Sean Christopherson wrote:
> > On Thu, Apr 25, 2024, Xiaoyao Li wrote:
> > > On 4/24/2024 12:53 AM, Sean Christopherson wrote:
> > > > Fix a goof where KVM fails to re-initialize the set of supported VM types,
> > > > resulting in KVM overreporting the set of supported types when a vendor
> > > > module is reloaded with incompatible settings. E.g. unload kvm-intel.ko,
> > > > reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as
> > > > supported.
> > >
> > > Hah, this reminds me of the bug of msrs_to_save[] and etc.
> > >
> > > 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists")
> >
> > Yeah, and we had the same bug with allow_smaller_maxphyaddr
> >
> > 88213da23514 ("kvm: x86: disable the narrow guest module parameter on unload")
> >
> > If the side effects of linking kvm.ko into kvm-{amd,intel}.ko weren't so painful
> > for userspace,
> >
>
> Do we have any real side effects for _userspace_ here?
kvm.ko ceasing to exist, and "everything" being tied to the vendor module is the
big problem. E.g. params from the kernel command line for kvm.??? will become
ineffective, etc. Some of that can be handled in the kernel, e.g. KVM can create
a sysfs symlink so that the accesses through sysfs continue to work, but AFAIK
params don't supporting such aliasing/links.
I don't think there are any deal breakers, but I don't expect it to Just Work either.
On 27/04/2024 3:47 am, Sean Christopherson wrote:
> On Fri, Apr 26, 2024, Kai Huang wrote:
>> On Thu, 2024-04-25 at 07:30 -0700, Sean Christopherson wrote:
>>> On Thu, Apr 25, 2024, Xiaoyao Li wrote:
>>>> On 4/24/2024 12:53 AM, Sean Christopherson wrote:
>>>>> Fix a goof where KVM fails to re-initialize the set of supported VM types,
>>>>> resulting in KVM overreporting the set of supported types when a vendor
>>>>> module is reloaded with incompatible settings. E.g. unload kvm-intel.ko,
>>>>> reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as
>>>>> supported.
>>>>
>>>> Hah, this reminds me of the bug of msrs_to_save[] and etc.
>>>>
>>>> 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists")
>>>
>>> Yeah, and we had the same bug with allow_smaller_maxphyaddr
>>>
>>> 88213da23514 ("kvm: x86: disable the narrow guest module parameter on unload")
>>>
>>> If the side effects of linking kvm.ko into kvm-{amd,intel}.ko weren't so painful
>>> for userspace,
>>>
>>
>> Do we have any real side effects for _userspace_ here?
>
> kvm.ko ceasing to exist, and "everything" being tied to the vendor module is the
> big problem. E.g. params from the kernel command line for kvm.??? will become
> ineffective, etc. Some of that can be handled in the kernel, e.g. KVM can create
> a sysfs symlink so that the accesses through sysfs continue to work, but AFAIK
> params don't supporting such aliasing/links.
>
> I don't think there are any deal breakers, but I don't expect it to Just Work either.
Perhaps we can make the kvm.ko as a dummy module which only keeps the
module parameters for backward compatibility?
On Mon, Apr 29, 2024, Kai Huang wrote:
> On 27/04/2024 3:47 am, Sean Christopherson wrote:
> > On Fri, Apr 26, 2024, Kai Huang wrote:
> > > On Thu, 2024-04-25 at 07:30 -0700, Sean Christopherson wrote:
> > > > On Thu, Apr 25, 2024, Xiaoyao Li wrote:
> > > > > On 4/24/2024 12:53 AM, Sean Christopherson wrote:
> > > > > > Fix a goof where KVM fails to re-initialize the set of supported VM types,
> > > > > > resulting in KVM overreporting the set of supported types when a vendor
> > > > > > module is reloaded with incompatible settings. E.g. unload kvm-intel.ko,
> > > > > > reload with ept=0, and KVM will incorrectly treat SW_PROTECTED_VM as
> > > > > > supported.
> > > > >
> > > > > Hah, this reminds me of the bug of msrs_to_save[] and etc.
> > > > >
> > > > > 7a5ee6edb42e ("KVM: X86: Fix initialization of MSR lists")
> > > >
> > > > Yeah, and we had the same bug with allow_smaller_maxphyaddr
> > > >
> > > > 88213da23514 ("kvm: x86: disable the narrow guest module parameter on unload")
> > > >
> > > > If the side effects of linking kvm.ko into kvm-{amd,intel}.ko weren't so painful
> > > > for userspace,
> > > >
> > >
> > > Do we have any real side effects for _userspace_ here?
> >
> > kvm.ko ceasing to exist, and "everything" being tied to the vendor module is the
> > big problem. E.g. params from the kernel command line for kvm.??? will become
> > ineffective, etc. Some of that can be handled in the kernel, e.g. KVM can create
> > a sysfs symlink so that the accesses through sysfs continue to work, but AFAIK
> > params don't supporting such aliasing/links.
> >
> > I don't think there are any deal breakers, but I don't expect it to Just Work either.
>
> Perhaps we can make the kvm.ko as a dummy module which only keeps the module
> parameters for backward compatibility?
Keeping parameters in a dummy kvm.ko would largely defeat the purpose of linking
everything into vendor modules, i.e. would make it possible for the parameters to
hold a stale value.