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

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 <xiaoyao.li@intel.com>

> 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/ZWYtDGH5p4RpGYBw@google.com

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/ZWYtDGH5p4RpGYBw@google.com
> 

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