Re: [Xen-devel] [PATCH 1/3] x86emul: support load forms of {, V}MOV{D, Q}

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/12/16 09:55, Jan Beulich wrote:
> --- a/xen/arch/x86/x86_emulate/x86_emulate.c
> +++ b/xen/arch/x86/x86_emulate/x86_emulate.c
> @@ -4995,6 +4995,12 @@ x86_emulate(
>                                           /* vmovntdq ymm,m256 */
>          fail_if(ea.type != OP_MEM);
>          /* fall through */
> +    case X86EMUL_OPC(0x0f, 0x6e):        /* movd r/m32,mm */
> +                                         /* movq r/m64,mm */
> +    case X86EMUL_OPC_66(0x0f, 0x6e):     /* movd r/m32,xmm */
> +                                         /* movq r/m64,xmm */
> +    case X86EMUL_OPC_VEX_66(0x0f, 0x6e): /* vmovd r/m32,xmm */
> +                                         /* vmovq r/m64,xmm */
>      case X86EMUL_OPC(0x0f, 0x6f):        /* movq mm/m64,mm */
>      case X86EMUL_OPC_66(0x0f, 0x6f):     /* movdqa xmm/m128,xmm */
>      case X86EMUL_OPC_F3(0x0f, 0x6f):     /* movdqu xmm/m128,xmm */
> @@ -5008,6 +5014,8 @@ x86_emulate(
>                                           /* movq xmm,r/m64 */
>      case X86EMUL_OPC_VEX_66(0x0f, 0x7e): /* vmovd xmm,r/m32 */
>                                           /* vmovq xmm,r/m64 */
> +    case X86EMUL_OPC_F3(0x0f, 0x7e):     /* movq xmm/m64,xmm */
> +    case X86EMUL_OPC_VEX_F3(0x0f, 0x7e): /* vmovq xmm/m64,xmm */
>      case X86EMUL_OPC(0x0f, 0x7f):        /* movq mm,mm/m64 */
>      case X86EMUL_OPC_66(0x0f, 0x7f):     /* movdqa xmm,xmm/m128 */
>      case X86EMUL_OPC_VEX_66(0x0f, 0x7f): /* vmovdqa xmm,xmm/m128 */
> @@ -5019,6 +5027,7 @@ x86_emulate(
>      case X86EMUL_OPC_VEX_66(0x0f, 0xd6): /* vmovq xmm,xmm/m64 */
>      {
>          uint8_t *buf = get_stub(stub);
> +        bool load = false;

I am afraid that this logic is already almost-opaque, and these change
make it even more complicated to follow.  Sufficiently so that I can't
review it; I have tried and failed to look at the end result and
conclude whether it is correct or not.

(I have no specific reasons to think that it isn't correct, but I can't
claim to have reviewed it with integrity.)

This block of code in particular has a higher security risk than most in
x86_emulate(), because of the risk of accidentally executing a stub with
a modrm which selects anything other than SIMD register or (%rax) r/m
destination.

Therefore, I'd like to see what we can do to make the logic easier to
follow.


This block deals with 3 kinds of operations, load / move / store,
depending on the whether the source operand is memory, both operands are
registers, or the destination operand is memory.  Beyond that however,
there doesn't appear to be any consistency to the required adjustments
to make the stub safe.

At a start, vex.pfx continues to be a source of confusion as it refers
to legacy prefixes rather than the vex prefix, and vex.opcx being the
entity which refers to legacy/vex/evex/xop encoding.  Renaming these
constants alone would be a help.

I wonder if doing things like this would help?

enum { LOAD, MOVE, STORE } type;

switch ( b )
{
    case ...
        type = LOAD;
        break;
    case ...
        type = MOVE;
        break;
    case ...
        type = STORE;
        break;
}

In particular, having a 128 line case statement (before this patch, 149
after), with most semantics based on b == one of 5 (before, 7 after)
different raw numbers, is too much cognitive context to hold.

~Andrew

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