[PATCH 1/2] ntoskrnl.exe: Track drivers created with IoCreateDriver

[Sebastian Lackner]

On 29.02.2016 04:38, Aric Stewart wrote:
> Thanks for the review!
> 
> 
> On 2/28/16 9:30 PM, Sebastian Lackner wrote:
>> On 26.02.2016 21:22, Aric Stewart wrote:
>>> Signed-off-by: Aric Stewart <aric at codeweavers.com>
>>> ---
>>>   dlls/ntoskrnl.exe/ntoskrnl.c | 64 +++++++++++++++++++++++++++++++++-----------
>>>   1 file changed, 48 insertions(+), 16 deletions(-)
>>>
>>>
>>>
>>> 0002-ntoskrnl.exe-Track-drivers-created-with-IoCreateDriver.txt
>>>
>>>
>>> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
>>> index 3bee2bf..49c8cab 100644
>>> --- a/dlls/ntoskrnl.exe/ntoskrnl.c
>>> +++ b/dlls/ntoskrnl.exe/ntoskrnl.c
>>> @@ -72,6 +72,16 @@ static DWORD request_thread;
>>>   static DWORD client_tid;
>>>   static DWORD client_pid;
>>>   
>>> +typedef struct _created_driver {
>>> +    struct list entry;
>>> +
>>> +    UNICODE_STRING driver_name;
>>> +    DRIVER_OBJECT *driver_obj;
>>> +    DRIVER_EXTENSION *driver_extension;
>>
>> Isn't it possible to add the DRIVER_OBJECT and DRIVER_EXTENSION struct directly
>> (without pointer), to avoid multiple memory allocations?
>>
> 
> Yeah, That would be just fine, to have it part of the base structure instead of a separate pointer. 
> 
>> I would also suggest to use a more meaningful name than "created_driver", and
>> you also don't really need the typedef. Something like "struct wine_driver"
>> would make more sense for example, but probably you have a better idea. ;)
>>
>>> +} created_driver;
>>> +
>>> +struct list created_drivers = LIST_INIT(created_drivers);
>>> +
>>>   #ifdef __i386__
>>>   #define DEFINE_FASTCALL1_ENTRYPOINT( name ) \
>>>       __ASM_STDCALL_FUNC( name, 4, \
>>> @@ -818,36 +828,47 @@ PIRP WINAPI IoBuildSynchronousFsdRequest(ULONG majorfunc, PDEVICE_OBJECT device,
>>>    */
>>>   NTSTATUS WINAPI IoCreateDriver( UNICODE_STRING *name, PDRIVER_INITIALIZE init )
>>>   {
>>> -    DRIVER_OBJECT *driver;
>>> -    DRIVER_EXTENSION *extension;
>>> +    created_driver *driver;
>>>       NTSTATUS status;
>>>   
>>>       TRACE("(%s, %p)\n", debugstr_us(name), init);
>>>   
>>>       if (!(driver = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY,
>>> -                                    sizeof(*driver) + sizeof(*extension) )))
>>> +                                    sizeof(*driver) )))
>>> +        return STATUS_NO_MEMORY;
>>> +
>>> +    if (!(driver->driver_obj = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY,
>>> +                                    sizeof(*driver->driver_obj) + sizeof(*driver->driver_extension) )))
>>> +    {
>>> +        RtlFreeHeap( GetProcessHeap(), 0, driver );
>>>           return STATUS_NO_MEMORY;
>>> +    }
>>>   
>>> -    if ((status = RtlDuplicateUnicodeString( 1, name, &driver->DriverName )))
>>> +    if ((status = RtlDuplicateUnicodeString( 1, name, &driver->driver_name )))
>>>       {
>>> +        RtlFreeHeap( GetProcessHeap(), 0, driver->driver_obj );
>>>           RtlFreeHeap( GetProcessHeap(), 0, driver );
>>>           return status;
>>>       }
>>>   
>>> -    extension = (DRIVER_EXTENSION *)(driver + 1);
>>> -    driver->Size            = sizeof(*driver);
>>> -    driver->DriverInit      = init;
>>> -    driver->DriverExtension = extension;
>>> -    extension->DriverObject   = driver;
>>> -    extension->ServiceKeyName = driver->DriverName;
>>> +    driver->driver_extension = (DRIVER_EXTENSION *)((BYTE*)driver->driver_obj + sizeof(*driver->driver_obj));
>>> +    driver->driver_obj->DriverName      = driver->driver_name;
>>> +    driver->driver_obj->Size            = sizeof(*driver->driver_obj);
>>> +    driver->driver_obj->DriverInit      = init;
>>> +    driver->driver_obj->DriverExtension = driver->driver_extension;
>>> +    driver->driver_extension->DriverObject   = driver->driver_obj;
>>> +    driver->driver_extension->ServiceKeyName = driver->driver_name;
>>>   
>>> -    status = driver->DriverInit( driver, name );
>>> +    status = driver->driver_obj->DriverInit( driver->driver_obj, name );
>>>   
>>>       if (status)
>>>       {
>>> -        RtlFreeUnicodeString( &driver->DriverName );
>>> +        RtlFreeUnicodeString( &driver->driver_name );
>>> +        RtlFreeHeap( GetProcessHeap(), 0, driver->driver_obj );
>>>           RtlFreeHeap( GetProcessHeap(), 0, driver );
>>>       }
>>> +
>>> +    list_add_tail( &created_drivers, &driver->entry );
>>
>> Such a global list will need locking.
>>
> 
> You think that a basic critical section would be sufficient or do you think other locking would be better?

A basic critical section should be fine.

> 
> 
> -aric
> 
>>>       return status;
>>>   }
>>>   
>>> @@ -855,12 +876,23 @@ NTSTATUS WINAPI IoCreateDriver( UNICODE_STRING *name, PDRIVER_INITIALIZE init )
>>>   /***********************************************************************
>>>    *           IoDeleteDriver   (NTOSKRNL.EXE.@)
>>>    */
>>> -void WINAPI IoDeleteDriver( DRIVER_OBJECT *driver )
>>> +void WINAPI IoDeleteDriver( DRIVER_OBJECT *driver_object )
>>>   {
>>> -    TRACE("(%p)\n", driver);
>>> +    created_driver *driver, *next;
>>> +
>>> +    TRACE("(%p)\n", driver_object);
>>> +
>>
>> By putting the driver_object in the same struct, you could use CONTAINING_RECORD here.
>> Also, a lock will be required while unlinking the driver from the list.
>>
>>> +    LIST_FOR_EACH_ENTRY_SAFE(driver, next, &created_drivers, created_driver, entry)
>>> +    {
>>> +        if (driver->driver_obj == driver_object)
>>> +        {
>>> +            list_remove( &driver->entry );
>>> +            RtlFreeUnicodeString( &driver->driver_name);
>>> +            break;
>>> +        }
>>> +    }
>>>   
>>> -    RtlFreeUnicodeString( &driver->DriverName );
>>> -    RtlFreeHeap( GetProcessHeap(), 0, driver );
>>> +    RtlFreeHeap( GetProcessHeap(), 0, driver_object );
>>>   }
>>>   
>>>   
>>>
>>>
>>>
>>