From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [RFC PATCH v2 3/3] x86 emulator: Add segment limit checks and
 helper functions
Date: Sun, 11 Jul 2010 21:07:13 +0300
Message-ID: <4C3A0851.1030109@redhat.com>
References: <1278598236-12103-1-git-send-email-m.gamal005@gmail.com> <1278598236-12103-4-git-send-email-m.gamal005@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: mtosatti@redhat.com, kvm@vger.kernel.org
To: Mohammed Gamal <m.gamal005@gmail.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:11025 "EHLO mx1.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753983Ab0GKSHT (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 11 Jul 2010 14:07:19 -0400
In-Reply-To: <1278598236-12103-4-git-send-email-m.gamal005@gmail.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 07/08/2010 05:10 PM, Mohammed Gamal wrote:
> This patch adds segment limit checks to the x86 emulator, in addition to some
> helper functions and changes to the return values of emulate_push to accomodate
> the new checks.
>
>
>
> +static u32 seg_limit(struct x86_emulate_ctxt *ctxt,
> +		     struct x86_emulate_ops *ops, int seg)
> +{
> +	if (ctxt->mode == X86EMUL_MODE_PROT64)
> +		return -1;
>    

That doesn't work well for 64 bit.  It returns 4G-1, you want 2^64-1.  
The return type needs to be u64 and the value -1ULL.


> +
> +	return ops->get_cached_segment_limit(seg, ctxt->vcpu);
> +}
> +
>   static unsigned long seg_override_base(struct x86_emulate_ctxt *ctxt,
>   				       struct x86_emulate_ops *ops,
>   				       struct decode_cache *c)
>
>   static void emulate_pf(struct x86_emulate_ctxt *ctxt, unsigned long addr,
>   		       int err)
>   {
> @@ -719,6 +761,12 @@ static int do_insn_fetch(struct x86_emulate_ctxt *ctxt,
>   {
>   	int rc;
>
> +	/* eip is already relative to CS, so we just check it against the limit */
> +	if (eip>  cs_limit(ctxt, ops) - size - 1) {
>    

What if

    eip = 1
    limit = 3
    size = 8?

> +		emulate_gp(ctxt, 0);
> +		return X86EMUL_PROPAGATE_FAULT;
> +	}
> +
>   	/* x86 instructions are limited to 15 bytes. */
>   	if (eip + size - ctxt->eip>  15)
>   		return X86EMUL_UNHANDLEABLE;
> @@ -1222,6 +1270,11 @@ done_prefixes:
>   		c->src.ptr = (unsigned long *)
>   			register_address(c,  seg_override_base(ctxt, ops, c),
>   					 c->regs[VCPU_REGS_RSI]);
> +		if ((unsigned long)c->src.ptr - seg_override_base(ctxt, ops, c)>
> +			seg_override_limit(ctxt, ops, c) - c->src.bytes - 1) {
> +			emulate_gp(ctxt, 0);
> +			return X86EMUL_PROPAGATE_FAULT;
> +		}
>    

Similar issue.

This code is repeated two often. Best to have a helper for reading and 
writing that accepts the segment ID, which does the limit check (don't 
forget expand-down segments), then the read/write/fetch or #GP/#SS.

That helper can also add the base, so it reduces code elsewhere.  I 
suggest a first patch that introduces the read/write/fetch helpers, and 
a second patch that adds the limit checks.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.