Re: [PATCH v3 ] programs/winedevice.exe: Use IoCreateDriver and IoDeleteDriver

[Sebastian Lackner]

On 29.07.2016 21:02, Aric Stewart wrote:
> v2: Pass a proper driver name to IoCreateDriver and use generated keyname
> v3: Remove global driver_name
>
> The two patches are not longer a set as each is independent of the other
>
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> ---
>  programs/winedevice/device.c | 179 +++++++++++++++++++++++--------------------
>  1 file changed, 94 insertions(+), 85 deletions(-)
>
> 0002-programs-winedevice.exe-Use-IoCreateDriver-and-IoDelet.txt
>
> diff --git a/programs/winedevice/device.c b/programs/winedevice/device.c
> index 94132ed..df0fac3 100644
> --- a/programs/winedevice/device.c
> +++ b/programs/winedevice/device.c
> @@ -40,11 +40,10 @@ WINE_DECLARE_DEBUG_CHANNEL(relay);
>
> extern NTSTATUS CDECL wine_ntoskrnl_main_loop( HANDLE stop_event );
>
> -static WCHAR *driver_name;
>  static SERVICE_STATUS_HANDLE service_handle;
>  static HANDLE stop_event;
> -static DRIVER_OBJECT driver_obj;
> -static DRIVER_EXTENSION driver_extension;
> +static DRIVER_OBJECT *driver_obj;
> +static HMODULE driver_module;

How do you plan to adjust this when multiple drivers are loaded? Its still not clear to me, unfortunately.
The static variables have to go away sooner or later. ;)
> /* find the LDR_MODULE corresponding to the driver module */
>  static LDR_MODULE *find_ldr_module( HMODULE module )
> @@ -132,92 +131,25 @@ error:
>      return NULL;
>  }
>
> -/* call the driver init entry point */
> -static NTSTATUS init_driver( HMODULE module, UNICODE_STRING *keyname )
> -{
> -    unsigned int i;
> -    NTSTATUS status;
> -    const IMAGE_NT_HEADERS *nt = RtlImageNtHeader( module );
> -
> -    if (!nt->OptionalHeader.AddressOfEntryPoint) return STATUS_SUCCESS;
> -
> -    driver_obj.Size            = sizeof(driver_obj);
> -    driver_obj.DriverSection   = find_ldr_module( module );
> -    driver_obj.DriverInit      = (PDRIVER_INITIALIZE)((char *)module + nt->OptionalHeader.AddressOfEntryPoint);
> -    driver_obj.DriverExtension = &driver_extension;
> -
> -    driver_extension.DriverObject   = &driver_obj;
> -    driver_extension.ServiceKeyName = *keyname;
> -
> -    if (WINE_TRACE_ON(relay))
> -        WINE_DPRINTF( "%04x:Call driver init %p (obj=%p,str=%s)\n", GetCurrentThreadId(),
> -                      driver_obj.DriverInit, &driver_obj, wine_dbgstr_w(keyname->Buffer) );
> -
> -    status = driver_obj.DriverInit( &driver_obj, keyname );
> -
> -    if (WINE_TRACE_ON(relay))
> -        WINE_DPRINTF( "%04x:Ret  driver init %p (obj=%p,str=%s) retval=%08x\n", GetCurrentThreadId(),
> -                      driver_obj.DriverInit, &driver_obj, wine_dbgstr_w(keyname->Buffer), status );
> -
> -    WINE_TRACE( "init done for %s obj %p\n", wine_dbgstr_w(driver_name), &driver_obj );
> -    WINE_TRACE( "- DriverInit = %p\n", driver_obj.DriverInit );
> -    WINE_TRACE( "- DriverStartIo = %p\n", driver_obj.DriverStartIo );
> -    WINE_TRACE( "- DriverUnload = %p\n", driver_obj.DriverUnload );
> -    for (i = 0; i <= IRP_MJ_MAXIMUM_FUNCTION; i++)
> -        WINE_TRACE( "- MajorFunction[%d] = %p\n", i, driver_obj.MajorFunction[i] );
> -
> -    return status;
> -}
> -
> -/* call the driver unload function */
> -static void unload_driver( HMODULE module, DRIVER_OBJECT *driver_obj )
> -{
> -    if (driver_obj->DriverUnload)
> -    {
> -        if (WINE_TRACE_ON(relay))
> -            WINE_DPRINTF( "%04x:Call driver unload %p (obj=%p)\n", GetCurrentThreadId(),
> -                          driver_obj->DriverUnload, driver_obj );
> -
> -        driver_obj->DriverUnload( driver_obj );
> -
> -        if (WINE_TRACE_ON(relay))
> -            WINE_DPRINTF( "%04x:Ret  driver unload %p (obj=%p)\n", GetCurrentThreadId(),
> -                          driver_obj->DriverUnload, driver_obj );
> -    }
> -    FreeLibrary( module );
> -}
> -
>  /* load the .sys module for a device driver */
> -static HMODULE load_driver(void)
> +static HMODULE load_driver(const WCHAR *driver_name, UNICODE_STRING * keyname)
>  {
>      static const WCHAR driversW[] = {'\\','d','r','i','v','e','r','s','\\',0};
>      static const WCHAR systemrootW[] = {'\\','S','y','s','t','e','m','R','o','o','t','\\',0};
>      static const WCHAR postfixW[] = {'.','s','y','s',0};
>      static const WCHAR ntprefixW[] = {'\\','?','?','\\',0};
>      static const WCHAR ImagePathW[] = {'I','m','a','g','e','P','a','t','h',0};
> -    static const WCHAR servicesW[] = {'\\','R','e','g','i','s','t','r','y',
> -                                      '\\','M','a','c','h','i','n','e',
> -                                      '\\','S','y','s','t','e','m',
> -                                      '\\','C','u','r','r','e','n','t','C','o','n','t','r','o','l','S','e','t',
> -                                      '\\','S','e','r','v','i','c','e','s','\\',0};
>
> -    UNICODE_STRING keypath;
>      HKEY driver_hkey;
>      HMODULE module;
>      LPWSTR path = NULL, str;
>      DWORD type, size;
>
> -    str = HeapAlloc( GetProcessHeap(), 0, sizeof(servicesW) + strlenW(driver_name)*sizeof(WCHAR) );
> -    lstrcpyW( str, servicesW );
> -    lstrcatW( str, driver_name );
> -
> -    if (RegOpenKeyW( HKEY_LOCAL_MACHINE, str + 18 /* skip \registry\machine */, &driver_hkey ))
> +    if (RegOpenKeyW( HKEY_LOCAL_MACHINE, keyname->Buffer + 18 /* skip \registry\machine */, &driver_hkey ))

Currently its hardcoded in ntoskrnl, but it would be better to check the prefix first.
> {
> -        WINE_ERR( "cannot open key %s, err=%u\n", wine_dbgstr_w(str), GetLastError() );
> -        HeapFree( GetProcessHeap(), 0, str);
> +        WINE_ERR( "cannot open key %s, err=%u\n", wine_dbgstr_w(keyname->Buffer), GetLastError() );
>          return NULL;
>      }
> -    RtlInitUnicodeString( &keypath, str );
>
> /* read the executable path from memory */
>      size = 0;
> @@ -233,7 +165,6 @@ static HMODULE load_driver(void)
>          HeapFree( GetProcessHeap(), 0, str );
>          if (!path)
>          {
> -            RtlFreeUnicodeString( &keypath );
>              RegCloseKey( driver_hkey );
>              return NULL;
>          }
> @@ -276,19 +207,98 @@ static HMODULE load_driver(void)
>
> module = load_driver_module( str );
>      HeapFree( GetProcessHeap(), 0, path );
> -    if (!module)
> +    return module;
> +}
> +
> +/* call the driver init entry point */
> +static NTSTATUS WINAPI init_driver( DRIVER_OBJECT *driver_object, UNICODE_STRING *keyname )
> +{
> +    unsigned int i;
> +    NTSTATUS status;
> +    const IMAGE_NT_HEADERS *nt;
> +    const WCHAR *driver_name;
> +
> +    /* Retrieve driver name from the keyname */
> +    driver_name = strrchrW(keyname->Buffer, '\\');

It would be better to test that this actually worked.
> +    driver_name++;
> +
> +    driver_module = load_driver( driver_name, keyname );
> +    if (!driver_module)
>      {
> -        RtlFreeUnicodeString( &keypath );
> -        return NULL;
> +        WINE_ERR("Failed to load device\n");
> +        return STATUS_DLL_INIT_FAILED;
>      }
>
> -    init_driver( module, &keypath );
> -    return module;
> +    nt = RtlImageNtHeader( driver_module );
> +
> +    if (!nt->OptionalHeader.AddressOfEntryPoint) return STATUS_SUCCESS;
> +
> +    driver_obj = driver_object;

driver_obj will remain uninitialized when the module has no entry point.
You will still need it during the unload step.
> +
> +    driver_object->DriverSection   = find_ldr_module( driver_module );
> +    driver_object->DriverInit      = (PDRIVER_INITIALIZE)((char *)driver_module + nt->OptionalHeader.AddressOfEntryPoint);
> +
> +    if (WINE_TRACE_ON(relay))
> +        WINE_DPRINTF( "%04x:Call driver init %p (obj=%p,str=%s)\n", GetCurrentThreadId(),
> +                      driver_object->DriverInit, driver_object, wine_dbgstr_w(keyname->Buffer) );
> +
> +    status = driver_object->DriverInit( driver_object, keyname );
> +
> +    if (WINE_TRACE_ON(relay))
> +        WINE_DPRINTF( "%04x:Ret  driver init %p (obj=%p,str=%s) retval=%08x\n", GetCurrentThreadId(),
> +                      driver_object->DriverInit, driver_object, wine_dbgstr_w(keyname->Buffer), status );
> +
> +    WINE_TRACE( "init done for %s obj %p\n", wine_dbgstr_w(driver_name), driver_object );
> +    WINE_TRACE( "- DriverInit = %p\n", driver_object->DriverInit );
> +    WINE_TRACE( "- DriverStartIo = %p\n", driver_object->DriverStartIo );
> +    WINE_TRACE( "- DriverUnload = %p\n", driver_object->DriverUnload );
> +    for (i = 0; i <= IRP_MJ_MAXIMUM_FUNCTION; i++)
> +        WINE_TRACE( "- MajorFunction[%d] = %p\n", i, driver_object->MajorFunction[i] );
> +
> +    return status;
> +}
> +
> +/* call the driver unload function */
> +static void unload_driver( HMODULE module, DRIVER_OBJECT *driver_obj )
> +{
> +    if (driver_obj->DriverUnload)
> +    {
> +        if (WINE_TRACE_ON(relay))
> +            WINE_DPRINTF( "%04x:Call driver unload %p (obj=%p)\n", GetCurrentThreadId(),
> +                          driver_obj->DriverUnload, driver_obj );
> +
> +        driver_obj->DriverUnload( driver_obj );
> +
> +        if (WINE_TRACE_ON(relay))
> +            WINE_DPRINTF( "%04x:Ret  driver unload %p (obj=%p)\n", GetCurrentThreadId(),
> +                          driver_obj->DriverUnload, driver_obj );
> +    }
> +    FreeLibrary( module );
> +    IoDeleteDriver( driver_obj );
> +}
> +
> +static HRESULT create_driver(const WCHAR *driver_name)
> +{

This should be a NTSTATUS return value.
> +    static const WCHAR driverW[] = {'\\','D','r','i','v','e','r','\\',0};
> +    UNICODE_STRING drv_name;
> +    HRESULT status;
> +    WCHAR *str;
> +
> +    str = HeapAlloc( GetProcessHeap(), 0, sizeof(driverW) + strlenW(driver_name)*sizeof(WCHAR) );
> +    lstrcpyW( str, driverW);
> +    lstrcatW( str, driver_name );
> +    RtlInitUnicodeString( &drv_name, str );
> +
> +    status = IoCreateDriver( &drv_name, init_driver );
> +
> +    RtlFreeUnicodeString( &drv_name);
> +    return status;
>  }
>
> static DWORD WINAPI service_handler( DWORD ctrl, DWORD event_type, LPVOID event_data, LPVOID context )
>  {
>      SERVICE_STATUS status;
> +    WCHAR* driver_name = (WCHAR*)context;
>
> status.dwServiceType             = SERVICE_WIN32;
>      status.dwControlsAccepted        = SERVICE_ACCEPT_STOP;
> @@ -318,13 +328,13 @@ static DWORD WINAPI service_handler( DWORD ctrl, DWORD event_type, LPVOID event_
>  static void WINAPI ServiceMain( DWORD argc, LPWSTR *argv )
>  {
>      SERVICE_STATUS status;
> -    HMODULE driver_module;
> +    const WCHAR* driver_name = argv[0];
>
> WINE_TRACE( "starting service %s\n", wine_dbgstr_w(driver_name) );
>
> stop_event = CreateEventW( NULL, TRUE, FALSE, NULL );
>
> -    service_handle = RegisterServiceCtrlHandlerExW( driver_name, service_handler, NULL );
> +    service_handle = RegisterServiceCtrlHandlerExW( driver_name, service_handler, (VOID*)driver_name);

Usually "void *" is preferred in Wine source.
> if (!service_handle)
>          return;
>
> @@ -337,15 +347,14 @@ static void WINAPI ServiceMain( DWORD argc, LPWSTR *argv )
>      status.dwWaitHint                = 10000;
>      SetServiceStatus( service_handle, &status );
>
> -    driver_module = load_driver();
> -    if (driver_module)
> +    if (SUCCEEDED(create_driver(driver_name)))

After changing the HRESULT -> NTSTATUS return value, this also needs to be adjusted.
> {
>          status.dwCurrentState     = SERVICE_RUNNING;
>          status.dwControlsAccepted = SERVICE_ACCEPT_STOP | SERVICE_ACCEPT_SHUTDOWN;
>          SetServiceStatus( service_handle, &status );
>
> wine_ntoskrnl_main_loop( stop_event );
> -        unload_driver( driver_module, &driver_obj );
> +        unload_driver( driver_module, driver_obj );
>      }
>      else WINE_ERR( "driver %s failed to load\n", wine_dbgstr_w(driver_name) );
>
> @@ -359,7 +368,7 @@ int wmain( int argc, WCHAR *argv[] )
>  {
>      SERVICE_TABLE_ENTRYW service_table[2];
>
> -    if (!(driver_name = argv[1]))
> +    if (!(argv[1]))

You can also remove the brackets. ;)
> {
>          WINE_ERR( "missing device name, winedevice isn't supposed to be run manually\n" );
>          return 1;

[PATCH v3 ] programs/winedevice.exe: Use IoCreateDriver and IoDeleteDriver

[Aric Stewart]

On 8/3/16 11:54 AM, Sebastian Lackner wrote:
> On 29.07.2016 21:02, Aric Stewart wrote:
> > v2: Pass a proper driver name to IoCreateDriver and use generated keyname
> > v3: Remove global driver_name
> >
> > The two patches are not longer a set as each is independent of the other
> >
> > Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> > ---
> >  programs/winedevice/device.c | 179 +++++++++++++++++++++++--------------------
> >  1 file changed, 94 insertions(+), 85 deletions(-)
> >
> > 0002-programs-winedevice.exe-Use-IoCreateDriver-and-IoDelet.txt
> >
> > diff --git a/programs/winedevice/device.c b/programs/winedevice/device.c
> > index 94132ed..df0fac3 100644
> > --- a/programs/winedevice/device.c
> > +++ b/programs/winedevice/device.c
> > @@ -40,11 +40,10 @@ WINE_DECLARE_DEBUG_CHANNEL(relay);
> >
> > extern NTSTATUS CDECL wine_ntoskrnl_main_loop( HANDLE stop_event );
> >
> > -static WCHAR *driver_name;
> >  static SERVICE_STATUS_HANDLE service_handle;
> >  static HANDLE stop_event;
> > -static DRIVER_OBJECT driver_obj;
> > -static DRIVER_EXTENSION driver_extension;
> > +static DRIVER_OBJECT *driver_obj;
> > +static HMODULE driver_module;
>
> How do you plan to adjust this when multiple drivers are loaded? Its still not clear to me, unfortunately.
> The static variables have to go away sooner or later. ;)

My plan was to replace this with a list (or maybe rb_tree) of structures containing this information keyed from the driver that is being loaded. 
So there would be a single global list(tree).

-aric

[PATCH v3 ] programs/winedevice.exe: Use IoCreateDriver and IoDeleteDriver

[Aric Stewart]

On 8/3/16 11:54 AM, Sebastian Lackner wrote:
> On 29.07.2016 21:02, Aric Stewart wrote:
> > v2: Pass a proper driver name to IoCreateDriver and use generated keyname
> > v3: Remove global driver_name
> >
> > The two patches are not longer a set as each is independent of the other
> >
> > Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> > ---
> >  programs/winedevice/device.c | 179 +++++++++++++++++++++++--------------------
> >  1 file changed, 94 insertions(+), 85 deletions(-)
> >
> > 0002-programs-winedevice.exe-Use-IoCreateDriver-and-IoDelet.txt
> >
> > diff --git a/programs/winedevice/device.c b/programs/winedevice/device.c
> > index 94132ed..df0fac3 100644
> > --- a/programs/winedevice/device.c
> > +++ b/programs/winedevice/device.c
> > ...
> > -    UNICODE_STRING keypath;
> >      HKEY driver_hkey;
> >      HMODULE module;
> >      LPWSTR path = NULL, str;
> >      DWORD type, size;
> >
> > -    str = HeapAlloc( GetProcessHeap(), 0, sizeof(servicesW) + strlenW(driver_name)*sizeof(WCHAR) );
> > -    lstrcpyW( str, servicesW );
> > -    lstrcatW( str, driver_name );
> > -
> > -    if (RegOpenKeyW( HKEY_LOCAL_MACHINE, str + 18 /* skip \registry\machine */, &driver_hkey ))
> > +    if (RegOpenKeyW( HKEY_LOCAL_MACHINE, keyname->Buffer + 18 /* skip \registry\machine */, &driver_hkey ))
>
> Currently its hardcoded in ntoskrnl, but it would be better to check the prefix first.

The specifications for DriverEntry state that

"The registry path string pointed to by RegistryPath is of the form \Registry\Machine\System\CurrentControlSet\Services\DriverName. A driver can use this path to store driver-specific information; see Registry Keys for Drivers."

So it is reasonable to assume that the keyname will begin with that. I can check but it seems unnecessary.
> > {
> > -        WINE_ERR( "cannot open key %s, err=%u\n", wine_dbgstr_w(str), GetLastError() );
> > -        HeapFree( GetProcessHeap(), 0, str);
> > +        WINE_ERR( "cannot open key %s, err=%u\n", wine_dbgstr_w(keyname->Buffer), GetLastError() );
> >          return NULL;
> >      }
> > -    RtlInitUnicodeString( &keypath, str );
> >
> > /* read the executable path from memory */
> >      size = 0;
> > @@ -233,7 +165,6 @@ static HMODULE load_driver(void)
> >          HeapFree( GetProcessHeap(), 0, str );
> >          if (!path)
> >          {
> > -            RtlFreeUnicodeString( &keypath );
> >              RegCloseKey( driver_hkey );
> >              return NULL;
> >          }
> > @@ -276,19 +207,98 @@ static HMODULE load_driver(void)
> >
> > module = load_driver_module( str );
> >      HeapFree( GetProcessHeap(), 0, path );
> > -    if (!module)
> > +    return module;
> > +}
> > +
> > +/* call the driver init entry point */
> > +static NTSTATUS WINAPI init_driver( DRIVER_OBJECT *driver_object, UNICODE_STRING *keyname )
> > +{
> > +    unsigned int i;
> > +    NTSTATUS status;
> > +    const IMAGE_NT_HEADERS *nt;
> > +    const WCHAR *driver_name;
> > +
> > +    /* Retrieve driver name from the keyname */
> > +    driver_name = strrchrW(keyname->Buffer, '\\');
>
> It would be better to test that this actually worked.

Again see above...  I can test, but assuming does not seem unreasonable.

The rest seem like helpful updates and so I will submit a new version.

-aric