Re: [PATCH 3/9] KVM: x86: SVM: Pass through shadow stack MSRs

[Maxim Levitsky <mlevitsk@redhat.com>]

On Mon, 2023-11-06 at 08:45 -0800, Sean Christopherson wrote:
> On Thu, Nov 02, 2023, Maxim Levitsky wrote:
> > On Wed, 2023-10-18 at 16:57 +0530, Nikunj A. Dadhania wrote:
> > > On 10/17/2023 11:47 PM, John Allen wrote:
> > > In that case, intercept should be cleared from the very beginning.
> > > 
> > > +	{ .index = MSR_IA32_PL0_SSP,                    .always = true },
> > > +	{ .index = MSR_IA32_PL1_SSP,                    .always = true },
> > > +	{ .index = MSR_IA32_PL2_SSP,                    .always = true },
> > > +	{ .index = MSR_IA32_PL3_SSP,                    .always = true },
> > 
> > .always is only true when a MSR is *always* passed through. CET msrs are only
> > passed through when CET is supported.
> > 
> > Therefore I don't expect that we ever add another msr to this list which has
> > .always = true.
> > 
> > In fact the .always = True for X86_64 arch msrs like MSR_GS_BASE/MSR_FS_BASE
> > and such is not 100% correct too - when we start a VM which doesn't have
> > cpuid bit X86_FEATURE_LM, these msrs should not exist and I think that we
> > have a kvm unit test that fails because of this on 32 bit but I didn't bother
> > yet to fix it.
> > 
> > .always probably needs to be dropped completely.
> 
> FWIW, I have a half-baked series to clean up SVM's MSR interception code and
> converge the SVM and VMX APIs.  E.g. set_msr_interception_bitmap()'s inverted
> polarity confuses me every time I look at its usage.

100% agree. Any refactoring here is very welcome!

> 
> I can hunt down the branch if someone plans on tackling this code.

One of the things I don't like that much in the SVM msr's bitmap code
is that it uses L1's msr bitmap when the guest's msr interception is disabled,
and the combined msr bitmap otherwise.

This is very fragile and one mistake away from a CVE.

Since no sane L1 hypervisor will ever allow access to all its msrs from L2,
it might make sense to always use a dedicated MSR bitmap for L2.

Also since all sane L1 hypervisors do use a msr bitmap means that
dedicated code path that doesn't use it is not well tested.

On VMX if I am not mistaken, this is not an issue because either all
MSRS are intercepted or a bitmap is used.

> 

Best regards,
	Maxim Levitsky