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.