| Message ID | 38F34842-3087-43CB-B814-CDBC52FD2084@getmailspring.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: hidraw: Keep the report ID on buffer in get_report | expand |
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; > } >
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;
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(-) }