Re: [RFC PATCH v2 3/3] x86 emulator: Add segment limit checks and helper functions

[Avi Kivity <avi@redhat.com>]

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.