Re: [PATCH 2/2][4.17] x86emul: pull permission check ahead for REP INS/OUTS

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 06/10/2022 14:11, Jan Beulich wrote:
> Based on observations on a fair range of hardware from both primary
> vendors even zero-iteration-count instances of these insns perform the
> port related permission checking first.
>
> Fixes: fe300600464c ("x86: Fix emulation of REP prefix")
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Partly RFC for this not being documented anywhere; inquiry pending.

Intel do actually document this in two roundabout ways.

1) The order of checks in the pseudocode.  Multiple times in the past,
Intel have said that the order of checks in pseudocode is authoritative.

2) This paragraph I've just found at the end of the INS description.

"These instructions may read from the I/O port without writing to the
memory location if an exception or VM exit occurs due to the write (e.g.
#PF). If this would be problematic, for example because the I/O port
read has side-effects, software should ensure the write to the memory
location does not cause an exception or VM exit."

This makes it clear that the IO port is read before the memory operand
is interpreted.  (As a tangent, while the SDM statement is all true,
it's entirely useless advice for e.g. a migrating VM.)


Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, preferably with
some of ^ discussed in the commit message.

>
> The referenced commit is still not really the one, but before it REP
> handling was so broken that I didn't want to go hunt further.
>
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -4248,14 +4248,15 @@ x86_emulate(
>          goto imul;
>  
>      case 0x6c ... 0x6d: /* ins %dx,%es:%edi */ {
> -        unsigned long nr_reps = get_rep_prefix(false, false);
> +        unsigned long nr_reps;
>          unsigned int port = _regs.dx;
>  
>          dst.bytes = !(b & 1) ? 1 : (op_bytes == 8) ? 4 : op_bytes;
> -        dst.mem.seg = x86_seg_es;
> -        dst.mem.off = truncate_ea_and_reps(_regs.r(di), nr_reps, dst.bytes);
>          if ( (rc = ioport_access_check(port, dst.bytes, ctxt, ops)) != 0 )
>              goto done;
> +        nr_reps = get_rep_prefix(false, false);
> +        dst.mem.off = truncate_ea_and_reps(_regs.r(di), nr_reps, dst.bytes);
> +        dst.mem.seg = x86_seg_es;

As a further observation, both the Intel and AMD manuals elude to the
use of unsegmented memory space for the 64bit forms of these.

However, as both %ds (outs) and %es (ins) ignore their bases in 64bit
mode, I can't think of any practical consequences of conditionally not
using x86_seg_none here.

~Andrew