Re: [PATCH v4] ntoskrnl.exe: Implement ZwLoadDriver and ZwUnloadDriver

[Sebastian Lackner]

On 22.01.2016 19:15, Aric Stewart wrote:
> Code is mostly moved from winedevice.exe
> winedevice.exe now uses these functions
> v3: No longer use advapi32 Registry functions
> v4: Fix FALSE return code (thanks to Huw Davies for spotting this one)
>     Remove WINE_ from debuging macros
> Signed-off-by: Aric Stewart
>
> dlls/ntoskrnl.exe/ntoskrnl.c        | 306 ++++++++++++++++++++++++++++++++++++
>  dlls/ntoskrnl.exe/ntoskrnl.exe.spec |   4 +-
>  programs/winedevice/device.c        | 243 +---------------------------
>  3 files changed, 313 insertions(+), 240 deletions(-)

I don't think moving the loader code from winedevice to ntoskrnl.exe is a good idea,
at least not without further changes, because:

* We should not initialize the same driver twice, the second initialization could
  fail because of namespace collisions.

* Drivers should be properly uninitialized, no matter how they were loaded.

* In theory, all drivers should be loaded into the same address space. This means,
  winedevice and the wineserver has to keep track of them, and its not sufficient to
  maintain a list in ntoskrnl.exe.

Besides those design issues, I've marked a couple of other issues in the patch below.
> 0001-ntoskrnl.exe-Implement-ZwLoadDriver-and-ZwUnloadDriver.txt
>
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
> index 04f0198..032ab21 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.c
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.c
> @@ -72,6 +72,18 @@ static DWORD request_thread;
>  static DWORD client_tid;
>  static DWORD client_pid;
>
> +/* Loaded driver list */
> +typedef struct _loaded_driver {
> - struct list entry;
> -
> - UNICODE_STRING name;
> - HMODULE module;
> - DRIVER_OBJECT driver_obj;
> - DRIVER_EXTENSION driver_extension;
>
> +} loaded_driver;
> -
>
> +struct list loaded_drivers = LIST_INIT(loaded_drivers);

It wouldn't hurt to add locking for such a global list.
> -
>
> #ifdef __i386__
>  #define DEFINE_FASTCALL1_ENTRYPOINT( name ) \
>      __ASM_STDCALL_FUNC( name, 4, \
> @@ -2407,3 +2419,297 @@ NTSTATUS WINAPI CmUnRegisterCallback(LARGE_INTEGER cookie)
>      FIXME("(%s): stub\n", wine_dbgstr_longlong(cookie.QuadPart));
>      return STATUS_NOT_IMPLEMENTED;
>  }
> -
> -
>
> +/* find the LDR_MODULE corresponding to the driver module */
> +static LDR_MODULE *find_ldr_module( HMODULE module )
> +{
> - LIST_ENTRY *entry, *list = &NtCurrentTeb()->Peb->LdrData->InMemoryOrderModuleList;
> -
> - for (entry = list->Flink; entry != list; entry = entry->Flink)
> - {
> -
>    LDR_MODULE *ldr = CONTAINING_RECORD(entry, LDR_MODULE, InMemoryOrderModuleList);
> -
>    if (ldr->BaseAddress == module) return ldr;
> -
>    if (ldr->BaseAddress > (void *)module) break;
> - }

The loader lock should be acquired while iterating through this list.
(Yes, this is an existing issue, but before it was less critical because
the code was single-threaded.)
> - return NULL;
>
> +}
> -
>
> +/* load the driver module file */
> +static HMODULE load_driver_module( const WCHAR *name )
> +{
> - IMAGE_NT_HEADERS *nt;
> - const IMAGE_IMPORT_DESCRIPTOR *imports;
> - SYSTEM_BASIC_INFORMATION info;
> - int i;
> - INT_PTR delta;
> - ULONG size;
> - HMODULE module = LoadLibraryW( name );
> -
> - if (!module) return NULL;
> - nt = RtlImageNtHeader( module );
> -
> - if (!(delta = (char *)module - (char *)nt->OptionalHeader.ImageBase)) return module;
> -
> - /* the loader does not apply relocations to non page-aligned binaries or executables,
> -
> * we have to do it ourselves */
> -
> - NtQuerySystemInformation( SystemBasicInformation, &info, sizeof(info), NULL );
> - if (nt->OptionalHeader.SectionAlignment < info.PageSize ||
> -
>    !(nt->FileHeader.Characteristics & IMAGE_FILE_DLL))
> - {
> -
>    DWORD old;
> -
>    IMAGE_BASE_RELOCATION *rel, *end;
> -
> -
>    if ((rel = RtlImageDirectoryEntryToData( module, TRUE, IMAGE_DIRECTORY_ENTRY_BASERELOC, &size )))
> -
>    {
> -
>        TRACE( "%s: relocating from %p to %p\n",
> -
>                    wine_dbgstr_w(name), (char *)module - delta, module );
> -
>        end = (IMAGE_BASE_RELOCATION *)((char *)rel + size);
> -
>        while (rel < end && rel->SizeOfBlock)
> -
>        {
> -
>            void *page = (char *)module + rel->VirtualAddress;
> -
>            VirtualProtect( page, info.PageSize, PAGE_EXECUTE_READWRITE, &old );
> -
>            rel = LdrProcessRelocationBlock( page, (rel->SizeOfBlock - sizeof(*rel)) / sizeof(USHORT),
> -
>                                             (USHORT *)(rel + 1), delta );
> -
>            if (old != PAGE_EXECUTE_READWRITE) VirtualProtect( page, info.PageSize, old, &old );
> -
>            if (!rel) goto error;
> -
>        }
> -
>        /* make sure we don't try again */
> -
>        size = FIELD_OFFSET( IMAGE_NT_HEADERS, OptionalHeader ) + nt->FileHeader.SizeOfOptionalHeader;
> -
>        VirtualProtect( nt, size, PAGE_READWRITE, &old );
> -
>        nt->OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_BASERELOC].VirtualAddress = 0;
> -
>        VirtualProtect( nt, size, old, &old );
> -
>    }
> - }
> -
> - /* make sure imports are relocated too */
> -
> - if ((imports = RtlImageDirectoryEntryToData( module, TRUE, IMAGE_DIRECTORY_ENTRY_IMPORT, &size )))
> - {
> -
>    for (i = 0; imports[i].Name && imports[i].FirstThunk; i++)
> -
>    {
> -
>        char *name = (char *)module + imports[i].Name;
> -
>        WCHAR buffer[32], *p = buffer;
> -
> -
>        while (p < buffer + 32) if (!(*p++ = *name++)) break;
> -
>        if (p <= buffer + 32) FreeLibrary( load_driver_module( buffer ) );
> -
>    }
> - }
> -
> - return module;
> -
>
> +error:
> - FreeLibrary( module );
> - return NULL;
>
> +}
> -
>
> +/* call the driver init entry point */
> +static NTSTATUS init_driver( const WCHAR *driver_name, UNICODE_STRING *keyname, loaded_driver *driver )
> +{
> - unsigned int i;
> - NTSTATUS status;
> - const IMAGE_NT_HEADERS *nt = RtlImageNtHeader( driver->module );
> -
> - if (!nt->OptionalHeader.AddressOfEntryPoint) return STATUS_SUCCESS;
> -
> - driver->driver_obj.Size            = sizeof(driver->driver_obj);
> - driver->driver_obj.DriverSection   = find_ldr_module( driver->module );
> - driver->driver_obj.DriverInit      = (PDRIVER_INITIALIZE)((char *)driver->module + nt->OptionalHeader.AddressOfEntryPoint);
> - driver->driver_obj.DriverExtension = &driver->driver_extension;
> -
> - driver->driver_obj.DriverExtension->DriverObject   = &driver->driver_obj;
> - driver->driver_obj.DriverExtension->ServiceKeyName = *keyname;
> -
> - if (TRACE_ON(relay))
> -
>    DPRINTF( "%04x:Call driver init %p (obj=%p,str=%s)\n", GetCurrentThreadId(),
> -
>                  driver->driver_obj.DriverInit, &driver->driver_obj, wine_dbgstr_w(keyname->Buffer) );
> -
> - status = driver->driver_obj.DriverInit( &driver->driver_obj, keyname );
> -
> - if (TRACE_ON(relay))
> -
>    DPRINTF( "%04x:Ret  driver init %p (obj=%p,str=%s) retval=%08x\n", GetCurrentThreadId(),
> -
>                  driver->driver_obj.DriverInit, &driver->driver_obj, wine_dbgstr_w(keyname->Buffer), status );
> -
> - TRACE( "init done for %s obj %p\n", wine_dbgstr_w(driver_name), &driver->driver_obj );
> - TRACE( "- DriverInit = %p\n", driver->driver_obj.DriverInit );
> - TRACE( "- DriverStartIo = %p\n", driver->driver_obj.DriverStartIo );
> - TRACE( "- DriverUnload = %p\n", driver->driver_obj.DriverUnload );
> - for (i = 0; i <= IRP_MJ_MAXIMUM_FUNCTION; i++)
> -
>    TRACE( "- MajorFunction[%d] = %p\n", i, driver->driver_obj.MajorFunction[i] );
> -
> - return status;
>
> +}
> -
>
> +/***********************************************************************
> -
> -
>       ZwLoadDriver (NTOSKRNL.EXE.@)
> - */
>
> +NTSTATUS WINAPI ZwLoadDriver(const UNICODE_STRING *DriverServiceName)

Both the Nt* and Zw* function should do the same, so I would suggest to implement
Nt* instead and forward Zw*, like its done in ntdll.
> +{
> - 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, reg_str;
> - HMODULE module;
> - LPWSTR path = NULL, str;
> - DWORD size;
> - HANDLE driver_hkey;
> - OBJECT_ATTRIBUTES attr;
> - NTSTATUS status;
> - char buffer[256];
> - KEY_VALUE_PARTIAL_INFORMATION *info = (KEY_VALUE_PARTIAL_INFORMATION *)buffer;

The initialization here looks unnecessary.
> - static const int info_size = offsetof( KEY_VALUE_PARTIAL_INFORMATION, Data );
> - loaded_driver *driver;
> -
> - /* Check if driver is already loaded */
> - LIST_FOR_EACH_ENTRY(driver, &loaded_drivers, loaded_driver, entry)
> - {
> -
>    if (RtlEqualUnicodeString(DriverServiceName, &driver->name, FALSE))
> -
>        return ERROR_SUCCESS;
> - }
> -
> - str = HeapAlloc(GetProcessHeap(), 0, sizeof(servicesW) + DriverServiceName->Length*sizeof(WCHAR));

Length is the length in bytes, so the calculation above is wrong.
> - lstrcpyW(str, servicesW);
> - lstrcatW(str, DriverServiceName->Buffer);

I don't think you can assume a terminating NULL character.
> -
> - attr.Length = sizeof(attr);
> - attr.RootDirectory = 0;
> - attr.ObjectName = &reg_str;
> - attr.Attributes = 0;
> - attr.SecurityDescriptor = NULL;
> - attr.SecurityQualityOfService = NULL;
> -
> - RtlInitUnicodeString(&reg_str, str);
> -
> - status = NtCreateKey(&driver_hkey, KEY_ALL_ACCESS, &attr, 0, NULL, 0, NULL);
> - if (status != STATUS_SUCCESS)
> - {
> -
>    ERR("cannot open key %s, err=0x%x\n", wine_dbgstr_w(str), status);
> -
>    HeapFree(GetProcessHeap(), 0, str);
> -
>    return status;
> - }
> - RtlInitUnicodeString(&keypath, str);
> -
> - /* read the executable path from memory */
> - size = 0;
> - RtlInitUnicodeString(&reg_str, ImagePathW);
> -
> - status = NtQueryValueKey(driver_hkey, &reg_str, KeyValuePartialInformation, buffer, info_size, &size);
> - if (status == STATUS_BUFFER_OVERFLOW)
> - {
> -
>    str = HeapAlloc(GetProcessHeap(), 0, size);

str was already set before, so this might leak memory, at least on some exit paths.
> -
>    info = (KEY_VALUE_PARTIAL_INFORMATION *) str;
> -
>    status = NtQueryValueKey(driver_hkey, &reg_str, KeyValuePartialInformation, str, size, &size);
> -
> -
>    if (status == STATUS_SUCCESS)
> -
>    {
> -
>        size = ExpandEnvironmentStringsW((LPCWSTR)info->Data,NULL,0);
> -
>        path = HeapAlloc(GetProcessHeap(),0,size*sizeof(WCHAR));
> -
>        ExpandEnvironmentStringsW((LPCWSTR)info->Data,path,size);
> -
>    }
> -
>    HeapFree(GetProcessHeap(), 0, str);
> -
>    if (!path)
> -
>    {
> -
>        TRACE("Failed to load driver path(0x%x): %s\n", status,
> -
>            wine_dbgstr_w((LPCWSTR)info->Data));

info->Data points into str, which was deallocated just before.
> -
>        NtClose(driver_hkey);
> -
>        return status;
> -
>    }
> -
> -
>    if (!strncmpiW(path, systemrootW, 12))
> -
>    {
> -
>        WCHAR buffer[MAX_PATH];

The variable name is the same as above, which might be misleading.
> -
> -
>        GetWindowsDirectoryW(buffer, MAX_PATH);
> -
> -
>        str = HeapAlloc(GetProcessHeap(), 0, (size -11 + strlenW(buffer))
> -
>                                                    * sizeof(WCHAR));
> -
>        lstrcpyW(str, buffer);
> -
>        lstrcatW(str, path + 11);
> -
>        HeapFree(GetProcessHeap(), 0, path);
> -
>        path = str;
> -
>    }
> -
>    else if (!strncmpW(path, ntprefixW, 4))
> -
>        str = path + 4;
> -
>    else
> -
>        str = path;
> - }
> - else
> - {
> -
>    /* default is to use the driver name + ".sys" */
> -
>    WCHAR buffer[MAX_PATH];
> -
>    GetSystemDirectoryW(buffer, MAX_PATH);
> -
>    path = HeapAlloc(GetProcessHeap(),0,
> -
>      (strlenW(buffer) + strlenW(driversW) + DriverServiceName->Length + strlenW(postfixW) + 1)
> -
>      *sizeof(WCHAR));

Similar to above, ->Length is the length in bytes.
> -
>    lstrcpyW(path, buffer);
> -
>    lstrcatW(path, driversW);
> -
>    lstrcatW(path, DriverServiceName->Buffer);
> -
>    lstrcatW(path, postfixW);
> -
>    str = path;
> - }
> - NtClose(driver_hkey);
> -
> - TRACE("loading driver %s\n", wine_dbgstr_w(str));
> -
> - module = load_driver_module(str);
> - HeapFree(GetProcessHeap(), 0, path);
> -
> - if (!module)
> -
>    return STATUS_NOT_FOUND;
> -
> - driver = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*driver));
> - driver->module = module;
> - RtlDuplicateUnicodeString(1, DriverServiceName, &driver->name);
> -
> - init_driver(DriverServiceName->Buffer, &keypath, driver);
> -
> - list_add_tail(&loaded_drivers, &driver->entry);
> -
> - return STATUS_SUCCESS;
>
> +}
> -
>
> +/***********************************************************************
> -
> -
>       ZwUnloadDriver (NTOSKRNL.EXE.@)
> - */
>
> +NTSTATUS WINAPI ZwUnloadDriver(const UNICODE_STRING *DriverServiceName)
> +{
> - loaded_driver *driver, *ptr;
> -
> - LIST_FOR_EACH_ENTRY_SAFE(driver, ptr, &loaded_drivers, loaded_driver, entry)
> - {
> -
>    if (RtlEqualUnicodeString(DriverServiceName, &driver->name, FALSE))
> -
>    {
> -
>        list_remove(&driver->entry);
> -
> -
>        if (driver->driver_obj.DriverUnload)
> -
>        {
> -
>            if (TRACE_ON(relay))
> -
>                DPRINTF("%04x:Call driver unload %p (obj=%p)\n", GetCurrentThreadId(),
> -
>                              driver->driver_obj.DriverUnload, &driver->driver_obj );
> -
> -
>            driver->driver_obj.DriverUnload( &driver->driver_obj);
> -
> -
>            if (TRACE_ON(relay))
> -
>                DPRINTF("%04x:Ret  driver unload %p (obj=%p)\n", GetCurrentThreadId(),
> -
>                              driver->driver_obj.DriverUnload, &driver->driver_obj);
> -
>        }
> -
> -
>        RtlFreeUnicodeString(&driver->name);
> -
>        RtlFreeUnicodeString(&driver->driver_extension.ServiceKeyName);
> -
>        FreeLibrary(driver->module);
> -
>        HeapFree(GetProcessHeap(), 0, driver);
> -
>        return STATUS_SUCCESS;
> -
>    }
> - }
> -
> - return STATUS_NOT_FOUND;
>
> +}
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> index 1319ada..a98a523 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> @@ -1314,7 +1314,7 @@
>  @ stdcall ZwFsControlFile(long long long long long long long long long long) ntdll.ZwFsControlFile
>  @ stdcall ZwInitiatePowerAction(long long long long) ntdll.ZwInitiatePowerAction
>  @ stdcall ZwIsProcessInJob(long long) ntdll.ZwIsProcessInJob
> -@ stdcall ZwLoadDriver(ptr) ntdll.ZwLoadDriver
> +@ stdcall ZwLoadDriver(ptr)
>  @ stdcall ZwLoadKey(ptr ptr) ntdll.ZwLoadKey
>  @ stdcall ZwMakeTemporaryObject(long) ntdll.ZwMakeTemporaryObject
>  @ stdcall ZwMapViewOfSection(long long ptr long long ptr ptr long long long) ntdll.ZwMapViewOfSection
> @@ -1384,7 +1384,7 @@
>  @ stdcall ZwTerminateJobObject(long long) ntdll.ZwTerminateJobObject
>  @ stdcall ZwTerminateProcess(long long) ntdll.ZwTerminateProcess
>  @ stub ZwTranslateFilePath
> -@ stdcall ZwUnloadDriver(ptr) ntdll.ZwUnloadDriver
> +@ stdcall ZwUnloadDriver(ptr)
>  @ stdcall ZwUnloadKey(long) ntdll.ZwUnloadKey
>  @ stdcall ZwUnmapViewOfSection(long ptr) ntdll.ZwUnmapViewOfSection
>  @ stdcall ZwWaitForMultipleObjects(long ptr long long ptr) ntdll.ZwWaitForMultipleObjects
> diff --git a/programs/winedevice/device.c b/programs/winedevice/device.c
> index ef1e1ef..cb6a6ba 100644
> --- a/programs/winedevice/device.c
> +++ b/programs/winedevice/device.c
> @@ -25,253 +25,19 @@
>
> #include "ntstatus.h"
>  #define WIN32_NO_STATUS
> -#include "windef.h"
> -#include "winbase.h"
>  #include "winternl.h"
> -#include "winreg.h"
> -#include "winnls.h"
>  #include "winsvc.h"
>  #include "ddk/wdm.h"
>  #include "wine/unicode.h"
>  #include "wine/debug.h"
>
> WINE_DEFAULT_DEBUG_CHANNEL(winedevice);
> -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 HKEY driver_hkey;
>  static HANDLE stop_event;
> -static DRIVER_OBJECT driver_obj;
> -static DRIVER_EXTENSION driver_extension;
> -
>
> -/* find the LDR_MODULE corresponding to the driver module */
> -static LDR_MODULE *find_ldr_module( HMODULE module )
> -{
> - LIST_ENTRY *entry, *list = &NtCurrentTeb()->Peb->LdrData->InMemoryOrderModuleList;
> -
> - for (entry = list->Flink; entry != list; entry = entry->Flink)
> - {
> -
>    LDR_MODULE *ldr = CONTAINING_RECORD(entry, LDR_MODULE, InMemoryOrderModuleList);
> -
>    if (ldr->BaseAddress == module) return ldr;
> -
>    if (ldr->BaseAddress > (void *)module) break;
> - }
> - return NULL;
>
> -}
> -
>
> -/* load the driver module file */
> -static HMODULE load_driver_module( const WCHAR *name )
> -{
> - IMAGE_NT_HEADERS *nt;
> - const IMAGE_IMPORT_DESCRIPTOR *imports;
> - SYSTEM_BASIC_INFORMATION info;
> - int i;
> - INT_PTR delta;
> - ULONG size;
> - HMODULE module = LoadLibraryW( name );
> -
> - if (!module) return NULL;
> - nt = RtlImageNtHeader( module );
> -
> - if (!(delta = (char *)module - (char *)nt->OptionalHeader.ImageBase)) return module;
> -
> - /* the loader does not apply relocations to non page-aligned binaries or executables,
> -
> * we have to do it ourselves */
> -
> - NtQuerySystemInformation( SystemBasicInformation, &info, sizeof(info), NULL );
> - if (nt->OptionalHeader.SectionAlignment < info.PageSize ||
> -
>    !(nt->FileHeader.Characteristics & IMAGE_FILE_DLL))
> - {
> -
>    DWORD old;
> -
>    IMAGE_BASE_RELOCATION *rel, *end;
> -
> -
>    if ((rel = RtlImageDirectoryEntryToData( module, TRUE, IMAGE_DIRECTORY_ENTRY_BASERELOC, &size )))
> -
>    {
> -
>        WINE_TRACE( "%s: relocating from %p to %p\n",
> -
>                    wine_dbgstr_w(name), (char *)module - delta, module );
> -
>        end = (IMAGE_BASE_RELOCATION *)((char *)rel + size);
> -
>        while (rel < end && rel->SizeOfBlock)
> -
>        {
> -
>            void *page = (char *)module + rel->VirtualAddress;
> -
>            VirtualProtect( page, info.PageSize, PAGE_EXECUTE_READWRITE, &old );
> -
>            rel = LdrProcessRelocationBlock( page, (rel->SizeOfBlock - sizeof(*rel)) / sizeof(USHORT),
> -
>                                             (USHORT *)(rel + 1), delta );
> -
>            if (old != PAGE_EXECUTE_READWRITE) VirtualProtect( page, info.PageSize, old, &old );
> -
>            if (!rel) goto error;
> -
>        }
> -
>        /* make sure we don't try again */
> -
>        size = FIELD_OFFSET( IMAGE_NT_HEADERS, OptionalHeader ) + nt->FileHeader.SizeOfOptionalHeader;
> -
>        VirtualProtect( nt, size, PAGE_READWRITE, &old );
> -
>        nt->OptionalHeader.DataDirectory[IMAGE_DIRECTORY_ENTRY_BASERELOC].VirtualAddress = 0;
> -
>        VirtualProtect( nt, size, old, &old );
> -
>    }
> - }
> -
> - /* make sure imports are relocated too */
> -
> - if ((imports = RtlImageDirectoryEntryToData( module, TRUE, IMAGE_DIRECTORY_ENTRY_IMPORT, &size )))
> - {
> -
>    for (i = 0; imports[i].Name && imports[i].FirstThunk; i++)
> -
>    {
> -
>        char *name = (char *)module + imports[i].Name;
> -
>        WCHAR buffer[32], *p = buffer;
> -
> -
>        while (p < buffer + 32) if (!(*p++ = *name++)) break;
> -
>        if (p <= buffer + 32) FreeLibrary( load_driver_module( buffer ) );
> -
>    }
> - }
> -
> - return module;
> -
>
> -error:
> - FreeLibrary( module );
> - 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 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;
> - 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 ))
> - {
> -
>    WINE_ERR( "cannot open key %s, err=%u\n", wine_dbgstr_w(str), GetLastError() );
> -
>    HeapFree( GetProcessHeap(), 0, str);
> -
>    return FALSE;
> - }
> - RtlInitUnicodeString( &keypath, str );
> -
> - /* read the executable path from memory */
> - size = 0;
> - if (!RegQueryValueExW( driver_hkey, ImagePathW, NULL, &type, NULL, &size ))
> - {
> -
>    str = HeapAlloc( GetProcessHeap(), 0, size );
> -
>    if (!RegQueryValueExW( driver_hkey, ImagePathW, NULL, &type, (LPBYTE)str, &size ))
> -
>    {
> -
>        size = ExpandEnvironmentStringsW(str,NULL,0);
> -
>        path = HeapAlloc(GetProcessHeap(),0,size*sizeof(WCHAR));
> -
>        ExpandEnvironmentStringsW(str,path,size);
> -
>    }
> -
>    HeapFree( GetProcessHeap(), 0, str );
> -
>    if (!path) return FALSE;
> -
> -
>    if (!strncmpiW( path, systemrootW, 12 ))
> -
>    {
> -
>        WCHAR buffer[MAX_PATH];
> -
> -
>        GetWindowsDirectoryW(buffer, MAX_PATH);
> -
> -
>        str = HeapAlloc(GetProcessHeap(), 0, (size -11 + strlenW(buffer))
> -
>                                                    * sizeof(WCHAR));
> -
>        lstrcpyW(str, buffer);
> -
>        lstrcatW(str, path + 11);
> -
>        HeapFree( GetProcessHeap(), 0, path );
> -
>        path = str;
> -
>    }
> -
>    else if (!strncmpW( path, ntprefixW, 4 ))
> -
>        str = path + 4;
> -
>    else
> -
>        str = path;
> - }
> - else
> - {
> -
>    /* default is to use the driver name + ".sys" */
> -
>    WCHAR buffer[MAX_PATH];
> -
>    GetSystemDirectoryW(buffer, MAX_PATH);
> -
>    path = HeapAlloc(GetProcessHeap(),0,
> -
>      (strlenW(buffer) + strlenW(driversW) + strlenW(driver_name) + strlenW(postfixW) + 1)
> -
>      *sizeof(WCHAR));
> -
>    lstrcpyW(path, buffer);
> -
>    lstrcatW(path, driversW);
> -
>    lstrcatW(path, driver_name);
> -
>    lstrcatW(path, postfixW);
> -
>    str = path;
> - }
> -
> - WINE_TRACE( "loading driver %s\n", wine_dbgstr_w(str) );
> -
> - module = load_driver_module( str );
> - HeapFree( GetProcessHeap(), 0, path );
> - if (!module) return NULL;
> -
> - init_driver( module, &keypath );
> - return module;
>
> -}
>
> static DWORD WINAPI service_handler( DWORD ctrl, DWORD event_type, LPVOID event_data, LPVOID context )
>  {
> @@ -305,7 +71,7 @@ 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;
> -
>
> UNICODE_STRING DriverName;
>
> WINE_TRACE( "starting service %s\n", wine_dbgstr_w(driver_name) );
>
> @@ -324,15 +90,16 @@ static void WINAPI ServiceMain( DWORD argc, LPWSTR *argv )
>      status.dwWaitHint                = 10000;
>      SetServiceStatus( service_handle, &status );
> - driver_module = load_driver();
> - if (driver_module)
> -
>
> RtlInitUnicodeString( &DriverName, driver_name );
> -
> -
>
> if ( ZwLoadDriver( &DriverName ) == ERROR_SUCCESS )
> {
>     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 );
> -
>    ZwUnloadDriver( &DriverName );
> }
>    else WINE_ERR( "driver %s failed to load\n", wine_dbgstr_w(driver_name) );

[PATCH v4] ntoskrnl.exe: Implement ZwLoadDriver and ZwUnloadDriver

[Alexandre Julliard]

Sebastian Lackner  writes:
> - In theory, all drivers should be loaded into the same address space. This means,
> winedevice and the wineserver has to keep track of them, and its not sufficient to
> maintain a list in ntoskrnl.exe.

We may want a system-wide list of drivers at some point, but so far we
haven't needed that. If it turns out to be necessary, it can certainly
be added later.

However, the goal is not to load all drivers in the same address space,
that would be very fragile. Drivers should be in separate processes,
except when they truly need to talk to each other; that what's this
patch is supposed to make possible.

[PATCH v4] ntoskrnl.exe: Implement ZwLoadDriver and ZwUnloadDriver

[Sebastian Lackner]

On 23.01.2016 15:44, Alexandre Julliard wrote:
> Sebastian Lackner  writes:
> > > - In theory, all drivers should be loaded into the same address space. This means,
> > > winedevice and the wineserver has to keep track of them, and its not sufficient to
> > > maintain a list in ntoskrnl.exe.
>
> We may want a system-wide list of drivers at some point, but so far we
> haven't needed that. If it turns out to be necessary, it can certainly
> be added later.
>
> However, the goal is not to load all drivers in the same address space,
> that would be very fragile. Drivers should be in separate processes,
> except when they truly need to talk to each other; that what's this
> patch is supposed to make possible.

We already have bugs for issues caused by drivers trying to communicate with
each other: https://bugs.winehq.org/show_bug.cgi?id=37356.

How do you want to decide which drivers should go into the same process, and
which not? As mentioned above, loading the same driver twice is something
which is not supported in most cases.

If a solution with marshalling all structs back and forth is preferred, there
is also no need to move the code for loading the driver. Instead, ZwLoadDriver
should spawn a new service then. However that only works as long as drivers
use officially documented structures.

[PATCH v4] ntoskrnl.exe: Implement ZwLoadDriver and ZwUnloadDriver

[Alexandre Julliard]

Sebastian Lackner  writes:
> We already have bugs for issues caused by drivers trying to communicate with
> each other: https://bugs.winehq.org/show_bug.cgi?id=37356.
>
> How do you want to decide which drivers should go into the same process, and
> which not? As mentioned above, loading the same driver twice is something
> which is not supported in most cases.

We could have configuration options for this, and maybe some heuristics
to detect it automatically when possible.
> If a solution with marshalling all structs back and forth is preferred, there
> is also no need to move the code for loading the driver. Instead, ZwLoadDriver
> should spawn a new service then. However that only works as long as drivers
> use officially documented structures.

I don't think we want to marshall kernel data structures across
processes.

[PATCH v4] ntoskrnl.exe: Implement ZwLoadDriver and ZwUnloadDriver

[Sebastian Lackner]

On 24.01.2016 06:04, Alexandre Julliard wrote:
> Sebastian Lackner  writes:
> > We already have bugs for issues caused by drivers trying to communicate with
> > each other: https://bugs.winehq.org/show_bug.cgi?id=37356.
> >
> > How do you want to decide which drivers should go into the same process, and
> > which not? As mentioned above, loading the same driver twice is something
> > which is not supported in most cases.
>
> We could have configuration options for this, and maybe some heuristics
> to detect it automatically when possible.
> > If a solution with marshalling all structs back and forth is preferred, there
> > is also no need to move the code for loading the driver. Instead, ZwLoadDriver
> > should spawn a new service then. However that only works as long as drivers
> > use officially documented structures.
>
> I don't think we want to marshall kernel data structures across
> processes.

I like the idea. Its not perfect, but a good compromise between isolation and
functionality. For drivers running in the same "kernel group", we should probably
use a separate thread for each one, so we can still catch most crashes. Its
relatively unlikely that a crash occurs while holding a loader or heap lock.

[PATCH v4] ntoskrnl.exe: Implement ZwLoadDriver and ZwUnloadDriver

[Aric Stewart]

Thanks for the review!

On 1/22/16 10:02 PM, Sebastian Lackner wrote:
> On 22.01.2016 19:15, Aric Stewart wrote:
> > Code is mostly moved from winedevice.exe
> > winedevice.exe now uses these functions
> > v3: No longer use advapi32 Registry functions
> > v4: Fix FALSE return code (thanks to Huw Davies for spotting this one)
> >      Remove WINE_ from debuging macros
> > Signed-off-by: Aric Stewart
> >
> > dlls/ntoskrnl.exe/ntoskrnl.c        | 306 ++++++++++++++++++++++++++++++++++++
> >   dlls/ntoskrnl.exe/ntoskrnl.exe.spec |   4 +-
> >   programs/winedevice/device.c        | 243 +---------------------------
> >   3 files changed, 313 insertions(+), 240 deletions(-)
>
> I don't think moving the loader code from winedevice to ntoskrnl.exe is a good idea,
> at least not without further changes, because:
> -
>
> We should not initialize the same driver twice, the second initialization could
>  fail because of namespace collisions.
> -
>
> Drivers should be properly uninitialized, no matter how they were loaded.
> -
>
> In theory, all drivers should be loaded into the same address space. This means,
>  winedevice and the wineserver has to keep track of them, and its not sufficient to
>  maintain a list in ntoskrnl.exe.
>
> Besides those design issues, I've marked a couple of other issues in the patch below.

I am going to let you and Alexandre continue to discuss this.  In theory you are correct, if we had proper kernel ring address space and such that would be true, but we don't. So we have to try to make do with what we can. What this does do is it lets my plug and play manager be able to load all its drivers into its own process space so that they can communicate. It opens the door to allowing things like that.

So if you have 2(+) drivers that need to be in the same process space then we can look at things like winedevice from being a service that loads each driver into a new process to being a single process that load each driver requested. But loading things like that into the same process is very fragile since if any driver crashes it can take down all the drivers in that process.
> > 0001-ntoskrnl.exe-Implement-ZwLoadDriver-and-ZwUnloadDriver.txt
> >
> > diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
> > index 04f0198..032ab21 100644

.
.
.
> It wouldn't hurt to add locking for such a global list.

Agreed, Added.  If we move this to ntdll then we can use the existing loader lock.

.
.
.
> > +/***********************************************************************
> > -
> > -
> >       ZwLoadDriver (NTOSKRNL.EXE.@)
> > - */
> >
> > +NTSTATUS WINAPI ZwLoadDriver(const UNICODE_STRING *DriverServiceName)
>
> Both the Nt* and Zw* function should do the same, so I would suggest to implement
> Nt* instead and forward Zw*, like its done in ntdll.

I would love to head Alexandre chime in on this. I could go either way. In our private discussions he lead me toward ZwLoadDriver but maybe I misunderstood and I can move this to NtLoadDriver if that is what is better.

Your other comments are also very helpful and I have made those fixes.

I will not submit a v5 until we figure out if this should remain in ntoskrnl or move to ntdll but then I will push a v5.

Thanks again for the review!

-aric

[PATCH v4] ntoskrnl.exe: Implement ZwLoadDriver and ZwUnloadDriver

[Alexandre Julliard]

Aric Stewart  writes:
> I would love to head Alexandre chime in on this. I could go either
> way. In our private discussions he lead me toward ZwLoadDriver but
> maybe I misunderstood and I can move this to NtLoadDriver if that is
> what is better.

There's no NtLoadDriver in ntoskrnl, which makes sense given its
purpose. I don't think we want to invent our own.

There's NtLoadDriver in ntdll, but its behavior should of course be
different, it's meant to be called from userspace instead of kernel
space.

[PATCH v4] ntoskrnl.exe: Implement ZwLoadDriver and ZwUnloadDriver

[Sebastian Lackner]

On 24.01.2016 05:58, Alexandre Julliard wrote:
> Aric Stewart  writes:
> > I would love to head Alexandre chime in on this. I could go either
> > way. In our private discussions he lead me toward ZwLoadDriver but
> > maybe I misunderstood and I can move this to NtLoadDriver if that is
> > what is better.
>
> There's no NtLoadDriver in ntoskrnl, which makes sense given its
> purpose. I don't think we want to invent our own.
>
> There's NtLoadDriver in ntdll, but its behavior should of course be
> different, it's meant to be called from userspace instead of kernel
> space.

Thanks for pointing this out, I wasn't aware that NtLoadDriver is not
accessible from kernel mode.