From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] x86/hvm: Further restrict access to x2apic
	MSRs
Date: Fri, 17 Oct 2014 11:34:57 +0100
Message-ID: <5440F0D1.70905@citrix.com>
References: <1413482672-16865-1-git-send-email-andrew.cooper3@citrix.com>
	<544104AB020000780003F845@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <544104AB020000780003F845@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: Kevin Tian <kevin.tian@intel.com>, Feng Wu <feng.wu@intel.com>, Matt Wilson <msw@linux.com>, Eddie Dong <eddie.dong@intel.com>, Donald Dugger <donald.d.dugger@intel.com>, Xen-devel <xen-devel@lists.xen.org>, Jun Nakajima <jun.nakajima@intel.com>, Yang Zhang <yang.z.zhang@intel.com>, Keir Fraser <keir@xen.org>
List-Id: xen-devel@lists.xenproject.org

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