Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/internal/obj/x86: assembler accepts memory operands for DR/CR MOVs #24981

Closed
quasilyte opened this issue Apr 20, 2018 · 5 comments
Closed

Comments

@quasilyte
Copy link
Contributor

quasilyte commented Apr 20, 2018

Originated from CL107075.

Problem: MOV instructions for CR/DR registers accept memory operands in addition to registers.
Both AMD and Intel manuals don't specify memory args as valid.
This can lead to unexpected results for the programmer.

Solution: fix Yml operand class to Yrl. Not totally backwards-compatible.

Details below.


MOV DebugReg instructions describe non-DR argument as reg/mem (Yml) instead of just reg (Yrl):

{AMOVL, Ydr0, Ynone, Yml, 3, [4]uint8{0x0f, 0x21, 0, 0}},
{AMOVL, Ydr6, Ynone, Yml, 3, [4]uint8{0x0f, 0x21, 6, 0}},
{AMOVL, Ydr7, Ynone, Yml, 3, [4]uint8{0x0f, 0x21, 7, 0}},
{AMOVQ, Ydr0, Ynone, Yml, 3, [4]uint8{0x0f, 0x21, 0, 0}},
{AMOVQ, Ydr2, Ynone, Yrl, 3, [4]uint8{0x0f, 0x21, 2, 0}},
{AMOVQ, Ydr3, Ynone, Yrl, 3, [4]uint8{0x0f, 0x21, 3, 0}},
{AMOVQ, Ydr6, Ynone, Yml, 3, [4]uint8{0x0f, 0x21, 6, 0}},
{AMOVQ, Ydr7, Ynone, Yml, 3, [4]uint8{0x0f, 0x21, 7, 0}},
{AMOVL, Yml, Ynone, Ydr0, 4, [4]uint8{0x0f, 0x23, 0, 0}},
{AMOVL, Yml, Ynone, Ydr6, 4, [4]uint8{0x0f, 0x23, 6, 0}},
{AMOVL, Yml, Ynone, Ydr7, 4, [4]uint8{0x0f, 0x23, 7, 0}},
{AMOVQ, Yml, Ynone, Ydr0, 4, [4]uint8{0x0f, 0x23, 0, 0}},
{AMOVQ, Yml, Ynone, Ydr2, 4, [4]uint8{0x0f, 0x23, 2, 0}},
{AMOVQ, Yml, Ynone, Ydr3, 4, [4]uint8{0x0f, 0x23, 3, 0}},
{AMOVQ, Yml, Ynone, Ydr6, 4, [4]uint8{0x0f, 0x23, 6, 0}},
{AMOVQ, Yml, Ynone, Ydr7, 4, [4]uint8{0x0f, 0x23, 7, 0}},

Note that two entries have expected Yrl, they were added later:

{AMOVQ, Ydr2, Ynone, Yrl, 3, [4]uint8{0x0f, 0x21, 2, 0}},
{AMOVQ, Ydr3, Ynone, Yrl, 3, [4]uint8{0x0f, 0x21, 3, 0}},

So, all lines below are assembled:

MOVL DR0, AX   // 0f21c0 MOD=11
MOVL DR0, (AX) // 0f2100 MOD=00

MOVL DR0, 777(AX) // 0f218009030000 MOD=10

MOV_DR is special because it's ModR/M.MOD bits are all aliased to 0b11 (reg).
In theory, they should execute identically (source, see "COMMENT").
Also, quote from AMD manual for MOV DR:

The MOV(DR) instruction is always treated as a register-to-regi ster (MOD = 11) instruction, regardless of the encoding of the MOD field in the MODR/M byte

This makes it debatable whether we should forbid memory arguments or not, but it's almost certainly an error to use memory operand with displacement (SIB byte is error, too).

Here are external disassemblers output for code above:

0f21c0 
0f2100
0f218009030000

0:  0f 21 c0                mov    rax,db0
3:  0f 21 00                mov    rax,db0
6:  0f 21 80                mov    rax,db0
9:  09 03                   or     DWORD PTR [rbx],eax

XED accepts all MOD bits combinations, but not displacement immediate itself.

./xed -64 -d '0f2180'
0F2180
ICLASS: MOV_DR   CATEGORY: DATAXFER   EXTENSION: BASE  IFORM: MOV_DR_GPR64_DR   ISA_SET: I86
SHORT: mov rax, dr0

./xed -64 -d '0f218009030000'
0F218009030000
ICLASS: MOV_DR   CATEGORY: DATAXFER   EXTENSION: BASE  IFORM: MOV_DR_GPR64_DR   ISA_SET: I86
SHORT: mov rax, dr0
09030000
ICLASS: OR   CATEGORY: LOGICAL   EXTENSION: BASE  IFORM: OR_MEMv_GPRv   ISA_SET: I86
SHORT: or dword ptr [rbx], eax
0000
ICLASS: ADD   CATEGORY: BINARY   EXTENSION: BASE  IFORM: ADD_MEMb_GPR8   ISA_SET: I86
SHORT: add byte ptr [rax], al

If we change Yml to Yrl, weird behavior can be mitigated, but this is not backwards-compatible.
Not sure if anyone depends on this.
Maybe such code can be considered as broken anyway.

Everything above also applies to CR moves.

@quasilyte
Copy link
Contributor Author

CC @cherrymui

This also affects CR moves (at least I believe so).

@quasilyte
Copy link
Contributor Author

CC @TocarIP, in case you're interested in this.

@quasilyte
Copy link
Contributor Author

quasilyte commented Apr 20, 2018

Current state is erroneous with almost 100% confidence.
What we need is decision whether we fix it or mark with comment and live with it.

@cherrymui
Copy link
Member

I think we should fix it, rejecting the invalid cases. MOVL DR0, (AX) looks like a store, but the machine instruction it generates is not. MOVL DR0, 777(AX) is even worse.

@gopherbot
Copy link

Change https://golang.org/cl/107075 mentions this issue: cmd/internal/obj/x86: forbid mem args for MOV_DR and MOV_CR

@golang golang locked and limited conversation to collaborators Apr 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants