[PATCH] hidclass.sys: fixed size passed IOCTL_HID_GET_INPUT_REPORT (Coverity)

Aric Stewart aric at codeweavers.com
Mon Oct 19 10:23:50 CDT 2015



On 10/19/15 10:21 AM, Sebastian Lackner wrote:
> On 19.10.2015 17:20, Aric Stewart wrote:
>> On 10/19/15 9:08 AM, Sebastian Lackner wrote:
>>> On 19.10.2015 15:04, Aric Stewart wrote:
>>>> On 10/17/15 8:34 AM, Marcus Meissner wrote:
>>>>> On Sat, Oct 17, 2015 at 03:31:24PM +0200, Sebastian Lackner wrote:
>>>>>> On 17.10.2015 14:52, Marcus Meissner wrote:
>>>>>>> 1327477 Wrong sizeof argument
>>>>>>>
>>>>>>> Signed-off-by: Marcus Meissner <marcus at jet.franken.de>
>>>>>>> ---
>>>>>>>     dlls/hidclass.sys/device.c | 2 +-
>>>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
>>>>>>> index e7e7e11..dcc7d3c 100644
>>>>>>> --- a/dlls/hidclass.sys/device.c
>>>>>>> +++ b/dlls/hidclass.sys/device.c
>>>>>>> @@ -277,7 +277,7 @@ static DWORD CALLBACK hid_device_thread(void *args)
>>>>>>>                 packet->reportId = 0;
>>>>>>>     
>>>>>>>                 irp = IoBuildDeviceIoControlRequest(IOCTL_HID_GET_INPUT_REPORT,
>>>>>>> -                device, NULL, 0, packet, sizeof(packet), TRUE, events[0],
>>>>>>> +                device, NULL, 0, packet, sizeof(*packet)+ext->preparseData->caps.InputReportByteLength, TRUE, events[0],
>>>>>>>                     &irp_status);
>>>>>>>     
>>>>>>>                 irpsp = IoGetNextIrpStackLocation(irp);
>>>>>>>
>>>>>>
>>>>>> This looks wrong, you have to allocate a separate buffer, and then copy it (like in the code below).
>>>>>> I don't know why some of these buffers are HEAP_ZERO_MEMORY though, and others not? Adding Aric, he
>>>>>> might want to review this part again, especially since Coverity detected more issues in this code
>>>>>> (unnecessary assignment of "rc" for example).
>>>>>
>>>>> something seems wrong though here, yes.
>>>>>
>>>>> rc issue is "CID 1327478 Unused value"
>>>>>
>>>>> Ciao, Marcus
>>>>>
>>>>
>>>> What is happening here is that the HID_XFER_PACKET is not a variable length structure, it takes a pointer to the buffer.  I am taking a bit of a shortcut and instead of allocating the HID_XFER_PACKET structure and a separate buffer, I am allocating 1 block of memory that is both, the buffer just comes after the HID_XFER_PACKET structure.
>>>>
>>>> I am pretty sure Marcus' original patch is unnecessary.  I am not surprised that there are issues in this code area though. It when through a lot of revision and tweaking as I was getting things working and that often results in cruft.
>>>>
>>>> -aric
>>>>
>>>
>>> I still don't think its correct. The IOCTL_HID_GET_INPUT_REPORT expects a raw data buffer,
>>> without any header in front of it, right? Then you would have to pass packet->reportBuffer
>>> (and the correct size) to IoBuildDeviceIoControlRequest, but that doesn't work because
>>> the buffer will be deallocated automatically after the request is done.
>>>
>>
>>
>> The confusion here, and I remember it well, is that IOCTL_HID_GET_INPUT_REPORT is used twice, one from user->hidclass IOCTL and again from hidclass->minidriver IOCTL, and the IOCTL parameters are different depending on usage.
>>
>> This case is the minidriver IOCTL: https://msdn.microsoft.com/en-us/library/windows/hardware/ff541126%28v=vs.85%29.aspx
>>
>> The minidriver IOCTL uses the HID_XFER_PACKET structure.
>>
>> -aric
>>
> 
> But Henris comment still applies, you want to pass the size of the struct, not the size of a pointer.
> 

That is correct. I am working on a patch for that, and the unused assignment. Had an e-mail in composition to that regard, but multitasking pulled me away. :)

-aric



More information about the wine-devel mailing list