[PATCH] HID: hidraw: Keep the report ID on buffer in get_report

[PATCH] HID: hidraw: Keep the report ID on buffer in get_report

[Antoine C]

The ioctl syscall with arg HIDIOCGINPUT must not override
the report ID contained in the first byte of the buffer
and should offset the report data next to it.

Signed-off-by: Antoine Colombier <kernel@acolombier.dev>
---
Hello,

Apologies for the resend, I forgot to disable the HTML format on the
previous email. Please ignore the previous one.

This addresses the bug report in the hidapi: https://github.com/libusb/hidapi/issues/514
The patch was tested using the test snippets attached in the issue above
on 6.2.0-76060200-generic (PopOS 22.04)

 drivers/hid/hidraw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


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;
  }

Re: [PATCH] HID: hidraw: Keep the report ID on buffer in get_report

[Benjamin Tissoires]

On Thu, Mar 9, 2023 at 12:36 PM Antoine C <contact@antoinecolombier.fr> wrote:
>
> The ioctl syscall with arg HIDIOCGINPUT must not override
> the report ID contained in the first byte of the buffer
> and should offset the report data next to it.
>
> Signed-off-by: Antoine Colombier <kernel@acolombier.dev>
> ---
> Hello,
>
> Apologies for the resend, I forgot to disable the HTML format on the
> previous email. Please ignore the previous one.
>
> This addresses the bug report in the hidapi: https://github.com/libusb/hidapi/issues/514
> The patch was tested using the test snippets attached in the issue above
> on 6.2.0-76060200-generic (PopOS 22.04)

The problem is that hidapi is not the sole user of hidraw, and this is
a breaking change for everyone.

If we were to accept this, when hidraw has always been that way on
linux since 2011 when it was introduced, you can be sure that there
are going to be very angry users who suddenly have a failure when
retrieving the input/feature report.
So if hidapi expects the first byte to stay the same, just fix that
case when calling that hidraw ioctl in hidapi.

A possible solution would be to add a new ioctl with a "better"
behavior, but a new ioctl will actually be worse because you have to
update both the kernel *and* hidapi to make use of the new ioctl, at
which point, just fixing userspace is actually simpler and better.

Cheers,
Benjamin

>
>  drivers/hid/hidraw.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
>
> 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;
>   }
>