[PATCH v2 0/1] MR3007: ntdll: Avoid requests with null pointer but invalid size.
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51770 - v1: - https://www.winehq.org/pipermail/wine-devel/2021-December/202913.html - https://www.winehq.org/pipermail/wine-devel/2022-April/213866.html (resend) - v2: - https://www.winehq.org/pipermail/wine-devel/2022-April/214306.html - Validate pointer before calling wine_server_add_data. - v3: - https://www.winehq.org/pipermail/wine-devel/2022-May/218285.html - Move check to DeviceIoControl. - Always use fixed values for IOCTL_STORAGE_GET_DEVICE_NUMBER. - Remove warning at expected hang. - v4: - Fix typo in commit message. -- v2: ntdll: Avoid requests with null pointer but invalid size. https://gitlab.winehq.org/wine/wine/-/merge_requests/3007
From: Bernhard Übelacker <bernhardu(a)mailbox.org> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51770 --- dlls/kernel32/tests/volume.c | 16 ++++++++++++++++ dlls/kernelbase/file.c | 5 +++++ 2 files changed, 21 insertions(+) diff --git a/dlls/kernel32/tests/volume.c b/dlls/kernel32/tests/volume.c index 5d2d971232f..115e8e75293 100644 --- a/dlls/kernel32/tests/volume.c +++ b/dlls/kernel32/tests/volume.c @@ -620,6 +620,7 @@ static void test_disk_query_property(void) STORAGE_PROPERTY_QUERY query = {0}; STORAGE_DESCRIPTOR_HEADER header = {0}; STORAGE_DEVICE_DESCRIPTOR descriptor = {0}; + STORAGE_DEVICE_NUMBER device_number = {0}; HANDLE handle; DWORD error; DWORD size; @@ -656,6 +657,21 @@ static void test_disk_query_property(void) ok(descriptor.Version == sizeof(descriptor), "got descriptor.Version %ld\n", descriptor.Version); ok(descriptor.Size >= sizeof(descriptor), "got descriptor.Size %ld\n", descriptor.Size); + SetLastError(0xdeadbeef); + ret = DeviceIoControl(handle, IOCTL_STORAGE_GET_DEVICE_NUMBER, NULL, 0, &device_number, sizeof(device_number), &size, NULL); + error = GetLastError(); + ok(ret, "expect ret %#x, got %#x\n", TRUE, ret); + ok(error == 0xdeadbeef, "expect err %#x, got err %#lx\n", 0xdeadbeef, error); + ok(size == sizeof(device_number), "got size %ld\n", size); + + /* unclean call with valid in_buffer=NULL but incorrect in_size=4 */ + SetLastError(0xdeadbeef); + ret = DeviceIoControl(handle, IOCTL_STORAGE_GET_DEVICE_NUMBER, NULL, 4, &device_number, sizeof(device_number), &size, NULL); + error = GetLastError(); + ok(ret, "expect ret %#x, got %#x\n", TRUE, ret); + ok(error == 0xdeadbeef, "expect err %#x, got err %#lx\n", 0xdeadbeef, error); + ok(size == sizeof(device_number), "got size %ld\n", size); + CloseHandle(handle); } diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index 7c2e132bdcb..936a822d169 100644 --- a/dlls/kernelbase/file.c +++ b/dlls/kernelbase/file.c @@ -4146,6 +4146,11 @@ BOOL WINAPI DECLSPEC_HOTPATCH DeviceIoControl( HANDLE handle, DWORD code, void * TRACE( "(%p,%lx,%p,%ld,%p,%ld,%p,%p)\n", handle, code, in_buff, in_count, out_buff, out_count, returned, overlapped ); + if (code == IOCTL_STORAGE_GET_DEVICE_NUMBER) { + in_buff = NULL; + in_count = 0; + } + if (overlapped) { piosb = (IO_STATUS_BLOCK *)overlapped; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3007
This looks very wrong. Is this really supposed to be done in kernel32 and not ntdll? Is this really specific to IOCTL_STORAGE_GET_DEVICE_NUMBER? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3007#note_35030
On Wed Jun 7 22:34:48 2023 +0000, Zebediah Figura wrote:
This looks very wrong. Is this really supposed to be done in kernel32 and not ntdll? Is this really specific to IOCTL_STORAGE_GET_DEVICE_NUMBER? Thanks for having a look.
That is the callstack up to the wineserver call: ``` in writev () from libc.so.6 in send_request (req=req(a)entry=0x22fbc0) at dlls/ntdll/unix/server.c:219 in server_call_unlocked (req_ptr=0x22fbc0) at dlls/ntdll/unix/server.c:292 in virtual_locked_server_call (req_ptr=0x22fbc0) at dlls/ntdll/unix/virtual.c:3821 in server_ioctl_file (handle=0x17c, ...) at dlls/ntdll/unix/file.c:5132 in NtDeviceIoControlFile (handle=0x17c, ..., in_size=4, out_buffer=0x60f428, out_size=12) at dlls/ntdll/unix/file.c:5985 in __wine_syscall_dispatcher () from dlls/ntdll/ntdll.so in NtDeviceIoControlFile(a)40 () from .wine/drive_c/windows/syswow64/ntdll.dll in DeviceIoControl(a)32 (...) at dlls/kernelbase/file.c:4168 in KERNEL32_DeviceIoControl(a)32 (...) at dlls/kernel32/file.c:411 in libKF5Solid!_ZN5Solid8Backends3Win16WinDeviceManager20getDeviceInfoPrivateI22_STORAGE_DEVICE_NUMBERPvEEvRK7QStringiPT_mPT0_ () from .wine/drive_c/digiKam/libKF5Solid.dll ``` So that has better to go into `NtDeviceIoControlFile` ? I am not sure if there are other IOCTLs, I have just looked at this application with this specific IOCTL. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3007#note_35041
So that has better to go into `NtDeviceIoControlFile` ?
I am not sure if there are other IOCTLs, I have just looked at this application with this specific IOCTL.
I can't answer that question without testing. My questions were kind of meant to suggest specific tests. IIRC METHOD_BUFFERED ioctls (and IOCTL_STORAGE_GET_DEVICE_NUMBER is one of these) are supposed to have valid pointers regardless of whether they're used, although it's been a while and I'm not fully sure of this. Now, with that said, Alexandre's original review [1] may ultimately be problematic, in that I believe non-BUFFERED ioctls do *not* validate their pointers, but rather just pass them through directly to the driver, and it's up to the driver to decide whether it wants to use that data. In that case "either it's needed and it should fail without sending the request, or it's not needed and it should never be sent at all" simply can't work—since we have no idea from ntdll whether it's needed, but shouldn't fail if it's not—and something like the original form of this patch is probably the best we can do. [1] https://www.winehq.org/pipermail/wine-devel/2022-May/215635.html -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3007#note_35043
participants (2)
-
Bernhard Übelacker -
Zebediah Figura (＠zfigura)