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