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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.

~Andrew