[PATCH] hidclass: All reports read or written to user space lead with a reportId
Sebastian Lackner
sebastian at fds-team.de
Mon Nov 14 09:00:02 CST 2016
On 14.11.2016 15:38, Aric Stewart wrote:
>
>
> On 11/14/16 8:17 AM, Sebastian Lackner wrote:
>> On 14.11.2016 14:35, Aric Stewart wrote:
>>> This is a little confusing because reports to and from the device
>>> itself only have a leading reportId if the reportid is not 0.
>>> Otherwise there is no leading reportId. So we need to add a 0 reportId
>>> to reports coming from such a device and strip the leading 0 from
>>> reports written to the device.
>>>
>>> Signed-off-by: Aric Stewart <aric at codeweavers.com>
>>> ---
>>> dlls/hidclass.sys/descriptor.c | 31 ++++-----------
>>> dlls/hidclass.sys/device.c | 90 ++++++++++++++++++++++++++++--------------
>>> 2 files changed, 67 insertions(+), 54 deletions(-)
>>>
>>>
>>>
>>> 0001-hidclass-All-reports-read-or-written-to-user-space-lea.txt
>>>
>>>
>>> diff --git a/dlls/hidclass.sys/descriptor.c b/dlls/hidclass.sys/descriptor.c
>>> index 9a26f0f4..8bcda43 100644
>>> --- a/dlls/hidclass.sys/descriptor.c
>>> +++ b/dlls/hidclass.sys/descriptor.c
>>> @@ -887,10 +887,8 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData(
>>> new_report(wine_report, input_features[0]);
>>> data->dwInputReportCount++;
>>>
>>> - if (input_features[0]->caps.ReportID != 0)
>>> - bitOffset = 8;
>>> - else
>>> - bitOffset = 0;
>>> + /* Room for the reportID */
>>> + bitOffset = 8;
>>>
>>> for (i = 0; i < i_count; i++)
>>> {
>>> @@ -901,10 +899,7 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData(
>>> new_report(wine_report, input_features[i]);
>>> data->dwInputReportCount++;
>>> bitLength = max(bitOffset, bitLength);
>>> - if (input_features[i]->caps.ReportID != 0)
>>> - bitOffset = 8;
>>> - else
>>> - bitOffset = 0;
>>> + bitOffset = 8;
>>> }
>>> build_elements(wine_report, input_features[i], &bitOffset);
>>> count_elements(input_features[i], &data->caps.NumberInputButtonCaps,
>>> @@ -922,10 +917,7 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData(
>>> data->dwOutputReportOffset = (BYTE*)wine_report - (BYTE*)data->InputReports;
>>> new_report(wine_report, output_features[0]);
>>> data->dwOutputReportCount++;
>>> - if (output_features[0]->caps.ReportID != 0)
>>> - bitOffset = 8;
>>> - else
>>> - bitOffset = 0;
>>> + bitOffset = 8;
>>>
>>> for (i = 0; i < o_count; i++)
>>> {
>>> @@ -936,10 +928,7 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData(
>>> new_report(wine_report, output_features[i]);
>>> data->dwOutputReportCount++;
>>> bitLength = max(bitOffset, bitLength);
>>> - if (output_features[0]->caps.ReportID != 0)
>>> - bitOffset = 8;
>>> - else
>>> - bitOffset = 0;
>>> + bitOffset = 8;
>>> }
>>> build_elements(wine_report, output_features[i], &bitOffset);
>>> count_elements(output_features[i], &data->caps.NumberOutputButtonCaps,
>>> @@ -957,10 +946,7 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData(
>>> data->dwFeatureReportOffset = (BYTE*)wine_report - (BYTE*)data->InputReports;
>>> new_report(wine_report, feature_features[0]);
>>> data->dwFeatureReportCount++;
>>> - if (feature_features[0]->caps.ReportID != 0)
>>> - bitOffset = 8;
>>> - else
>>> - bitOffset = 0;
>>> + bitOffset = 8;
>>>
>>> for (i = 0; i < f_count; i++)
>>> {
>>> @@ -971,10 +957,7 @@ static WINE_HIDP_PREPARSED_DATA* build_PreparseData(
>>> new_report(wine_report, feature_features[i]);
>>> data->dwFeatureReportCount++;
>>> bitLength = max(bitOffset, bitLength);
>>> - if (feature_features[0]->caps.ReportID != 0)
>>> - bitOffset = 8;
>>> - else
>>> - bitOffset = 0;
>>> + bitOffset = 8;
>>> }
>>> build_elements(wine_report, feature_features[i], &bitOffset);
>>> count_elements(feature_features[i], &data->caps.NumberFeatureButtonCaps,
>>> diff --git a/dlls/hidclass.sys/device.c b/dlls/hidclass.sys/device.c
>>> index 5dd5ebb..6454b67 100644
>>> --- a/dlls/hidclass.sys/device.c
>>> +++ b/dlls/hidclass.sys/device.c
>>> @@ -197,6 +197,28 @@ void HID_DeleteDevice(HID_MINIDRIVER_REGISTRATION *driver, DEVICE_OBJECT *device
>>> IoDeleteDevice(device);
>>> }
>>>
>>> +static NTSTATUS copy_packet_into_buffer(HID_XFER_PACKET *packet, BYTE* buffer, ULONG buffer_length, ULONG *out_length)
>>> +{
>>> + *out_length = 0;
>>> + if (buffer_length > packet->reportBufferLen)
>>> + {
>>> + if (packet->reportId != 0)
>>> + {
>>> + memcpy(buffer, packet->reportBuffer, packet->reportBufferLen);
>>> + *out_length = packet->reportBufferLen;
>>
>> The length check is a bit too strict in this case. You are returning STATUS_BUFFER_OVERFLOW
>> even when the buffer is sufficient (buffer_length == packet->reportBufferLen).
>>
>>> + }
>>> + else
>>> + {
>>> + buffer[0] = 0;
>>> + memcpy(&buffer[1], packet->reportBuffer, packet->reportBufferLen);
>>> + *out_length = packet->reportBufferLen + 1;
>>> + }
>>> + return STATUS_SUCCESS;
>>> + }
>>> + else
>>> + return STATUS_BUFFER_OVERFLOW;
>>> +}
>>> +
>>> static void HID_Device_processQueue(DEVICE_OBJECT *device)
>>> {
>>> LIST_ENTRY *entry;
>>> @@ -217,20 +239,14 @@ static void HID_Device_processQueue(DEVICE_OBJECT *device)
>>> RingBuffer_Read(ext->ring_buffer, ptr, packet, &buffer_size);
>>> if (buffer_size)
>>> {
>>> + NTSTATUS hr;
>>
>> Maybe its just me, but I think "hr" is really a very bad name for NTSTATUS codes. ;)
>>
>>> + ULONG out_length;
>>> IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation(irp);
>>> packet->reportBuffer = (BYTE *)packet + sizeof(*packet);
>>> TRACE_(hid_report)("Processing Request (%i)\n",ptr);
>>> - if (irpsp->Parameters.Read.Length >= packet->reportBufferLen)
>>> - {
>>> - memcpy(irp->AssociatedIrp.SystemBuffer, packet->reportBuffer, packet->reportBufferLen);
>>> - irp->IoStatus.Information = packet->reportBufferLen;
>>> - irp->IoStatus.u.Status = STATUS_SUCCESS;
>>> - }
>>> - else
>>> - {
>>> - irp->IoStatus.Information = 0;
>>> - irp->IoStatus.u.Status = STATUS_BUFFER_OVERFLOW;
>>> - }
>>> + hr = copy_packet_into_buffer(packet, irp->AssociatedIrp.SystemBuffer, irpsp->Parameters.Read.Length, &out_length);
>>> + irp->IoStatus.u.Status = hr;
>>> + irp->IoStatus.Information = out_length;
>>> }
>>> else
>>> {
>>> @@ -331,7 +347,10 @@ static DWORD CALLBACK hid_device_thread(void *args)
>>> if (!exit_now && irp->IoStatus.u.Status == STATUS_SUCCESS)
>>> {
>>> packet->reportBufferLen = irp->IoStatus.Information;
>>> - packet->reportId = packet->reportBuffer[0];
>>> + if (ext->preparseData->InputReports[0].reportID)
>>> + packet->reportId = packet->reportBuffer[0];
>>> + else
>>> + packet->reportId = 0;
>>> RingBuffer_Write(ext->ring_buffer, packet);
>>> HID_Device_processQueue(device);
>>> }
>>> @@ -461,9 +480,17 @@ static NTSTATUS HID_set_to_device(DEVICE_OBJECT *device, IRP *irp)
>>> NTSTATUS rc;
>>>
>>> TRACE_(hid_report)("Device %p Buffer length %i Buffer %p\n", device, irpsp->Parameters.DeviceIoControl.InputBufferLength, irp->AssociatedIrp.SystemBuffer);
>>> - packet.reportBuffer = irp->AssociatedIrp.SystemBuffer;
>>> - packet.reportId = ((char*)irp->AssociatedIrp.SystemBuffer)[0];
>>> - packet.reportBufferLen = irpsp->Parameters.DeviceIoControl.InputBufferLength;
>>> + packet.reportId = ((BYTE*)irp->AssociatedIrp.SystemBuffer)[0];
>>> + if (packet.reportId == 0)
>>> + {
>>> + packet.reportBuffer = &((BYTE*)irp->AssociatedIrp.SystemBuffer)[1];
>>> + packet.reportBufferLen = irpsp->Parameters.DeviceIoControl.InputBufferLength - 1;
>>> + }
>>> + else
>>> + {
>>> + packet.reportBuffer = irp->AssociatedIrp.SystemBuffer;
>>> + packet.reportBufferLen = irpsp->Parameters.DeviceIoControl.InputBufferLength;
>>> + }
>>> TRACE_(hid_report)("(id %i, len %i buffer %p)\n", packet.reportId, packet.reportBufferLen, packet.reportBuffer);
>>>
>>> rc = call_minidriver(irpsp->Parameters.DeviceIoControl.IoControlCode,
>>> @@ -636,20 +663,15 @@ NTSTATUS WINAPI HID_Device_read(DEVICE_OBJECT *device, IRP *irp)
>>> if (buffer_size)
>>> {
>>> IO_STACK_LOCATION *irpsp = IoGetCurrentIrpStackLocation( irp );
>>> + NTSTATUS hr;
>>
>> Same here.
>>
>>> + ULONG out_length;
>>> packet->reportBuffer = (BYTE *)packet + sizeof(*packet);
>>> TRACE_(hid_report)("Got Packet %p %i\n", packet->reportBuffer, packet->reportBufferLen);
>>> - if (irpsp->Parameters.Read.Length >= packet->reportBufferLen)
>>> - {
>>> - memcpy(irp->AssociatedIrp.SystemBuffer, packet->reportBuffer, packet->reportBufferLen);
>>> - irp->IoStatus.Information = packet->reportBufferLen;
>>> - irp->IoStatus.u.Status = STATUS_SUCCESS;
>>> - }
>>> - else
>>> - {
>>> - irp->IoStatus.Information = 0;
>>> - irp->IoStatus.u.Status = STATUS_BUFFER_OVERFLOW;
>>> - }
>>> - IoCompleteRequest( irp, IO_NO_INCREMENT );
>>> +
>>> + hr = copy_packet_into_buffer(packet, irp->AssociatedIrp.SystemBuffer, irpsp->Parameters.Read.Length, &out_length);
>>> + irp->IoStatus.Information = out_length;
>>> + irp->IoStatus.u.Status = hr;
>>> + IoCompleteRequest(irp, IO_NO_INCREMENT);
>>> }
>>> else
>>> {
>>> @@ -671,9 +693,17 @@ NTSTATUS WINAPI HID_Device_write(DEVICE_OBJECT *device, IRP *irp)
>>> irp->IoStatus.Information = 0;
>>>
>>> TRACE_(hid_report)("Device %p Buffer length %i Buffer %p\n", device, irpsp->Parameters.Write.Length, irp->AssociatedIrp.SystemBuffer);
>>> - packet.reportBuffer = irp->AssociatedIrp.SystemBuffer;
>>> - packet.reportId = ((char*)irp->AssociatedIrp.SystemBuffer)[0];
>>> - packet.reportBufferLen = irpsp->Parameters.Write.Length;
>>> + packet.reportId = ((BYTE*)irp->AssociatedIrp.SystemBuffer)[0];
>>> + if (packet.reportId == 0)
>>> + {
>>> + packet.reportBuffer = &((BYTE*)irp->AssociatedIrp.SystemBuffer)[1];
>>> + packet.reportBufferLen = irpsp->Parameters.Write.Length - 1;
>>
>> Besides the fact that the code looks suspicious (couldn't a 0 byte also have
>> a different meaning?)
>
> What part do you mean about a 0 byte having a different meaning. When coming in from the write (user space) the first byte will be the report id, so a 0 is a 0 reportId.
Unfortunately its a bit difficult to find documentation, so I am not sure if the 0 should always
be present, or if it depends on the device (like the hidraw read() command). If you are sure
it is present this should be fine (well, except that you would need a length check to avoid crashes
caused by empty messages).
>
>
>> this will probably break the udev backend. As documented
>> in https://www.kernel.org/doc/Documentation/hid/hidraw.txt, the write() command
>> expects that the first byte is 0 if the device does not use numbered reports.
>>
>
> Yeah, thinking about this I will fix this on the hidraw backend. I like having this level be consistent where the packet buffer only leads with the reportid if the reportid is non-zero. So adding that zero in on the hidraw platform backend is not hard.
Is that also how it is done on Windows? We have to keep in mind that Hidclass itself is not Wine
specific and also exists on Windows.
>
>
> -aric
>
>>> + }
>>> + else
>>> + {
>>> + packet.reportBuffer = irp->AssociatedIrp.SystemBuffer;
>>> + packet.reportBufferLen = irpsp->Parameters.Write.Length;
>>> + }
>>> TRACE_(hid_report)("(id %i, len %i buffer %p)\n", packet.reportId, packet.reportBufferLen, packet.reportBuffer);
>>>
>>> rc = call_minidriver(IOCTL_HID_WRITE_REPORT, device, NULL, 0, &packet, sizeof(packet));
>>>
>>>
>>>
>>
More information about the wine-devel
mailing list