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 );
>
> }

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

[Aric Stewart]

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
> >
> > 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 );
> > }

[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
> > >
> > > 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 );
> > > }