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

[Sebastian Lackner]

On 26.02.2016 21:22, Aric Stewart wrote:
> Signed-off-by: Aric Stewart
>
> 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 );
>
> }