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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fa69866979cdb8ad445d0dffe98d6158288af339.camel@redhat.com>
Date: Tue, 20 Aug 2024 15:13:04 +0300
From: mlevitsk@...hat.com
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
 Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>,
 Dave Hansen <dave.hansen@...ux.intel.com>, Borislav Petkov <bp@...en8.de>, 
 linux-kernel@...r.kernel.org, "H. Peter Anvin" <hpa@...or.com>, Chao Gao
 <chao.gao@...el.com>
Subject: Re: [PATCH v3 1/4] KVM: x86: relax canonical check for some x86
 architectural msrs

У пт, 2024-08-16 у 14:49 -0700, Sean Christopherson пише:
> > > > On Thu, Aug 15, 2024, Maxim Levitsky wrote:
> > > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > > > > > > index ce7c00894f32..2e83f7d74591 100644
> > > > > > > > --- a/arch/x86/kvm/x86.c
> > > > > > > > +++ b/arch/x86/kvm/x86.c
> > > > > > > > @@ -302,6 +302,31 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
> > > > > > > >                        sizeof(kvm_vcpu_stats_desc),
> > > > > > > >  };
> > > > > > > >  
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * Most x86 arch MSR values which contain linear addresses like
> > > > 
> > > > Is it most, or all?  I'm guessing all?

I can't be sure that all of them are like that - there could be some outliers that behave differently.

One of the things my work at Intel taught me is that there is nothing consistent
in x86 spec, anything is possible and nothing can be assumed.

I dealt only with those msrs, that KVM checks for canonicality, therefore I use the word 
'most'. There could be other msrs that are not known to me and/or to KVM.

I can write 'some' if you prefer.

> > > > 
> > > > > > > > + * segment bases, addresses that are used in instructions (e.g SYSENTER),
> > > > > > > > + * have static canonicality checks,
> > > > 
> > > > Weird and early line breaks.
> > > > 
> > > > How about this?
> > > > 
> > > > /*
> > > >  * The canonicality checks for MSRs that hold linear addresses, e.g. segment
> > > >  * bases, SYSENTER targets, etc., are static, in the sense that they are based
> > > >  * on CPU _support_ for 5-level paging, not the state of CR4.LA57.
> > > > 
> > > > > > > > + * size of whose depends only on CPU's support for 5-level
> > > > > > > > + * paging, rather than state of CR4.LA57.
> > > > > > > > + *
> > > > > > > > + * In addition to that, some of these MSRS are directly passed
> > > > > > > > + * to the guest (e.g MSR_KERNEL_GS_BASE) thus even if the guest
> > > > > > > > + * doen't have LA57 enabled in its CPUID, for consistency with
> > > > > > > > + * CPUs' ucode, it is better to pivot the check around host
> > > > > > > > + * support for 5 level paging.
> > > > 
> > > > I think we should elaborate on why it's better.  It only takes another line or
> > > > two, and that way we don't forget the edge cases that make properly emulating
> > > > guest CPUID a bad idea.

OK, will do.

> > > > 
> > > >  * This creates a virtualization hole where a guest writes to passthrough MSRs
> > > >  * may incorrectly succeed if the CPU supports LA57, but the vCPU does not
> > > >  * (because hardware has no awareness of guest CPUID).  Do not try to plug this
> > > >  * hole, i.e. emulate the behavior for intercepted accesses, as injecting #GP
> > > >  * depending on whether or not KVM happens to emulate a WRMSR would result in
> > > >  * non-deterministic behavior, and could even allow L2 to crash L1, e.g. if L1
> > > >  * passes through an MSR to L2, and then tries to save+restore L2's value.
> > > >  */
> > > > 
> > > > > > > > +
> > > > > > > > +static u8  max_host_supported_virt_addr_bits(void)
> > > > 
> > > > Any objection to dropping the "supported", i.e. going with max_host_virt_addr_bits()?
> > > > Mostly to shorten the name, but also because "supported" suggests there's software
> > > > involvement, e.g. the max supported by the kernel/KVM, which isn't the case.

Doesn't matter to me.

> > > > 
> > > > If you're ok with the above, I'll fixup when applying.
> > > > 

Best regards,
   Maxim Levitsky


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ