Re: [PATCH 1/2] ntoskrnl.exe: Track drivers created with IoCreateDriver
On 26.02.2016 21:22, Aric Stewart wrote:
Signed-off-by: Aric Stewart <aric(a)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? 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.
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 ); }
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(a)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 ); }
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(a)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 ); }
participants (2)
-
Aric Stewart -
Sebastian Lackner