Report ID get overwritten when using hid_get_input_report on Linux

[acolombier]

When using hid_get_input_report on Linux, the first byte of the buffer containing the report ID gets overwritten by the report first byte of data while it should not (spec)

Here is a simple way to reproduce the bug with a uhid virtual device

test_dev.c

#include <fcntl.h>
#include <poll.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <stdint.h>

#include <sys/ioctl.h>
#include <linux/uhid.h>
#include <errno.h>


static volatile uint32_t done;

static unsigned char rdesc[] = {
	0x06, 0x02, 0xFF,               /*  Usage Page (FF02h),                     */
	0x09, 0x00,                     /*  Usage (00h),                            */
	0xA1, 0x01,                     /*  Collection (Application),               */
	0x09, 0x01,                     /*      Usage (01h),                        */
	0xA1, 0x02,                     /*      Collection (Logical),               */
	0x85, 0x01,                     /*          Report ID (1),                  */
	0x09, 0x02,                     /*          Usage (02h),                    */
	0x15, 0x00,                     /*          Logical Minimum (0),            */
	0x25, 0x01,                     /*          Logical Maximum (1),            */
	0x75, 0x01,                     /*          Report Size (1),                */
	0x95, 0x88,                     /*          Report Count (136),             */
	0x81, 0x02,                     /*          Input (Variable),               */
	0x09, 0x07,                     /*          Usage (07h),                    */
	0x15, 0x00,                     /*          Logical Minimum (0),            */
	0x25, 0x01,                     /*          Logical Maximum (1),            */
	0x75, 0x01,                     /*          Report Size (1),                */
	0x95, 0x10,                     /*          Report Count (16),              */
	0x81, 0x02,                     /*          Input (Variable),               */
	0x09, 0x03,                     /*          Usage (03h),                    */
	0x15, 0x00,                     /*          Logical Minimum (0),            */
	0x25, 0x0F,                     /*          Logical Maximum (15),           */
	0x75, 0x04,                     /*          Report Size (4),                */
	0x95, 0x06,                     /*          Report Count (6),               */
	0x81, 0x02,                     /*          Input (Variable),               */
	0xC0,                           /*      End Collection,                     */
	0xC0                            /*  End Collection                          */
};

void sighndlr(int signal)
{
	done = 1;
	printf("\n");
}

static int uhid_write(int fd, const struct uhid_event *ev)
{
	ssize_t ret;

	ret = write(fd, ev, sizeof(*ev));
	if (ret < 0) {
		fprintf(stderr, "Cannot write to uhid: %m\n");
		return -errno;
	} else if (ret != sizeof(*ev)) {
		fprintf(stderr, "Wrong size written to uhid: %zd != %zu\n",
			ret, sizeof(ev));
		return -EFAULT;
	} else {
		return 0;
	}
}

static int create(int fd)
{
	struct uhid_event ev;

	memset(&ev, 0, sizeof(ev));
	ev.type = UHID_CREATE;
	strcpy((char*)ev.u.create.name, "test-uhid-device");
	ev.u.create.rd_data = rdesc;
	ev.u.create.rd_size = sizeof(rdesc);
	ev.u.create.bus = BUS_USB;
	ev.u.create.vendor = 0xDEAD;
	ev.u.create.product = 0xBEAF;
	ev.u.create.version = 0;
	ev.u.create.country = 0;

	return uhid_write(fd, &ev);
}

static void destroy(int fd)
{
	struct uhid_event ev;

	memset(&ev, 0, sizeof(ev));
	ev.type = UHID_DESTROY;

	uhid_write(fd, &ev);
}

static void handle_report(struct uhid_event *ev, int fd)
{
	if (ev->u.get_report.rtype != UHID_START)
		return;

	struct uhid_event rep;

	memset(&rep, 0, sizeof(rep));
	rep.type = UHID_GET_REPORT_REPLY;

	rep.u.get_report_reply.id = ev->u.get_report.id;
	rep.u.get_report_reply.err = 0;
	rep.u.get_report_reply.size = 16;
	memset(rep.u.get_report_reply.data, 55, UHID_DATA_MAX);

	uhid_write(fd, &rep);
	return;
}

static int event(int fd)
{
	struct uhid_event ev;
	ssize_t ret;

	memset(&ev, 0, sizeof(ev));
	ret = read(fd, &ev, sizeof(ev));
	if (ret == 0) {
		fprintf(stderr, "Read HUP on uhid-cdev\n");
		return -EFAULT;
	} else if (ret < 0) {
		fprintf(stderr, "Cannot read uhid-cdev: %m\n");
		return -errno;
	} else if (ret != sizeof(ev)) {
		fprintf(stderr, "Invalid size read from uhid-dev: %zd != %zu\n",
			ret, sizeof(ev));
		return -EFAULT;
	}

	switch (ev.type) {
	case UHID_START:
		break;
	case UHID_STOP:
		break;
	case UHID_OPEN:
		break;
	case UHID_CLOSE:
	case UHID_OUTPUT:
		break;
	case UHID_OUTPUT_EV:
		break;
	case UHID_GET_REPORT:
		handle_report(&ev, fd);
		break;
	default:
		fprintf(stderr, "Invalid event from uhid-dev: %u\n", ev.type);
	}

	return 0;
}

int main(int argc, char **argv)
{
	signal(SIGINT, sighndlr);

	// UHID
	int fd = open("/dev/uhid", O_RDWR);
    if (fd < 0) {
        perror("open");
        return -1;
    }

   	fprintf(stderr, "Create uhid device\n");
	if (create(fd)) {
		close(fd);
		return EXIT_FAILURE;
	}

	struct pollfd pfds;
	pfds.fd = fd;
	pfds.events = POLLIN;

	while(!done) {
		int ret = poll(&pfds, 1, 10);
		if (ret < 0) {
			fprintf(stderr, "Cannot poll for fds: %m\n");
			break;
		}
		if (pfds.revents & POLLHUP) {
			fprintf(stderr, "Received HUP on uhid-cdev\n");
			break;
		}
		if (pfds.revents & POLLIN) {
			ret = event(fd);
			if (ret)
				break;
		}
	}

	destroy(fd);

	return 0;
}

test_get_report.c

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <hidapi/hidapi.h>

#define VENDOR_ID 0xdead
#define PRODUCT_ID 0xbeaf

int main()
{
    int res;
    unsigned char buf[255];
    hid_device *handle;

    res = hid_init();
    if (res != 0) {
        printf("hid_init failed: %ls\n", hid_error(NULL));
        return -1;
    }

    handle = hid_open(VENDOR_ID, PRODUCT_ID, NULL);
    if (!handle) {
        printf("Failed to open UHID device: %ls\n", hid_error(NULL));
        return -1;
    }


    buf[0] = 42;
    int bytesRead = hid_get_input_report(handle, buf, 16);

	fprintf(stderr, "Report received for 42\n");
	for (int i = 0; i < 16; i++)
	{
		printf("%d ", buf[i]);
	}
    printf("\n");

    hid_close(handle);
    hid_exit();

    return 0;
}

With the two previous files in the current directory, run the following commands

sudo modprobe uhid
gcc test_dev.c -lhidapi-hidraw -o testdev && sudo ./testdev &
gcc test_get_report.c -lhidapi-hidraw -o testhid_report && sudo ./testhid_report

Expected output

Report received for 42
42 55 55 55 55 55 55 55 55 55 55 55 55 55 55 55 

Actual output

Report received for 42
55 55 55 55 55 55 55 55 55 55 55 55 55 55 55 55 

Version

libhidapi-hidraw: 0.11.2
Linux Kernel: 6.1.11

I'd be happy to help addressing the bug once I get confirmation on it. At first glance, it feels like the fix may have to go in the kernel driver (drivers/hid/hid-core.c or drivers/hid/hidraw.c) as I cannot see any ways that would allow to make a bug fix without a memmove on the library side. But since the name of Alan Ott is mentioned at a few places on the kernel sources/docs, I thought I would raise the bug first before escalating it :)

[acolombier]

Update on this - it also affects the kernel 6.2.0

I have managed to fix the issue by using this patch on the kernel:

diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 197b1e7bf029..2c12f25817e6 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -231,9 +231,10 @@ static ssize_t hidraw_get_report(struct file *file, char __user *buffer, size_t
 	if (ret < 0)
 		goto out_free;
 
+	count--;
 	len = (ret < count) ? ret : count;
 
-	if (copy_to_user(buffer, buf, len)) {
+	if (copy_to_user(buffer + 1, buf, len)) {
 		ret = -EFAULT;
 		goto out_free;
 	}

I'm wondering if this could be a breaking change for some people. It'd be great to get some feedback, before I got ahead a submit if upstream.

Also, looking at core implementation (used by libhidapi-libusb I believe), the bug doesn't seem to affect it.

[Youw]

Unfortunately I'm not familiar with uhid much, but my first though is that the problem is inside handle_report implementation.
I went thru some kernel sources briefly, and it seem to me that get_report_reply.data should contain the report ID as a first byte, despite that fact that it also present as get_report_reply.id member.

I can't find the uhid documentation to support that statement, but my reasoning is simple - there is not even a mention of a special handling of the get_report_reply.id or a first byte of the get_report_reply.data inside uhid implementation, and the change that you propose - it affects all kind of HID devices, not only the UHID ones, and I haven't heard similar issue reports for any other (e.g. USB/HID) hidraw device kinds.

[acolombier]

Thanks for your reply @Youw

According to this doc, get_report_reply.id isn't the report ID, but instead a unique identifier that represent the request and should be used when replying with UHID_GET_REPORT_REPLY. The actual report ID is stored in get_report_reply.rnum and shouldn't be forwarded back to the user.

I should mention that I have only implemented that dummy UHID device to prove the bug and help anyone with an easy way to reproduce the problem locally, without needing to mess up with a real HID ready device; that bug happens with an actual HID device. (Which I'm waiting to get my hands on again to test if that module patch solves the issue, hopefully in a few hours)

IMHO, the bug seems to be quite clear, looking at that function and docstring, the first byte shouldn't be overwritten.

[Youw]

If you have a real HID device - it is even easier to test: try the same HIDAPI code you have, but link agains hidapi-libusb backend (assuming your HID device is a USB/HID device).

That should tell you if the problem is the kernel/hidraw or the aproach you take. We consider libusb backend of HIDAPI is one of the most tested implementation of client-side USB/HID protocol.

the first byte shouldn't be overwritten

You're missing one point: "the first byte of the report is the report ID" - that is not only a driver/function description; that is also part of the HID specification. And the device, as an implementation of HID protocol, is responsible to make sure that the first byte is set to report ID for "GET Report" request - not the driver.

So, technicall, the first byte does gets overwritten in the driver implementation by the data returned by the device, but normally it matches the Report ID from the request, as the device has to copy it.

[Youw]

Also, if you have a real device, I suggest trying to compare the behavior on Windows and macOS - those are independent backend implementations.

[acolombier]

Apologies, it looks like I got carried away. This is not a bug at all. It looks like there was a genuine decision to obfuscate the Report ID in the project I'm working on, nothing wrong on hidapi. Sorry about the noise :)

[mcuee]

Interesting that @acolombier proposed a patch to linux-input subsystem here. But the patch is not accepted for backwards compatability reason.
https://www.spinics.net/lists/linux-input/msg84368.html