[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 for-4.15] x86/msr: introduce an option for compatible MSR behavior selection
On 09.03.2021 20:57, Andrew Cooper wrote: > On 09/03/2021 11:36, Jan Beulich wrote: >> On 09.03.2021 11:56, Roger Pau Monne wrote: >>> --- a/xen/arch/x86/hvm/svm/svm.c >>> +++ b/xen/arch/x86/hvm/svm/svm.c >>> @@ -1795,6 +1795,7 @@ static int svm_msr_read_intercept(unsigned int msr, >>> uint64_t *msr_content) >>> const struct domain *d = v->domain; >>> struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb; >>> const struct nestedsvm *nsvm = &vcpu_nestedsvm(v); >>> + uint64_t tmp; >>> >>> switch ( msr ) >>> { >> While to some degree a matter of taste, I think such variables needed >> only inside a switch() and not needing an initializer would better >> live in the respective switch()'s scope. > > Actually, I was hoping to make a CODING_SYTLE change removing this as a > permitted construct. > > Recent compilers have hardening features to automatically initialise all > stack variables, because even if your code isn't architecturally buggy, > an attacker can launch speculative attacks the stack rubble. > > However, because of the way the compiler transform works, it cannot > tolerate this specific construct in a switch statement, as there is no > available execution to cope with the implicit "=0" or "=POISON". While an entirely orthogonal issue, since you bring this up here I'd like to point out that this then is a compiler implementation issue, not something one ought to change source code for. The compiler can very well put its initialization at a suitable place, which - for internal handling purposes - could go as far as introducing and artificial block and hence making code structure as if it was { int tmp; switch ( ... ) { case ...: ... } } Trying to limit variable scope as much as possible shouldn't be crippled by incompletely thought through new hardening options. >>> --- a/xen/arch/x86/pv/emul-priv-op.c >>> +++ b/xen/arch/x86/pv/emul-priv-op.c >>> @@ -875,6 +875,7 @@ static int read_msr(unsigned int reg, uint64_t *val, >>> const struct domain *currd = curr->domain; >>> const struct cpuid_policy *cp = currd->arch.cpuid; >>> bool vpmu_msr = false; >>> + uint64_t tmp; >>> int ret; >>> >>> if ( (ret = guest_rdmsr(curr, reg, val)) != X86EMUL_UNHANDLEABLE ) >>> @@ -986,6 +987,12 @@ static int read_msr(unsigned int reg, uint64_t *val, >>> } >>> /* fall through */ >>> default: >>> + if ( currd->arch.msr_relaxed && !rdmsr_safe(reg, tmp) ) >>> + { >>> + *val = 0; >>> + return X86EMUL_OKAY; >>> + } >>> + >>> gdprintk(XENLOG_WARNING, "RDMSR 0x%08x unimplemented\n", reg); >>> break; >>> >>> @@ -1148,6 +1155,9 @@ static int write_msr(unsigned int reg, uint64_t val, >>> } >>> /* fall through */ >>> default: >>> + if ( currd->arch.msr_relaxed && !rdmsr_safe(reg, val) ) >>> + return X86EMUL_OKAY; >>> + >>> gdprintk(XENLOG_WARNING, >>> "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n", >>> reg, val); >> So what are your thoughts wrt my change to this file? Drop it >> altogether and require people to use this new option? Or do you >> see both coexist? In the latter case, since you had suggested >> that I drop the write side of my change - does your changing of >> the write path indicate you've changed your mind? > > I don't think we should legitimise buggy PV behaviour, either by > codifying in the ABI, or providing a knob beyond this one. > > A guest blindly stumbling forward with a missed #GP could very well > worse than crashing hard. There's a fundamental missing piece in your reply here: Do you mean this just as an argument against extending my change to the write side, or as one against my change as a whole? In the latter case, if instead one had to use Roger's new option, the same missing #GP would cause the same possible problems, plus - once the guest has installed a handler - further #GP-s may end up getting suppressed. Jan > Case in point - the 4.15 behaviour spotted a very serious bug in NetBSD > where it tried writing MSR_PAT with its own choice (which wasn't the > same as Xen's choice). The consequence of this bug is getting cache > attributes in pagetables wrong. > > It is unfortunate that multiple bugs have combined to make this mess, > but every instance needs investigating and fixing. Continuing to paper > over the hole doesn't help anyone in the long run. > > ~Andrew >
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |