Re: [PATCH v3 7/8] x86/hvm: Disallow access to unknown MSRs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.09.2020 11:44, Andrew Cooper wrote:
> On 04/09/2020 09:53, Jan Beulich wrote:
>> On 01.09.2020 12:54, Roger Pau Monne wrote:
>>> @@ -3290,11 +3288,6 @@ static int vmx_msr_write_intercept(unsigned int msr, 
>>> uint64_t msr_content)
>>>          __vmwrite(GUEST_IA32_DEBUGCTL, msr_content);
>>>          break;
>>>  
>>> -    case MSR_IA32_FEATURE_CONTROL:
>>> -    case MSR_IA32_VMX_BASIC ... MSR_IA32_VMX_VMFUNC:
>>> -        /* None of these MSRs are writeable. */
>>> -        goto gp_fault;
>> I understand Andrew did ask for this (and I didn't look closely
>> when I saw the comment), but ...
>>
>>> @@ -3320,10 +3313,9 @@ static int vmx_msr_write_intercept(unsigned int msr, 
>>> uint64_t msr_content)
>>>               is_last_branch_msr(msr) )
>>>              break;
>>>  
>>> -        /* Match up with the RDMSR side; ultimately this should go away. */
>>> -        if ( rdmsr_safe(msr, msr_content) == 0 )
>>> -            break;
>>> -
>>> +        gdprintk(XENLOG_WARNING,
>>> +                 "WRMSR 0x%08x val 0x%016"PRIx64" unimplemented\n",
>>> +                 msr, msr_content);
>>>          goto gp_fault;
>> ... above from here is logic that handling of these MSRs now goes
>> through. I'm particularly worried about vmx_write_guest_msr(),
>> which blindly updates the value of any MSR it can find, i.e. if
>> any r/o MSR (from the set above, or even more generally) ever got
>> added to this vCPU-specific set, the r/o-ness would no longer be
>> maintained. Do we perhaps need to white-list MSRs for which
>> vmx_write_guest_msr() may get called here?
> 
> If a read-only MSR ever actually gets into the load/save list, we'll
> take a VMEntry failure (guest load) or SHUTDOWN (host load) as a
> consequence of microcode takes a #GP fault.

Oh, of course. In order to surface a value different from the hardware's
one has to intercept such MSRs.

> However, restricting the content of this catch-all clause to nothing
> (but the printk) is something I have planned as part of the ongoing MSR
> cleanup work.

Good to know.

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan