[PATCH v3 0/1] MR721: ntoskrnl: Implement cmp instruction with opcode 0x83
Required for vgk.sys (vanguard anti-cheat) -- v3: parent 7546b4a63d437c2f7f8673cae9341d358f84f1a5 https://gitlab.winehq.org/wine/wine/-/merge_requests/721
From: Etaash Mathamsetty <etaash.mathamsetty(a)gmail.com> implement cmp instruction fix minor styling fix minor styling --- dlls/ntoskrnl.exe/instr.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 8f1aa4d45a3..cb3cce521c9 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -890,7 +890,29 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) } break; /* Unable to emulate it */ } + case 0x83: /* cmp r/m imm8 */ + { + BYTE *data = INSTR_GetOperandAddr(context, instr + 1, prefixlen + 1, long_addr, rex, segprefix, &len); + SIZE_T offset = data - user_shared_data; + SIZE_T data_size = get_op_size(long_op, rex); + if(offset <= KSHARED_USER_DATA_PAGE_SIZE - data_size) + { + TRACE("USD offset %#x at %p\n", (unsigned int)offset, (void*)context->Rip); + + /* Clear ZF and CF */ + context->EFlags &= ~(1UL << 6); + context->EFlags &= ~(1UL); + + if(*(wine_user_shared_data + offset) == instr[2]) + context->EFlags |= (1UL << 6); /* ZF */ + else if(*(wine_user_shared_data + offset) < instr[2]) + context->EFlags |= (1UL); /* CF */ + context->Rip += prefixlen + len + 2; + return ExceptionContinueExecution; + } + break; + } case 0xa0: /* mov Ob, AL */ case 0xa1: /* mov Ovqp, rAX */ { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/721
Hello Etaash; thanks for the patch! I have some comments inlined. On 9/2/22 14:26, Etaash Mathamsetty wrote:
From: Etaash Mathamsetty <etaash.mathamsetty(a)gmail.com>
implement cmp instruction
fix minor styling
fix minor styling
Please fix the subject and patch description; we'd like all patches to be self-contained changes with coherent subjects.
--- dlls/ntoskrnl.exe/instr.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 8f1aa4d45a3..cb3cce521c9 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -890,7 +890,29 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) } break; /* Unable to emulate it */ } + case 0x83: /* cmp r/m imm8 */
This also needs to check the ModRM byte; this instruction multiplexes and CMP is only correct if the extension is 7.
+ { + BYTE *data = INSTR_GetOperandAddr(context, instr + 1, prefixlen + 1, long_addr, rex, segprefix, &len); + SIZE_T offset = data - user_shared_data; + SIZE_T data_size = get_op_size(long_op, rex); + if(offset <= KSHARED_USER_DATA_PAGE_SIZE - data_size) + { + TRACE("USD offset %#x at %p\n", (unsigned int)offset, (void*)context->Rip);
These are copy-pasted from other lines in the same file, but with the style changed. Why?
+ + /* Clear ZF and CF */ + context->EFlags &= ~(1UL << 6); + context->EFlags &= ~(1UL);
I'd recommend adding symbolic #defines for these.
+ + if(*(wine_user_shared_data + offset) == instr[2])
This is only comparing a single byte. The instruction should compare "data_size" bytes.
+ context->EFlags |= (1UL << 6); /* ZF */ + else if(*(wine_user_shared_data + offset) < instr[2]) + context->EFlags |= (1UL); /* CF */
This should also set OF, PF, AF, SF. I'd recommend making this all into a helper, since it'll need to be used for other CMP or SUB instructions. (Note also that the ZF, SF, and PF only depend on the result and IIRC can be shared across all arithmetic instructions, so those may deserve a separate helper.)
On Sat Sep 3 04:21:44 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` Hello Etaash; thanks for the patch! I have some comments inlined. On 9/2/22 14:26, Etaash Mathamsetty wrote:
From: Etaash Mathamsetty <etaash.mathamsetty(a)gmail.com>
implement cmp instruction
fix minor styling
fix minor styling Please fix the subject and patch description; we'd like all patches to be self-contained changes with coherent subjects. --- dlls/ntoskrnl.exe/instr.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/dlls/ntoskrnl.exe/instr.c b/dlls/ntoskrnl.exe/instr.c index 8f1aa4d45a3..cb3cce521c9 100644 --- a/dlls/ntoskrnl.exe/instr.c +++ b/dlls/ntoskrnl.exe/instr.c @@ -890,7 +890,29 @@ static DWORD emulate_instruction( EXCEPTION_RECORD *rec, CONTEXT *context ) } break; /* Unable to emulate it */ } + case 0x83: /* cmp r/m imm8 */ This also needs to check the ModRM byte; this instruction multiplexes and CMP is only correct if the extension is 7. + { + BYTE *data = INSTR_GetOperandAddr(context, instr + 1, prefixlen + 1, long_addr, rex, segprefix, &len); + SIZE_T offset = data - user_shared_data; + SIZE_T data_size = get_op_size(long_op, rex); + if(offset <= KSHARED_USER_DATA_PAGE_SIZE - data_size) + { + TRACE("USD offset %#x at %p\n", (unsigned int)offset, (void*)context->Rip); These are copy-pasted from other lines in the same file, but with the style changed. Why? + + /* Clear ZF and CF */ + context->EFlags &= ~(1UL << 6); + context->EFlags &= ~(1UL); I'd recommend adding symbolic #defines for these. + + if(*(wine_user_shared_data + offset) == instr[2]) This is only comparing a single byte. The instruction should compare "data_size" bytes. + context->EFlags |= (1UL << 6); /* ZF */ + else if(*(wine_user_shared_data + offset) < instr[2]) + context->EFlags |= (1UL); /* CF */ This should also set OF, PF, AF, SF. I'd recommend making this all into a helper, since it'll need to be used for other CMP or SUB instructions. (Note also that the ZF, SF, and PF only depend on the result and IIRC can be shared across all arithmetic instructions, so those may deserve a separate helper.)
This instruction only needs to compare 1 byte, since it's imm8 is just an 8 bit number (unless I am wrong) In my other patch that implements 0x38 and 0x39, I compare with data_size bytes I'll fix the rest of the stuff soon
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/721#note_7601
On 9/3/22 12:22, Etaash Mathamsetty (@etaash.mathamsetty) wrote:
This instruction only needs to compare 1 byte, since it's imm8 is just an 8 bit number (unless I am wrong)
The 0x83 family is an arithmetic operation between a sign-extended immediate byte and a 2/4/8-byte register or memory location. The equivalent 1-byte comparison is 0x80 or 0x82 (they are duplicates).
On Sat Sep 3 17:43:11 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 9/3/22 12:22, Etaash Mathamsetty (@etaash.mathamsetty) wrote:
This instruction only needs to compare 1 byte, since it's imm8 is just an 8 bit number (unless I am wrong) The 0x83 family is an arithmetic operation between a sign-extended immediate byte and a 2/4/8-byte register or memory location. The equivalent 1-byte comparison is 0x80 or 0x82 (they are duplicates).
the comparison is between a signed byte and a 2/4/8 byte register/memory location, how am I supposed to compare more than 1 byte?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/721#note_7604
On 9/3/22 12:48, Etaash Mathamsetty (@etaash.mathamsetty) wrote:
On Sat Sep 3 17:43:11 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 9/3/22 12:22, Etaash Mathamsetty (@etaash.mathamsetty) wrote:
This instruction only needs to compare 1 byte, since it's imm8 is just an 8 bit number (unless I am wrong) The 0x83 family is an arithmetic operation between a sign-extended immediate byte and a 2/4/8-byte register or memory location. The equivalent 1-byte comparison is 0x80 or 0x82 (they are duplicates).
the comparison is between a signed byte and a 2/4/8 byte register/memory location, how am I supposed to compare more than 1 byte?
As stated, the immediate byte should be sign-extended to the appropriate length.
participants (3)
-
Etaash Mathamsetty -
Etaash Mathamsetty (＠etaash.mathamsetty)
-
Zebediah Figura