[PATCH v3 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. - Modified format strings. -- v3: 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 | 35 +++++++++++++++++++++++++++++++++++ include/wine/server.h | 2 +- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/dlls/kernel32/tests/volume.c b/dlls/kernel32/tests/volume.c index 5d2d971232f..138b3af4dff 100644 --- a/dlls/kernel32/tests/volume.c +++ b/dlls/kernel32/tests/volume.c @@ -659,6 +659,40 @@ 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; + + 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; + } + + 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); +} + static void test_GetVolumePathNameA(void) { char volume_path[MAX_PATH], cwd[MAX_PATH], expect_path[MAX_PATH]; @@ -1731,6 +1765,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(); diff --git a/include/wine/server.h b/include/wine/server.h index 228682dc914..8e59ec95ca7 100644 --- a/include/wine/server.h +++ b/include/wine/server.h @@ -71,7 +71,7 @@ static inline data_size_t wine_server_reply_size( const void *reply ) static inline void wine_server_add_data( void *req_ptr, const void *ptr, data_size_t size ) { struct __server_request_info * const req = req_ptr; - if (size) + if (size && ptr != NULL) { req->data[req->data_count].ptr = ptr; req->data[req->data_count++].size = size; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3007
On Wed Jun 7 22:58:04 2023 +0000, Zebediah Figura wrote:
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 Thanks for your time.
If I understand this right in this case the "driver" is below `NtDeviceIoControlFile` and therefore the call into wineserver itself? I moved in v5 the check into wine_server_add_data - was this the version you are referring to? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3007#note_35103
On Thu Jun 8 08:40:42 2023 +0000, Bernhard Übelacker wrote:
Thanks for your time. If I understand this right in this case the "driver" is below `NtDeviceIoControlFile` and therefore the call into wineserver itself? I moved in v5 the check into wine_server_add_data - was this the version you are referring to? I'm sorry, but I think you're still misunderstanding.
Review comments like "this check doesn't look like it should be done here" are not directives to do the check somewhere else, they are a request to figure out whether the check *should* be done somewhere else, and then do so if and only if it actually is the right thing to do. In the case where the difference can be tested, that may mean actually writing those tests. In this specific case: according to my *faulty* memory, BUFFERED ioctls always have their buffers validated, possibly even regardless of whether they're used or not, and this makes sense, because in order to be buffered, they need to be copied from user space to a kernel space buffer even before reaching the the kernel driver. It also makes sense because IIRC the kernel driver is allowed to assume that InputBuffer [or whatever the field is called] points to valid memory. The fact that DeviceIoControl(handle, IOCTL_STORAGE_GET_DEVICE_NUMBER, NULL, 4, ...) apparently succeeds in your tests contradicts this. So, I would test: is this memory of mine correct in general? If so, what's the difference in this case? DeviceIoControl() versus NtDeviceIoControlFile()? The specific ioctl number [and are there any other ioctls that get similar treatment]? NtDeviceIoControlFile() versus NtFsControlFile()? Passing NULL instead of a non-NULL invalid pointer? Only by running tests can you figure out for yourself where the right place is to have these checks. And then, hopefully, keep whichever tests are necessary to prove the conclusion. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3007#note_35175
participants (2)
-
Bernhard Übelacker -
Zebediah Figura (＠zfigura)