Re: [PATCH 1/5] ntoskrnl: Implement IoSetDevicePropertyData().

[Zebediah Figura (she/her)]

On 4/26/21 5:37 AM, Rémi Bernon wrote:
> From: Arkadiusz Hiler
> Signed-off-by: Rémi Bernon
>
> In PATCH 1, the return GetLastError() look wrong to me but the same is
> done in IoGetDeviceProperty so I kept them. I guess we would need to
> convert the setupapi error back to NTSTATUS, or implement the higher
> level setupapi on top of these lower level primitives intead.

It's maybe not exactly ideal, but it's also not clear how to map 
setupapi errors to NTSTATUS, and moreover it probably doesn't matter 
anyway (we don't expect those functions to fail).

I don't think we can implement setupapi functions on top of ntoskrnl 
ones; that crosses the kernel boundary (and there's no clear syscall 
interface for it). Probably on Windows the code is just duplicated.

FWIW, we have a PnP test driver now, so it's actually possible to write 
tests for this function.
> In PATCH 2, I moved the error case before IoInvalidateDeviceRelations,
> because I don't know what the call is actually expecting. Maybe it was
> unnecessary.

I think it's correct as-is to move it—as soon as we call 
IoInvalidateDeviceRelations() the device can be exposed.

Actually, to be completely correct (i.e. if we were running this driver 
in Windows), I think we should treat it as possible to receive 
IRP_MN_QUERY_DEVICE_RELATIONS at any time. What we probably should be 
doing is to initialize everything necessary *before* the statement 
"fdo_ext->u.fdo.child_pdo = child_pdo", and then everything else at 
IRP_MN_START_DEVICE (on the child PDO).
> dlls/ntoskrnl.exe/ntoskrnl.exe.spec |  1 +
>   dlls/ntoskrnl.exe/pnp.c             | 51 +++++++++++++++++++++++++++++
>   include/ddk/wdm.h                   |  4 +++
>   3 files changed, 56 insertions(+)
>
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> index 2e5e2e6e11b..4eb08faec2e 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> @@ -465,6 +465,7 @@
>   @ stdcall IoReuseIrp(ptr long)
>   @ stub IoSetCompletionRoutineEx
>   @ stdcall IoSetDeviceInterfaceState(ptr long)
> +@ stdcall IoSetDevicePropertyData(ptr ptr long long long long ptr)
>   @ stub IoSetDeviceToVerify
>   @ stub IoSetFileOrigin
>   @ stub IoSetHardErrorOrVerifyDevice
> diff --git a/dlls/ntoskrnl.exe/pnp.c b/dlls/ntoskrnl.exe/pnp.c
> index 095344b1073..e425712d738 100644
> --- a/dlls/ntoskrnl.exe/pnp.c
> +++ b/dlls/ntoskrnl.exe/pnp.c
> @@ -38,6 +38,12 @@ DEFINE_GUID(GUID_NULL,0,0,0,0,0,0,0,0,0,0,0);
>
> WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
>
> +static inline const char *debugstr_propkey( const DEVPROPKEY *id )
> +{
> - if (!id) return "(null)";
> - return wine_dbg_sprintf( "{%s,%04x}", wine_dbgstr_guid( &id->fmtid ), id->pid );
>
> +}
> -
>
> #define MAX_SERVICE_NAME 260
>
> struct device_interface
>
> @@ -783,6 +789,51 @@ NTSTATUS WINAPI IoSetDeviceInterfaceState( UNICODE_STRING *name, BOOLEAN enable
>       return ret;
>   }
>
> +/***********************************************************************
> -
> -
>       IoSetDevicePropertyData (NTOSKRNL.EXE.@)
> - */
>
> +NTSTATUS WINAPI IoSetDevicePropertyData( DEVICE_OBJECT *device, const DEVPROPKEY *property_key, LCID lcid,
> -
>                                     ULONG flags, DEVPROPTYPE type, ULONG size, void *data )
>
> +{
> - SP_DEVINFO_DATA sp_device = {sizeof(sp_device)};
> - WCHAR device_instance_id[MAX_DEVICE_ID_LEN];
> - NTSTATUS status;
> - HDEVINFO set;
> -
> - TRACE( "device %p, property_key %s, lcid %#x, flags %#x, type %#x, size %u, data %p.\n",
> -
>       device, debugstr_propkey(property_key), lcid, flags, type, size, data );
> -
> - /* flags is always treated as PLUGPLAY_PROPERTY_PERSISTENT starting with Win 8 / 2012 */
> -
> - if (lcid != LOCALE_NEUTRAL) FIXME( "only LOCALE_NEUTRAL is supported\n" );
> -
> - if ((status = get_device_instance_id( device, device_instance_id ))) return status;
> -
> - if ((set = SetupDiCreateDeviceInfoList( &GUID_NULL, NULL )) == INVALID_HANDLE_VALUE)
> - {
> -
>    ERR( "Failed to create device list, error %#x.\n", GetLastError() );
> -
>    return GetLastError();
> - }
> -
> - if (!SetupDiOpenDeviceInfoW( set, device_instance_id, NULL, 0, &sp_device ))
> - {
> -
>    ERR( "Failed to open device, error %#x.\n", GetLastError() );
> -
>    SetupDiDestroyDeviceInfoList( set );
> -
>    return GetLastError();
> - }
> -
> - if (!SetupDiSetDevicePropertyW( set, &sp_device, property_key, type, data, size, 0 ))
> - {
> -
>    ERR( "Failed to set property, error %#x.\n", GetLastError() );
> -
>    SetupDiDestroyDeviceInfoList( set );
> -
>    return GetLastError();
> - }
> -
> - SetupDiDestroyDeviceInfoList( set );
> -
> - return STATUS_SUCCESS;
>
> +}
> - /***********************************************************************
> -
>       IoRegisterDeviceInterface (NTOSKRNL.EXE.@)
> */
>
> diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h
> index 51097bfe3ab..447833c4bc9 100644
> --- a/include/ddk/wdm.h
> +++ b/include/ddk/wdm.h
> @@ -21,6 +21,7 @@
>   #define _NTDDK_
>
> #include <ntstatus.h>
> +#include <devpropdef.h>
>
> #ifdef _WIN64
>   #define POINTER_ALIGNMENT DECLSPEC_ALIGN(8)
> @@ -1681,6 +1682,8 @@ void      WINAPI ExReleaseResourceForThreadLite(ERESOURCE*,ERESOURCE_THREAD);
>   ULONG     WINAPI ExSetTimerResolution(ULONG,BOOLEAN);
>   void      WINAPI ExUnregisterCallback(void*);
>
> +#define PLUGPLAY_PROPERTY_PERSISTENT 0x0001
> - void      WINAPI IoAcquireCancelSpinLock(KIRQL*);
> NTSTATUS  WINAPI IoAcquireRemoveLockEx(IO_REMOVE_LOCK*,void*,const char*,ULONG, ULONG);
> NTSTATUS  WINAPI IoAllocateDriverObjectExtension(PDRIVER_OBJECT,PVOID,ULONG,PVOID*);
>
> @@ -1728,6 +1731,7 @@ void      WINAPI IoReleaseRemoveLockAndWaitEx(IO_REMOVE_LOCK*,void*,ULONG);
>   void      WINAPI IoReleaseRemoveLockEx(IO_REMOVE_LOCK*,void*,ULONG);
>   void      WINAPI IoReuseIrp(IRP*,NTSTATUS);
>   NTSTATUS  WINAPI IoSetDeviceInterfaceState(UNICODE_STRING*,BOOLEAN);
> +NTSTATUS  WINAPI IoSetDevicePropertyData(DEVICE_OBJECT*,const DEVPROPKEY*,LCID,ULONG,DEVPROPTYPE,ULONG,void*);
>   NTSTATUS  WINAPI IoWMIRegistrationControl(PDEVICE_OBJECT,ULONG);
>
> void    FASTCALL KeAcquireInStackQueuedSpinLockAtDpcLevel(KSPIN_LOCK*,KLOCK_QUEUE_HANDLE*);