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.
-- v4: ntdll: Avoid requests with null pointer but invalid size.
From: Bernhard Übelacker bernhardu@mailbox.org
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51770 --- dlls/kernel32/tests/volume.c | 117 +++++++++++++++++++++++++++++++++++ dlls/ntdll/unix/file.c | 14 +++++ 2 files changed, 131 insertions(+)
diff --git a/dlls/kernel32/tests/volume.c b/dlls/kernel32/tests/volume.c index 5d2d971232f..3283c680897 100644 --- a/dlls/kernel32/tests/volume.c +++ b/dlls/kernel32/tests/volume.c @@ -659,6 +659,122 @@ 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); + + /* NtFsControlFile */ + io.Status = 0xdeadf00d; + io.Information = 0xdeadf00d; + status = NtFsControlFile(handle, NULL, NULL, NULL, &io, + IOCTL_STORAGE_GET_DEVICE_NUMBER, NULL, 0, &device_number, sizeof(device_number)); + ok(status == STATUS_INVALID_PARAMETER, "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 = NtFsControlFile(handle, NULL, NULL, NULL, &io, + IOCTL_STORAGE_GET_DEVICE_NUMBER, NULL, 4, &device_number, sizeof(device_number)); + ok(status == STATUS_INVALID_PARAMETER, "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 = NtFsControlFile(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 = NtFsControlFile(handle, NULL, NULL, NULL, &io, + IOCTL_STORAGE_GET_DEVICE_NUMBER, &device_number, sizeof(device_number), &device_number, sizeof(device_number)); + ok(status == STATUS_INVALID_PARAMETER, "got %#lx\n", status); + ok(io.Status == 0xdeadf00d, "got status %#lx\n", io.Status); + ok(io.Information == 0xdeadf00d, "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 +1847,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/dlls/ntdll/unix/file.c b/dlls/ntdll/unix/file.c index ff1618f31fd..aae76df16ac 100644 --- a/dlls/ntdll/unix/file.c +++ b/dlls/ntdll/unix/file.c @@ -5956,6 +5956,15 @@ NTSTATUS WINAPI NtDeviceIoControlFile( HANDLE handle, HANDLE event, PIO_APC_ROUT if (HandleToLong( handle ) == ~0) return STATUS_INVALID_HANDLE;
+ if (code == IOCTL_STORAGE_GET_DEVICE_NUMBER) { + if (!in_buffer) { + in_size = 0; + } else { + if (!virtual_check_buffer_for_read( in_buffer, in_size )) + return STATUS_ACCESS_VIOLATION; + } + } + switch (device) { case FILE_DEVICE_BEEP: @@ -6033,6 +6042,11 @@ NTSTATUS WINAPI NtFsControlFile( HANDLE handle, HANDLE event, PIO_APC_ROUTINE ap
switch (code) { + case IOCTL_STORAGE_GET_DEVICE_NUMBER: + if (in_buffer && !virtual_check_buffer_for_read( in_buffer, in_size )) + return STATUS_ACCESS_VIOLATION; + return STATUS_INVALID_PARAMETER; + case FSCTL_DISMOUNT_VOLUME: status = server_ioctl_file( handle, event, apc, apc_context, io, code, in_buffer, in_size, out_buffer, out_size );
On Thu Jun 8 16:32:57 2023 +0000, Zebediah Figura wrote:
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.
Thank you for the very detailed explanation, I did not realize that NtDeviceIoControlFile is a "public" API and can also be simply used in tests.
I added more tests. These show DeviceIoControl and NtDeviceIoControlFile behave equally. Therefore I assume the check should take place in NtDeviceIoControlFile.
The NtFsControlFile tests do all fail, so I assume that it isn't supposed to handle this ioctl at all. So I guess I can drop tests for NtFsControlFile?
If the modified function is inside ntdll should then the test be also moved into ntdll?
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.
-
Bernhard Übelacker -
Bernhard Übelacker (＠bernhardu)
-
Zebediah Figura (＠zfigura)