Re: [PATCH 2/4] HID: core: introduce hid_safe_input_report()

[Benjamin Tissoires]

On Thu, Apr 16, 2026 at 11:41 AM Bastien Nocera <hadess@xxxxxxxxxx> wrote:
>
> On Wed, 2026-04-15 at 11:38 +0200, Benjamin Tissoires wrote:
> > hid_input_report() is used in too many places to have a commit that
> > doesn't cross subsystem borders. Instead of changing the API,
> > introduce
> > a new one when things matters in the transport layers:
> > - usbhid
> > - i2chid
> >
> > This effectively revert to the old behavior for those two transport
> > layers.
> >
> > Fixes: 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing
> > bogus memset()")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Benjamin Tissoires <bentiss@xxxxxxxxxx>
> > ---
> >  drivers/hid/hid-core.c             | 21 +++++++++++++++++++++
> >  drivers/hid/i2c-hid/i2c-hid-core.c |  7 ++++---
> >  drivers/hid/usbhid/hid-core.c      | 11 ++++++-----
> >  include/linux/hid.h                |  2 ++
> >  4 files changed, 33 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index a806820df7e5..cb0ad99e7a0a 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -2191,6 +2191,27 @@ int hid_input_report(struct hid_device *hid,
> > enum hid_report_type type, u8 *data
> >  }
> >  EXPORT_SYMBOL_GPL(hid_input_report);
> >
> > +/**
> > + * hid_safe_input_report - report data from lower layer (usb, bt...)
> > + *
> > + * @hid: hid device
> > + * @type: HID report type (HID_*_REPORT)
> > + * @data: report contents
> > + * @bufsize: allocated size of the data buffer
> > + * @size: useful size of data parameter
> > + * @interrupt: distinguish between interrupt and control transfers
> > + *
> > + * This is data entry for lower layers.
>
> You probably want to explain why it should be used instead of
> hid_input_report() in this doc blurb, and modify the hid_input_report()
> docs to mention that this should be used.

Good point. Sending v2 ASAP.

>
> Maybe hid_input_report() should also be marked as deprecated somehow,
> to avoid new users?

Well, it's not entirely deprecated because, for instance, in uhid we
only have the buffer with the provided size around. So we can't be
less restrictive in that precise case, and then switching to _safe
will not change a bit.

Cheers,
Benjamin

>
> Cheers
>
> > + */
> > +int hid_safe_input_report(struct hid_device *hid, enum
> > hid_report_type type, u8 *data,
> > +                       size_t bufsize, u32 size, int interrupt)
> > +{
> > +     return __hid_input_report(hid, type, data, bufsize, size,
> > interrupt, 0,
> > +                               false, /* from_bpf */
> > +                               false /* lock_already_taken */);
> > +}
> > +EXPORT_SYMBOL_GPL(hid_safe_input_report);
> > +
> >  bool hid_match_one_id(const struct hid_device *hdev,
> >                     const struct hid_device_id *id)
> >  {
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-
> > hid/i2c-hid-core.c
> > index 5a183af3d5c6..e0a302544cef 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > @@ -574,9 +574,10 @@ static void i2c_hid_get_input(struct i2c_hid
> > *ihid)
> >               if (ihid->hid->group != HID_GROUP_RMI)
> >                       pm_wakeup_event(&ihid->client->dev, 0);
> >
> > -             hid_input_report(ihid->hid, HID_INPUT_REPORT,
> > -                             ihid->inbuf + sizeof(__le16),
> > -                             ret_size - sizeof(__le16), 1);
> > +             hid_safe_input_report(ihid->hid, HID_INPUT_REPORT,
> > +                                   ihid->inbuf + sizeof(__le16),
> > +                                   ihid->bufsize -
> > sizeof(__le16),
> > +                                   ret_size - sizeof(__le16), 1);
> >       }
> >
> >       return;
> > diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-
> > core.c
> > index fbbfc0f60829..5af93b9b1fb5 100644
> > --- a/drivers/hid/usbhid/hid-core.c
> > +++ b/drivers/hid/usbhid/hid-core.c
> > @@ -283,9 +283,9 @@ static void hid_irq_in(struct urb *urb)
> >                       break;
> >               usbhid_mark_busy(usbhid);
> >               if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) {
> > -                     hid_input_report(urb->context,
> > HID_INPUT_REPORT,
> > -                                      urb->transfer_buffer,
> > -                                      urb->actual_length, 1);
> > +                     hid_safe_input_report(urb->context,
> > HID_INPUT_REPORT,
> > +                                           urb->transfer_buffer,
> > urb->transfer_buffer_length,
> > +                                           urb->actual_length,
> > 1);
> >                       /*
> >                        * autosuspend refused while keys are
> > pressed
> >                        * because most keyboards don't wake up when
> > @@ -482,9 +482,10 @@ static void hid_ctrl(struct urb *urb)
> >       switch (status) {
> >       case 0:                 /* success */
> >               if (usbhid->ctrl[usbhid->ctrltail].dir ==
> > USB_DIR_IN)
> > -                     hid_input_report(urb->context,
> > +                     hid_safe_input_report(urb->context,
> >                               usbhid->ctrl[usbhid-
> > >ctrltail].report->type,
> > -                             urb->transfer_buffer, urb-
> > >actual_length, 0);
> > +                             urb->transfer_buffer, urb-
> > >transfer_buffer_length,
> > +                             urb->actual_length, 0);
> >               break;
> >       case -ESHUTDOWN:        /* unplug */
> >               unplug = 1;
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index ac432a2ef415..bfb9859f391e 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -1030,6 +1030,8 @@ struct hid_field *hid_find_field(struct
> > hid_device *hdev, unsigned int report_ty
> >  int hid_set_field(struct hid_field *, unsigned, __s32);
> >  int hid_input_report(struct hid_device *hid, enum hid_report_type
> > type, u8 *data, u32 size,
> >                    int interrupt);
> > +int hid_safe_input_report(struct hid_device *hid, enum
> > hid_report_type type, u8 *data,
> > +                       size_t bufsize, u32 size, int interrupt);
> >  struct hid_field *hidinput_get_led_field(struct hid_device *hid);
> >  unsigned int hidinput_count_leds(struct hid_device *hid);
> >  __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16
> > code);
>