[PATCH] hidclass: All reports read or written to user space lead with a reportId
Aric Stewart
aric at codeweavers.com
Mon Nov 14 08:24:29 CST 2016
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?) 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.
Ah I miss remembered and when I looked it up for reading I saw what was expected and forgot to re-check for writing.
That does complicate things a bit because this is an area where IOHID and HIDRAW are now different. so the logic will need to get pushed down to the platform.
-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