Re: [Xen-devel] [PATCH v2 01/13] x86emul/fuzz: add rudimentary limit checking

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 10/06/2017 04:54 PM, Jan Beulich wrote:
>>>> On 06.10.17 at 17:21, <george.dunlap@xxxxxxxxxx> wrote:
>> On Mon, Sep 25, 2017 at 3:26 PM, George Dunlap <george.dunlap@xxxxxxxxxx> 
>> wrote:
>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> fuzz_insn_fetch() is the only data access helper where it is possible
>>> to see offsets larger than 4Gb in 16- or 32-bit modes, as we leave the
>>> incoming rIP untouched in the emulator itself. The check is needed here
>>> as otherwise, after successfully fetching insn bytes, we may end up
>>> zero-extending EIP soon after complete_insn, which collides with the
>>> X86EMUL_EXCEPTION-conditional respective ASSERT() in
>>> x86_emulate_wrapper(). (NB: put_rep_prefix() is what allows
>>> complete_insn to be reached with rc set to other than X86EMUL_OKAY or
>>> X86EMUL_DONE. See also commit 53f87c03b4 ["x86emul: generalize
>>> exception handling for rep_* hooks"].)
>>>
>>> Add assert()-s for all other (data) access routines, as effective
>>> address generation in the emulator ought to guarantee in-range values.
>>> For them to not trigger, several adjustments to the emulator's address
>>> calculations are needed: While for DstBitBase it is really mandatory,
>>> the specification allows for either behavior for two-part accesses.
>>
>> Something seems to be missing here -- what's mandatory for DstBitBase,
>> and what are the two  behaviors allowed by the specification for
>> two-part accesses?
> 
> "it" in the sentence above refers to "adjustments" (for DstBitBase
> it's just one adjustment, so the singular/plural mismatch is not
> really helpful). Similarly "either behavior" refers to the code with
> and without the changes done. Would
> 
> "Add assert()-s for all other (data) access routines, as effective
>  address generation in the emulator ought to guarantee in-range values.
>  For them to not trigger, several adjustments to the emulator's address
>  calculations are needed: While the DstBitBase one is really mandatory,
>  the specification allows for either original or new behavior for two-
>  part accesses. Observed behavior on real hardware, however, is for such
>  accesses to silently wrap at the 2^^32 boundary in other than 64-bit
>  mode, just like they do at the 2^^64 boundary in 64-bit mode, which our
>  code is now being brought in line with. While adding truncate_ea()
>  invocations there, also convert open coded instances of it."
> 
> be any better?

Yes, I think so.

One more thing:

> @@ -1249,10 +1249,10 @@ static void __put_rep_prefix(
>
>  /* Clip maximum repetitions so that the index register at most just
wraps. */
>  #define truncate_ea_and_reps(ea, reps, bytes_per_rep) ({
> -    unsigned long todo__, ea__ = truncate_word(ea, ad_bytes);

> +    unsigned long todo__, ea__ = truncate_ea(ea);

>      if ( !(_regs.eflags & X86_EFLAGS_DF) )
> -        todo__ = truncate_word(-(ea), ad_bytes) / (bytes_per_rep);
> -    else if ( truncate_word((ea) + (bytes_per_rep) - 1, ad_bytes) <
ea__ )\
> +        todo__ = truncate_ea(-ea__) / (bytes_per_rep);
> +    else if ( truncate_ea(ea__ + (bytes_per_rep) - 1) < ea__ )

This changes truncate_ea(-ea) to truncate_ea(-truncate_ea(ea)), and
truncate_ea(ea + bpr-1) to truncate_ea(truncate_ea(ea) + bpr-1).  That
sounds like a plausible change, but it's worth checking to see that it
was intentional.

I'm not at the moment able to evaluate the assertion that "the
specification allows for either original or new behavior for two-part
accesses", nor that there is a 1:1 mapping between what this patch
changes and what needs to be changed.

But assuming the above are true (and the change I asked about was
intentional):

Reviewed-by: George Dunlap <george.dunlap@xxxxxxxxxx>

If that's not good enough to check it in, feel free to give me the
appropriate references to the Intel manual that will allow me to
independently verify those facts, and get rid of my caveat.

 -George

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