[PATCH 1/2] ntoskrnl.exe: Track drivers created with IoCreateDriver
Aric Stewart
aric at codeweavers.com
Sun Feb 28 21:38:31 CST 2016
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?
-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 );
>> }
>>
>>
>>
>>
>>
>
More information about the wine-devel
mailing list