[PATCH v5 0/6] MR3007: Draft: 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: (d4a7440a) - 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: (6a69c680) - Fix typo in commit message. - Modified format strings. - v5: (47784feb) - Move test to own test function. - Move check to wine_server_add_data. - v6: (8c641d09) - Move check to NtDeviceIoControlFile - Add (at least temporary) tests to compare with NtDeviceIoControlFile and NtFsControlFile. -- v5: kernel32/tests: Add test for IOCTL_STORAGE_GET_DEVICE_NUMBER. ntdll: Add validation for input and output buffers in NtDeviceIoControlFile. ntoskrnl.exe: Allow sending a null input buffer to the driver. ntoskrnl.exe: Add todo_wine_if to test_ioctl_buffers. ntoskrnl.exe: Remove buffer assignments in test_ioctl_buffers. ntoskrnl.exe: Add test_ioctl_buffers by Zebediah Figura. https://gitlab.winehq.org/wine/wine/-/merge_requests/3007
From: Bernhard Übelacker <bernhardu(a)mailbox.org> https://gitlab.winehq.org/wine/wine/-/merge_requests/3007#note_36196 https://testbot.winehq.org/JobDetails.pl?Key=133935 --- dlls/ntoskrnl.exe/tests/driver.c | 48 ++++++++++++++++++++++++++++++ dlls/ntoskrnl.exe/tests/driver.h | 4 +++ dlls/ntoskrnl.exe/tests/ntoskrnl.c | 47 +++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+) diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c index a80bef78fab..7ac06126a66 100644 --- a/dlls/ntoskrnl.exe/tests/driver.c +++ b/dlls/ntoskrnl.exe/tests/driver.c @@ -2501,6 +2501,48 @@ static NTSTATUS test_load_driver_ioctl(IRP *irp, IO_STACK_LOCATION *stack, ULONG return ZwUnloadDriver(&name); } +static NTSTATUS empty_ioctl(IRP *irp, IO_STACK_LOCATION *stack) +{ + ULONG code = stack->Parameters.DeviceIoControl.IoControlCode; + const void *input_buffer, *output_buffer; + + input_buffer = irp->AssociatedIrp.SystemBuffer; + output_buffer = irp->AssociatedIrp.SystemBuffer; + + if (code == IOCTL_WINETEST_EMPTY_NEITHER) + { + input_buffer = stack->Parameters.DeviceIoControl.Type3InputBuffer; + output_buffer = irp->UserBuffer; + } + else if (code == IOCTL_WINETEST_EMPTY_IN_DIRECT) + { + if (irp->MdlAddress) + input_buffer = MmGetSystemAddressForMdlSafe(irp->MdlAddress, NormalPagePriority); + else + input_buffer = NULL; + input_buffer = irp->MdlAddress; + } + else if (code == IOCTL_WINETEST_EMPTY_OUT_DIRECT) + { + if (irp->MdlAddress) + output_buffer = MmGetSystemAddressForMdlSafe(irp->MdlAddress, NormalPagePriority); + else + output_buffer = NULL; + output_buffer = irp->MdlAddress; + } + + if (input_buffer) + return STATUS_INVALID_PARAMETER_1; + if (stack->Parameters.DeviceIoControl.InputBufferLength) + return STATUS_INVALID_PARAMETER_2; + if (output_buffer) + return STATUS_INVALID_PARAMETER_3; + if (stack->Parameters.DeviceIoControl.OutputBufferLength) + return STATUS_INVALID_PARAMETER_4; + + return STATUS_END_OF_FILE; +} + static NTSTATUS completion_ioctl(DEVICE_OBJECT *device, IRP *irp, IO_STACK_LOCATION *stack) { if (device == upper_device) @@ -2617,6 +2659,12 @@ static NTSTATUS WINAPI driver_IoControl(DEVICE_OBJECT *device, IRP *irp) break; case IOCTL_WINETEST_COMPLETION: return completion_ioctl(device, irp, stack); + case IOCTL_WINETEST_EMPTY_BUFFERED: + case IOCTL_WINETEST_EMPTY_IN_DIRECT: + case IOCTL_WINETEST_EMPTY_OUT_DIRECT: + case IOCTL_WINETEST_EMPTY_NEITHER: + status = empty_ioctl(irp, stack); + break; default: break; } diff --git a/dlls/ntoskrnl.exe/tests/driver.h b/dlls/ntoskrnl.exe/tests/driver.h index 34dfdca476c..160391c990b 100644 --- a/dlls/ntoskrnl.exe/tests/driver.h +++ b/dlls/ntoskrnl.exe/tests/driver.h @@ -36,6 +36,10 @@ #define IOCTL_WINETEST_RETURN_STATUS_DIRECT CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80a, METHOD_OUT_DIRECT, FILE_ANY_ACCESS) #define IOCTL_WINETEST_RETURN_STATUS_NEITHER CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80a, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_WINETEST_COMPLETION CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80c, METHOD_NEITHER, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_EMPTY_BUFFERED CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80d, METHOD_BUFFERED, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_EMPTY_IN_DIRECT CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80d, METHOD_IN_DIRECT, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_EMPTY_OUT_DIRECT CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80d, METHOD_OUT_DIRECT, FILE_ANY_ACCESS) +#define IOCTL_WINETEST_EMPTY_NEITHER CTL_CODE(FILE_DEVICE_UNKNOWN, 0x80d, METHOD_NEITHER, FILE_ANY_ACCESS) #define IOCTL_WINETEST_BUS_MAIN CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x800, METHOD_BUFFERED, FILE_ANY_ACCESS) #define IOCTL_WINETEST_BUS_REGISTER_IFACE CTL_CODE(FILE_DEVICE_BUS_EXTENDER, 0x801, METHOD_BUFFERED, FILE_ANY_ACCESS) diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 859dab4b3a9..63c7c5bb2b6 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1148,6 +1148,52 @@ static void test_blocking_irp(void) CloseHandle(file); } +static void test_ioctl_buffers(void) +{ + IO_STATUS_BLOCK io; + NTSTATUS status; + unsigned int j; + + static const struct + { + void *in; + ULONG in_size; + void *out; + ULONG out_size; + NTSTATUS buffered_status, direct_status, neither_status; + } + param_tests[] = + { + {NULL, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_END_OF_FILE}, + {NULL, 4, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_2}, + {(void *)16, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_1}, + {(void *)16, 4, NULL, 0, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_1}, + {NULL, 0, NULL, 4, STATUS_END_OF_FILE, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_4}, + {NULL, 0, (void *)16, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_3}, + {NULL, 0, (void *)16, 4, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_3}, + }; + + for (j = 0; j < ARRAY_SIZE(param_tests); ++j) + { + winetest_push_context("test %u", j); + + status = NtDeviceIoControlFile(device, NULL, NULL, NULL, &io, IOCTL_WINETEST_EMPTY_BUFFERED, + param_tests[j].in, param_tests[j].in_size, param_tests[j].out, param_tests[j].out_size); + ok(status == param_tests[j].buffered_status, "got %#lx\n", status); + status = NtDeviceIoControlFile(device, NULL, NULL, NULL, &io, IOCTL_WINETEST_EMPTY_IN_DIRECT, + param_tests[j].in, param_tests[j].in_size, param_tests[j].out, param_tests[j].out_size); + ok(status == param_tests[j].direct_status, "got %#lx\n", status); + status = NtDeviceIoControlFile(device, NULL, NULL, NULL, &io, IOCTL_WINETEST_EMPTY_OUT_DIRECT, + param_tests[j].in, param_tests[j].in_size, param_tests[j].out, param_tests[j].out_size); + ok(status == param_tests[j].direct_status, "got %#lx\n", status); + status = NtDeviceIoControlFile(device, NULL, NULL, NULL, &io, IOCTL_WINETEST_EMPTY_NEITHER, + param_tests[j].in, param_tests[j].in_size, param_tests[j].out, param_tests[j].out_size); + ok(status == param_tests[j].neither_status, "got %#lx\n", status); + + winetest_pop_context(); + } +} + static void test_driver3(struct testsign_context *ctx) { WCHAR filename[MAX_PATH]; @@ -1954,6 +2000,7 @@ START_TEST(ntoskrnl) test_return_status(); test_object_info(); test_blocking_irp(); + test_ioctl_buffers(); /* We need a separate ioctl to call IoDetachDevice(); calling it in the * driver unload routine causes a live-lock. */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3007
From: Bernhard Übelacker <bernhardu(a)mailbox.org> Are these intentional for METHOD_IN_DIRECT/METHOD_OUT_DIRECT or just leftovers? --- dlls/ntoskrnl.exe/tests/driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/driver.c b/dlls/ntoskrnl.exe/tests/driver.c index 7ac06126a66..25dcd092f0d 100644 --- a/dlls/ntoskrnl.exe/tests/driver.c +++ b/dlls/ntoskrnl.exe/tests/driver.c @@ -2520,7 +2520,6 @@ static NTSTATUS empty_ioctl(IRP *irp, IO_STACK_LOCATION *stack) input_buffer = MmGetSystemAddressForMdlSafe(irp->MdlAddress, NormalPagePriority); else input_buffer = NULL; - input_buffer = irp->MdlAddress; } else if (code == IOCTL_WINETEST_EMPTY_OUT_DIRECT) { @@ -2528,7 +2527,6 @@ static NTSTATUS empty_ioctl(IRP *irp, IO_STACK_LOCATION *stack) output_buffer = MmGetSystemAddressForMdlSafe(irp->MdlAddress, NormalPagePriority); else output_buffer = NULL; - output_buffer = irp->MdlAddress; } if (input_buffer) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3007
From: Bernhard Übelacker <bernhardu(a)mailbox.org> --- dlls/ntoskrnl.exe/tests/ntoskrnl.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index 63c7c5bb2b6..c1db4a6b650 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1161,16 +1161,17 @@ static void test_ioctl_buffers(void) void *out; ULONG out_size; NTSTATUS buffered_status, direct_status, neither_status; + int todowine[4]; } param_tests[] = { - {NULL, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_END_OF_FILE}, - {NULL, 4, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_2}, - {(void *)16, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_1}, - {(void *)16, 4, NULL, 0, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_1}, - {NULL, 0, NULL, 4, STATUS_END_OF_FILE, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_4}, - {NULL, 0, (void *)16, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_3}, - {NULL, 0, (void *)16, 4, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_3}, + {NULL, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_END_OF_FILE, {1, 1, 1, 1}}, + {NULL, 4, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_2, {1, 1, 1, 1}}, + {(void *)16, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_1, {1, 1, 1, 0}}, + {(void *)16, 4, NULL, 0, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_1, {0, 0, 0, 1}}, + {NULL, 0, NULL, 4, STATUS_END_OF_FILE, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_4, {1, 1, 1, 1}}, + {NULL, 0, (void *)16, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_3, {1, 1, 1, 1}}, + {NULL, 0, (void *)16, 4, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_3, {1, 1, 1, 1}}, }; for (j = 0; j < ARRAY_SIZE(param_tests); ++j) @@ -1179,15 +1180,19 @@ static void test_ioctl_buffers(void) status = NtDeviceIoControlFile(device, NULL, NULL, NULL, &io, IOCTL_WINETEST_EMPTY_BUFFERED, param_tests[j].in, param_tests[j].in_size, param_tests[j].out, param_tests[j].out_size); + todo_wine_if(param_tests[j].todowine[0]) ok(status == param_tests[j].buffered_status, "got %#lx\n", status); status = NtDeviceIoControlFile(device, NULL, NULL, NULL, &io, IOCTL_WINETEST_EMPTY_IN_DIRECT, param_tests[j].in, param_tests[j].in_size, param_tests[j].out, param_tests[j].out_size); + todo_wine_if(param_tests[j].todowine[1]) ok(status == param_tests[j].direct_status, "got %#lx\n", status); status = NtDeviceIoControlFile(device, NULL, NULL, NULL, &io, IOCTL_WINETEST_EMPTY_OUT_DIRECT, param_tests[j].in, param_tests[j].in_size, param_tests[j].out, param_tests[j].out_size); + todo_wine_if(param_tests[j].todowine[2]) ok(status == param_tests[j].direct_status, "got %#lx\n", status); status = NtDeviceIoControlFile(device, NULL, NULL, NULL, &io, IOCTL_WINETEST_EMPTY_NEITHER, param_tests[j].in, param_tests[j].in_size, param_tests[j].out, param_tests[j].out_size); + todo_wine_if(param_tests[j].todowine[3]) ok(status == param_tests[j].neither_status, "got %#lx\n", status); winetest_pop_context(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3007
From: Bernhard Übelacker <bernhardu(a)mailbox.org> --- dlls/ntoskrnl.exe/ntoskrnl.c | 5 +++++ dlls/ntoskrnl.exe/tests/ntoskrnl.c | 6 +++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c index 7e3ad603549..49220dd118b 100644 --- a/dlls/ntoskrnl.exe/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/ntoskrnl.c @@ -994,6 +994,11 @@ NTSTATUS CDECL wine_ntoskrnl_main_loop( HANDLE stop_event ) context.handle = wine_server_ptr_handle( reply->next ); context.params = reply->params; context.in_size = reply->in_size; + if (!context.in_size) + { + HeapFree( GetProcessHeap(), 0, context.in_buff ); + context.in_buff = NULL; + } client_tid = reply->client_tid; NtCurrentTeb()->Instrumentation[1] = wine_server_get_ptr( reply->client_thread ); } diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index c1db4a6b650..a2b3c508cd9 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1165,12 +1165,12 @@ static void test_ioctl_buffers(void) } param_tests[] = { - {NULL, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_END_OF_FILE, {1, 1, 1, 1}}, + {NULL, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_END_OF_FILE, {0, 1, 1, 0}}, {NULL, 4, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_2, {1, 1, 1, 1}}, - {(void *)16, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_1, {1, 1, 1, 0}}, + {(void *)16, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_1, {0, 1, 1, 1}}, {(void *)16, 4, NULL, 0, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_1, {0, 0, 0, 1}}, {NULL, 0, NULL, 4, STATUS_END_OF_FILE, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_4, {1, 1, 1, 1}}, - {NULL, 0, (void *)16, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_3, {1, 1, 1, 1}}, + {NULL, 0, (void *)16, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_3, {0, 1, 1, 1}}, {NULL, 0, (void *)16, 4, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_3, {1, 1, 1, 1}}, }; -- GitLab 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/ntdll/unix/file.c | 14 ++++++++++++++ dlls/ntoskrnl.exe/tests/ntoskrnl.c | 12 ++++++------ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index eef21e12ceb..6e782af1238 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -5965,6 +5965,7 @@ NTSTATUS WINAPI NtDeviceIoControlFile( HANDLE handle, HANDLE event, PIO_APC_ROUT void *out_buffer, ULONG out_size ) { ULONG device = (code >> 16); + ULONG method = (code & 3); NTSTATUS status = STATUS_NOT_SUPPORTED; TRACE( "(%p,%p,%p,%p,%p,0x%08x,%p,0x%08x,%p,0x%08x)\n", @@ -5976,6 +5977,19 @@ NTSTATUS WINAPI NtDeviceIoControlFile( HANDLE handle, HANDLE event, PIO_APC_ROUT if (HandleToLong( handle ) == ~0) return STATUS_INVALID_HANDLE; + switch (method) + { + case METHOD_BUFFERED: + case METHOD_IN_DIRECT: + case METHOD_OUT_DIRECT: + if (in_buffer && in_size && !virtual_check_buffer_for_read(in_buffer, in_size)) return STATUS_ACCESS_VIOLATION; + if ((out_buffer || method != METHOD_BUFFERED) && out_size && !virtual_check_buffer_for_write(out_buffer, out_size)) + return STATUS_ACCESS_VIOLATION; + if (!in_buffer && in_size) in_size = 0; + if (!out_buffer && out_size) out_size = 0; + break; + } + switch (device) { case FILE_DEVICE_BEEP: diff --git a/dlls/ntoskrnl.exe/tests/ntoskrnl.c b/dlls/ntoskrnl.exe/tests/ntoskrnl.c index a2b3c508cd9..76f64c3f96f 100644 --- a/dlls/ntoskrnl.exe/tests/ntoskrnl.c +++ b/dlls/ntoskrnl.exe/tests/ntoskrnl.c @@ -1165,13 +1165,13 @@ static void test_ioctl_buffers(void) } param_tests[] = { - {NULL, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_END_OF_FILE, {0, 1, 1, 0}}, - {NULL, 4, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_2, {1, 1, 1, 1}}, - {(void *)16, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_1, {0, 1, 1, 1}}, + {NULL, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_END_OF_FILE, {0, 0, 0, 0}}, + {NULL, 4, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_2, {0, 0, 0, 1}}, + {(void *)16, 0, NULL, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_1, {0, 0, 0, 1}}, {(void *)16, 4, NULL, 0, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_1, {0, 0, 0, 1}}, - {NULL, 0, NULL, 4, STATUS_END_OF_FILE, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_4, {1, 1, 1, 1}}, - {NULL, 0, (void *)16, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_3, {0, 1, 1, 1}}, - {NULL, 0, (void *)16, 4, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_3, {1, 1, 1, 1}}, + {NULL, 0, NULL, 4, STATUS_END_OF_FILE, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_4, {0, 0, 0, 1}}, + {NULL, 0, (void *)16, 0, STATUS_END_OF_FILE, STATUS_END_OF_FILE, STATUS_INVALID_PARAMETER_3, {0, 0, 0, 1}}, + {NULL, 0, (void *)16, 4, STATUS_ACCESS_VIOLATION, STATUS_ACCESS_VIOLATION, STATUS_INVALID_PARAMETER_3, {0, 0, 0, 1}}, }; for (j = 0; j < ARRAY_SIZE(param_tests); ++j) -- GitLab 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 | 84 ++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/dlls/kernel32/tests/volume.c b/dlls/kernel32/tests/volume.c index 5d2d971232f..92b7982f639 100644 --- a/dlls/kernel32/tests/volume.c +++ b/dlls/kernel32/tests/volume.c @@ -659,6 +659,89 @@ static void test_disk_query_property(void) CloseHandle(handle); } +static void test_disk_get_device_number(void) +{ + STORAGE_DEVICE_NUMBER device_number = {0}; + HANDLE handle; + DWORD error; + DWORD size; + BOOL ret; + IO_STATUS_BLOCK io; + NTSTATUS status; + + handle = CreateFileA("\\\\.\\PhysicalDrive0", GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, + 0, 0); + if (handle == INVALID_HANDLE_VALUE) + { + win_skip("can't open \\\\.\\PhysicalDrive0 %#lx\n", GetLastError()); + return; + } + + /* DeviceIoControl */ + 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); + + 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); + + SetLastError(0xdeadbeef); + ret = DeviceIoControl(handle, IOCTL_STORAGE_GET_DEVICE_NUMBER, (LPVOID)0xdeadbeef, 4, &device_number, sizeof(device_number), &size, NULL); + error = GetLastError(); + ok(!ret, "expect ret %#x, got %#x\n", FALSE, ret); + ok(error == ERROR_NOACCESS, "expect err %#x, got err %#lx\n", 0xdeadbeef, error); + ok(size == sizeof(device_number), "got size %ld\n", size); + + SetLastError(0xdeadbeef); + ret = DeviceIoControl(handle, IOCTL_STORAGE_GET_DEVICE_NUMBER, &device_number, sizeof(device_number), &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); + + /* NtDeviceIoControlFile */ + io.Status = 0xdeadf00d; + io.Information = 0xdeadf00d; + status = NtDeviceIoControlFile(handle, NULL, NULL, NULL, &io, + IOCTL_STORAGE_GET_DEVICE_NUMBER, NULL, 0, &device_number, sizeof(device_number)); + ok(status == STATUS_SUCCESS, "got %#lx\n", status); + ok(io.Status == STATUS_SUCCESS, "got status %#lx\n", io.Status); + ok(io.Information == sizeof(device_number), "got information %#Ix\n", io.Information); + + io.Status = 0xdeadf00d; + io.Information = 0xdeadf00d; + status = NtDeviceIoControlFile(handle, NULL, NULL, NULL, &io, + IOCTL_STORAGE_GET_DEVICE_NUMBER, NULL, 4, &device_number, sizeof(device_number)); + ok(status == STATUS_SUCCESS, "got %#lx\n", status); + ok(io.Status == STATUS_SUCCESS, "got status %#lx\n", io.Status); + ok(io.Information == sizeof(device_number), "got information %#Ix\n", io.Information); + + io.Status = 0xdeadf00d; + io.Information = 0xdeadf00d; + status = NtDeviceIoControlFile(handle, NULL, NULL, NULL, &io, + IOCTL_STORAGE_GET_DEVICE_NUMBER, (LPVOID)0xdeadbeef, 4, &device_number, sizeof(device_number)); + ok(status == STATUS_ACCESS_VIOLATION, "got %#lx\n", status); + ok(io.Status == 0xdeadf00d, "got status %#lx\n", io.Status); + ok(io.Information == 0xdeadf00d, "got information %#Ix\n", io.Information); + + io.Status = 0xdeadf00d; + io.Information = 0xdeadf00d; + status = NtDeviceIoControlFile(handle, NULL, NULL, NULL, &io, + IOCTL_STORAGE_GET_DEVICE_NUMBER, &device_number, sizeof(device_number), &device_number, sizeof(device_number)); + ok(status == STATUS_SUCCESS, "got %#lx\n", status); + ok(io.Status == STATUS_SUCCESS, "got status %#lx\n", io.Status); + ok(io.Information == sizeof(device_number), "got information %#Ix\n", io.Information); + + CloseHandle(handle); +} + static void test_GetVolumePathNameA(void) { char volume_path[MAX_PATH], cwd[MAX_PATH], expect_path[MAX_PATH]; @@ -1731,6 +1814,7 @@ START_TEST(volume) test_enum_vols(); test_disk_extents(); test_disk_query_property(); + test_disk_get_device_number(); test_GetVolumePathNamesForVolumeNameA(); test_GetVolumePathNamesForVolumeNameW(); test_cdrom_ioctl(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3007
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=134256 Your paranoid android. === debian11 (32 bit report) === ws2_32: sock.c:1490: Test failed: i 2, level 65535, optname 128: Unexpected setsockopt result -1. sock.c:1492: Test failed: i 2, level 65535, optname 128: Unexpected WSAGetLastError() 10014.
On Wed Jun 28 09:46:40 2023 +0000, Zebediah Figura wrote:
I took the liberty of writing some ntoskrnl-based tests: <https://testbot.winehq.org/JobDetails.pl?Key=133935> The conclusions that one can apparently draw are: * Buffered ioctls pass through zero, for both the pointer and size, if either the pointer or the size is zero. This basically contradicts Alexandre's original feedback—at least for buffered ioctls, that really is what's supposed to happen. * Buffered ioctls return STATUS_ACCESS_VIOLATION if the buffer pointer and size are nonzero but invalid. This is done somewhere in the NT kernel. * Direct ioctls behave similarly, with the exception that a NULL output buffer is *not* treated as not-specified. * Neither ioctls simply pass through their parameters without modification. In order to exactly emulate this behaviour for NEITHER ioctls, we will need extra flags (in order to signal to the Unix side whether the ioctl was originally NULL or not). On the other hand, NEITHER ioctls are kind of generally tetchy. I suspect that the right thing to do is something close to the original patch, possibly limited to buffered ioctls. Thank you very much for spending your time. I would not have gotten the idea to look at this interface from the driver side or defining own ioctls.
I added at least temporary your test here as a draft. Your test contains assignments for the direct ioctls (8833c709). Are these intentional or should they get removed? Basically with e93bf711 and 9c0974ce the buffered and direct tests would succeed. Like you wrote to support the neither ioctls it would take more. Should I still limit this MR to just the buffered ioctls? And should your test go into wine? If yes how can I properly add your authorship? Is then the specific IOCTL_STORAGE_GET_DEVICE_NUMBER test still needed? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3007#note_37097
Thank you very much for spending your time. I would not have gotten the idea to look at this interface from the driver side or defining own ioctls.
No problem, I was curious :-)
Your test contains assignments for the direct ioctls (8833c709). Are these intentional or should they get removed?
IIRC those were intentional. As you've noticed I didn't quite fully clean up the tests. I think what I encountered was that if the user address was NULL, then the system would actually pass a NULL pointer instead of a real MDL (to a NULL pointer). It does make the names "input_buffer" / "output_buffer" slightly misleading, though.
Basically with e93bf711 and 9c0974ce the buffered and direct tests would succeed. Like you wrote to support the neither ioctls it would take more.
Should I still limit this MR to just the buffered ioctls?
Since the direct tests look easy that seems reasonable to include here.
And should your test go into wine? If yes how can I properly add your authorship? Is then the specific IOCTL_STORAGE_GET_DEVICE_NUMBER test still needed?
Yes, probably, though as mentioned it may need a bit of cleaning up. You can probably fix the authorship by amending it with something like git commit --amend --no-edit --author="Zebediah Figura <zfigura(a)codeweavers.com>" The tests I wrote are comprehensive, but it does have the disadvantage that it will only run on the testbot machines configured to support ntoskrnl tests, so keeping the other tests may be a good idea. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3007#note_37243
participants (3)
-
Bernhard Übelacker -
Marvin -
Zebediah Figura (＠zfigura)