Re: [PATCH] hidclass: All reports read or written to user space lead with a reportId

[Aric Stewart]

On 11/14/16 9:00 AM, Sebastian Lackner wrote:
> 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
> > > >
> > > > 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).

Coming in from user space then yes. I am pretty sure that is how it works.
> > > 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.

Yeah, this is really hard to find conclusive documentation on. I will keep digging but MSDN does not explicitly say in the minidriver level what is going on.

Cracking open my books (virtually), I find in Programming the Microsoft Windows Driver Model (2nd Edition) the following paragraph:

http://flylib.com/books/en/4.168.1.84/1/

"When your driver supports feature reports, you’ll normally be using report identification bytes to identify the different feature reports and your input and output reports. In that case, the reportBuffer buffer begins with a single-byte identifier, which will equal the reportId value in the HID_XFER_PACKET structure—HIDCLASS makes that so. The count in reportBufferLen includes the identifier byte. When you don’t use report identifiers, however, reportId will be 0, reportBuffer won’t have room for an identifier byte, and the reportBufferLen count won’t include an identifier byte. This arrangement is true even though the caller of HidD_GetFeature or HidD_SetFeature supplies a buffer that does include a zero identification byte. To put it another way, you copy the actual feature report data beginning at reportBuffer + 1 if you’re using report identifiers but beginning at reportBuffer if you’re not using report identifiers."

So that implies that the minidriver level will behave as I have implemented, only having a reportId at the front of the packet buffer if reportId is not 0.

-aric
> > -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));