[PATCH 3/3] ntoskrnl: Facilitate kernel object field offset fetchers.

[Derek Lesho]

On 6/19/20 1:43 PM, Zebediah Figura wrote:

> This subject line seems very confusingly worded. I'd suggest trying to
> describe what the patch does instead of why, e.g. 'force the "info"
> field of "_EPROCESS" to have an offset of at least 256.'


  👌

>
> On 6/19/20 12:35 PM, Derek Lesho wrote:
>> EasyAntiCheat.sys reads IoThreadToProcess and PsGetThreadProcessId to find out the offset of the
>> KPROCESS and PID fields in the KTHREAD structure.  They rely on the mov instruction using a 32-bit
>> displacement to get the offset, so we have to make sure the fields are deep enough into the structure.
>>
>> Signed-off-by: Derek Lesho <dlesho at codeweavers.com>
>> ---
>>   dlls/ntoskrnl.exe/ntoskrnl.c         | 1 -
>>   dlls/ntoskrnl.exe/ntoskrnl_private.h | 4 ++++
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
>> index 818ff56d25..51603ec3d7 100644
>> --- a/dlls/ntoskrnl.exe/ntoskrnl.c
>> +++ b/dlls/ntoskrnl.exe/ntoskrnl.c
>> @@ -2394,7 +2394,6 @@ HANDLE WINAPI PsGetThreadId(PETHREAD thread)
>>    */
>>   HANDLE WINAPI PsGetThreadProcessId( PETHREAD thread )
>>   {
>> -    TRACE( "%p -> %p\n", thread, thread->kthread.id.UniqueProcess );
> Why remove this trace?
Because EasyAntiCheat reads the first instruction of the function, if we 
used assembly to re-implement this function, we could avoid this.
>
>>       return thread->kthread.id.UniqueProcess;
> While this may reliably work in practice, there's no guarantee of it. It
> may be a better idea to reimplement the functions in assembly for the
> architectures that need it.
Agreed, I'll just do that instead.
>
>>   }
>>   
>> diff --git a/dlls/ntoskrnl.exe/ntoskrnl_private.h b/dlls/ntoskrnl.exe/ntoskrnl_private.h
>> index a1e1b892e8..9d56b236a5 100644
>> --- a/dlls/ntoskrnl.exe/ntoskrnl_private.h
>> +++ b/dlls/ntoskrnl.exe/ntoskrnl_private.h
>> @@ -39,6 +39,8 @@ struct _OBJECT_TYPE
>>   struct _EPROCESS
>>   {
>>       DISPATCHER_HEADER header;
>> +    /* padding to require a 32-bit displacement */
> I don't think this comment is nearly specific enough. "32-bit
> displacement" is meaningless unless you mention the architecture,
> instruction, and where that instruction is used. Essentially, everything
> that's in the patch summary should probably be here instead.
Makes sense, what do you think about writing the x86_64 without an 
assembler in order to ensure a 32-bit displacement value that is below 
0x100?  We could remove the padding this way.
>
>> +    CHAR padding[0x100 - sizeof(DISPATCHER_HEADER)];
> Presumably this doesn't have to be at offset exactly 0x100; i.e. the "-
> sizeof(DISPATCHER_HEADER)" is unnecessary.
It was just to save space I guess.
>
>>       PROCESS_BASIC_INFORMATION info;
>>       BOOL wow64;
>>   };
>> @@ -46,6 +48,8 @@ struct _EPROCESS
>>   struct _KTHREAD
>>   {
>>       DISPATCHER_HEADER header;
>> +    /* padding to require a 32-bit displacement */
>> +    CHAR padding[0x100 - sizeof(DISPATCHER_HEADER)];
> See above.
>
>>       PEPROCESS process;
>>       CLIENT_ID id;
>>       unsigned int critical_region;
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200619/98ed4c8f/attachment-0001.htm>