Re: [PATCH v3 for-4.15] x86/msr: introduce an option for compatible MSR behavior selection

[Jan Beulich <jbeulich@xxxxxxxx>]

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
>