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 <aric(a)codeweavers.com>
> ---
>  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);
>  }