[PATCH] hidclass: All reports read or written to user space lead with a reportId
Sebastian Lackner
sebastian at fds-team.de
Mon Nov 14 08:17:09 CST 2016
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.
> + }
> + 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