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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 16.10.14 at 20:04, <andrew.cooper3@xxxxxxxxxx> 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.

> --- 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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel