Re: [PATCH] x86/hvm: Further restrict access to x2apic MSRs

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 17/10/14 10:59, Jan Beulich wrote:
>>>> On 16.10.14 at 20:04, <andrew.cooper3@citrix.com> wrote:
>> Therefore, direct the entire reserved range (0x840-0xbff) unconditionally at a
>> side effect of this change is that hvm_x2apic_msr_read() can now no longer
>> read beyond the bounds of the vlapic->regs array (which is 1/4 the size of the
>> page backing it).
> There seems to be stuff missing between "at" and "a"; at least I can't
> make sense of the sentence.

Oops.  The paragraph read "...unconditionally at a\n#GP fault...".  It
appears that git considered the line as a comment and dropped it from a
resulting message.

>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4355,8 +4355,9 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t *msr_content)
>>          *msr_content = vcpu_vlapic(v)->hw.apic_base_msr;
>>          break;
>>  
>> -    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0xff:
>> +    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3f:
>>          if ( hvm_x2apic_msr_read(v, msr, msr_content) )
>> +    case MSR_IA32_APICBASE_MSR + 0x40 ... MSR_IA32_APICBASE_MSR + 0x3ff:
>>              goto gp_fault;
>>          break;
>>  
>> @@ -4482,8 +4483,9 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t msr_content)
>>          vlapic_tdt_msr_set(vcpu_vlapic(v), msr_content);
>>          break;
>>  
>> -    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0xff:
>> +    case MSR_IA32_APICBASE_MSR ... MSR_IA32_APICBASE_MSR + 0x3f:
>>          if ( hvm_x2apic_msr_write(v, msr, msr_content) )
>> +    case MSR_IA32_APICBASE_MSR + 0x40 ... MSR_IA32_APICBASE_MSR + 0x3ff:
>>              goto gp_fault;
>>          break;
>>  
> I don't think this is done properly here. The ranges here should simply
> be extended back to cover the whole 1k range. Anything more specific
> should be contained to vlapic.c. And this "more specific" should likely
> include #GP on accesses to register indices 0, 1, 4..7, 9, 0xc, 0xe,
> 0x29..0x2e, and 0x3a..0x3d. I.e. extend the recent tightening,
> perhaps by fully switching to a white list in hvm_x2apic_msr_read()
> instead of the current black list approach, as otherwise what Matt
> said would still be wrong for all the listed registers.

Ok - I will see what I can do.

~Andrew