Re: [PATCH 2/4] ntoskrnl.exe: Implement loading plug and play devices

[Sebastian Lackner]

On 29.08.2016 20:49, Aric Stewart wrote:
> Signed-off-by: Aric Stewart
>
> dlls/ntoskrnl.exe/pnp_manager.c | 150 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 150 insertions(+)
>
> 0002-ntoskrnl.exe-Implement-loading-plug-and-play-devices.txt
>
> diff --git a/dlls/ntoskrnl.exe/pnp_manager.c b/dlls/ntoskrnl.exe/pnp_manager.c
> index 5b42a0a..f1f3109 100644
> --- a/dlls/ntoskrnl.exe/pnp_manager.c
> +++ b/dlls/ntoskrnl.exe/pnp_manager.c
> @@ -17,6 +17,7 @@
> - Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>
> */
>
> +#define NONAMELESSUNION
>  #include <stdarg.h>
>  #include "ntstatus.h"
>  #define WIN32_NO_STATUS
> @@ -27,6 +28,7 @@
>  #include "cfgmgr32.h"
>  #include "winsvc.h"
>  #include "winreg.h"
> +#include "wine/unicode.h"
>  #include "wine/debug.h"
>  #include "wine/list.h"
>
> @@ -34,6 +36,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>
> #define MAX_SERVICE_NAME 260
>
> +typedef NTSTATUS WINAPI (*pAddDevice)(DRIVER_OBJECT *DriverObject, DEVICE_OBJECT *PhysicalDeviceObject);

My Win10 header files contain DRIVER_EXTENSION with the appropriate definition, which
would make the casts unnecessary.
> -
>
> typedef struct _device_driver {
>      struct list entry;
>
> @@ -105,6 +109,143 @@ static device_driver* register_device_driver(const WCHAR *drivername, const WCHA
>      return ptr;
>  }
>
> +static NTSTATUS WINAPI internalComplete(DEVICE_OBJECT *deviceObject, IRP *irp,
> - void *context )
>
> +{
> - SetEvent(irp->UserEvent);
> - return STATUS_MORE_PROCESSING_REQUIRED;
>
> +}
> -
>
> +static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR **id)
> +{
> - NTSTATUS status;
> - IO_STACK_LOCATION *irpsp;
> - IO_STATUS_BLOCK irp_status;
> - IRP *irp;
> - HANDLE event = CreateEventA(NULL, FALSE, FALSE, NULL);
> -
> - irp = IoBuildSynchronousFsdRequest(IRP_MJ_PNP, device, NULL, 0, NULL, NULL, &irp_status);
> - if (irp == NULL)
> -
>    return STATUS_NO_MEMORY;

The return path here will leak the event handle. The same issue was recently fixed in hidclass.sys.
Also I'm wondering a bit, do we need to keep this identical code at both places?
> -
> - irp->UserEvent = event;
> - irpsp = IoGetNextIrpStackLocation(irp);
> - irpsp->MinorFunction = IRP_MN_QUERY_ID;
> - irpsp->Parameters.QueryId.IdType = type;
> - irpsp->CompletionRoutine = internalComplete;
> - irpsp->Control = SL_INVOKE_ON_SUCCESS | SL_INVOKE_ON_ERROR;
> -
> - IoCallDriver(device, irp);
> -
> - if (irp->IoStatus.u.Status == STATUS_PENDING)
> -
>    WaitForSingleObject(event, INFINITE);
> -
> - *id = (WCHAR*)irp->IoStatus.Information;
> - status = irp->IoStatus.u.Status;
> - IoCompleteRequest(irp, IO_NO_INCREMENT );
> - CloseHandle(event);
> -
> - return status;
>
> +}
> -
>
> +static void handle_bus_relations(DEVICE_OBJECT *device)
> +{
> - static const WCHAR szDriverW[] = {'\','D','r','i','v','e','r','\',0};
> -
> - NTSTATUS status;
> - WCHAR *id, *idptr;
> - device_driver *driver = NULL;
> - WCHAR fullname[MAX_PATH];
> - DRIVER_OBJECT *driver_obj;
> - UNICODE_STRING driverName;
> -
> - TRACE("Device %p\n", device);
> -
> - /* We could(should?) do a full IRP_MN_QUERY_DEVICE_RELATIONS query,
> -
> * but we dont have to, We have the DEVICE_OBJECT of the new device
> -
> * so we can simply handle the process here */
> -
> - status = get_device_id(device, BusQueryCompatibleIDs, &id);
> - if (status != ERROR_SUCCESS || !id)
> - {
> -
>    ERR("Failed to get device IDs\n");
> -
>    return;
> - }
> -
> - idptr = id;
> - do {
> -
>    device_driver *ptr;
> -
> -
>    TRACE("Checking for id %s\n",debugstr_w(idptr));
> -
> -
>    /* Check loaded drivers */
> -
>    LIST_FOR_EACH_ENTRY(ptr, &device_drivers, device_driver, entry)
> -
>    {
> -
>        int i;
> -
>        for (i = 0; i < ptr->id_count; i++)
> -
>        {
> -
>            if (lstrcmpW(idptr, ptr->matching_ids[i]) == 0)
> -
>            {
> -
>                driver = ptr;
> -
>                break;

Is it intentional that this does not abort the outer loop?
> -
>            }
> -
>        }
> -
>    }
> -
>    idptr += (lstrlenW(idptr) + 1);
> - } while (!driver && *idptr != 0);
> -
> - HeapFree(GetProcessHeap(), 0, id);
> -
> - if (!driver)
> - {
> -
>    ERR("No matching driver found for device\n");
> -
>    return;
> - }
> -
> - strcpyW(fullname, szDriverW);
> - strcatW(fullname, driver->driver_name);
> - RtlInitUnicodeString(&driverName, fullname);
> -
> - if (!driver->loaded)
> - {
> -
>    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};
> -
>    WCHAR driverKey[MAX_PATH];
> -
>    UNICODE_STRING driverPath;
> -
> -
>    strcpyW(driverKey, servicesW);
> -
>    strcatW(driverKey, driver->driver_name);
> -
>    RtlInitUnicodeString(&driverPath, driverKey);
> -
>    if (ZwLoadDriver(&driverPath) != STATUS_SUCCESS)
> -
>    {
> -
>        ERR("Failed to load driver %s\n",debugstr_w(driver->driver_name));
> -
>        return;
> -
>    }
> -
>    driver->loaded = TRUE;
> - }
> -
> - if (ObReferenceObjectByName(&driverName, OBJ_CASE_INSENSITIVE, NULL,
> -
>    0, NULL, KernelMode, NULL, (void**)&driver_obj) != STATUS_SUCCESS)
> - {
> -
>    ERR("Failed to locate loaded driver\n");
> -
>    return;
> - }
> -
> - status = ((pAddDevice)driver_obj->DriverExtension->AddDevice)(driver_obj, device);

It would be better to handle the lack of a AddDevice routine.
> -
> - ObDereferenceObject(driver_obj);
> -
> - if (status != STATUS_SUCCESS)
> - {
> -
>    ERR("AddDevice failed\n");
> -
>    return;
> - }
>
> +}
> -
>
> static void initialize_driver_store(void)
>  {
>      static const WCHAR criticalW[] =
> @@ -181,5 +322,14 @@ VOID WINAPI IoInvalidateDeviceRelations(DEVICE_OBJECT *DeviceObject, DEVICE_RELA
>          initialized = TRUE;
>          initialize_driver_store();
>      }
> -
> - switch (Type)
> - {
> -
>    case BusRelations:
> -
>        handle_bus_relations(DeviceObject);
> -
>        break;
> -
>    default:
> -
>        FIXME("Unhandled Relation %i\n", Type);
> - }
> LeaveCriticalSection(&pnp_cs);
>
> }