Re: [PATCH v2] HID: truncate hid reports exceeding HID_MAX_BUFFER_SIZE

[js <sym.i.nem@gmail.com>]

Commit 8ec321e96e05 ("HID: Fix slab-out-of-bounds read in
hid_field_extract") introduced a regression bug that broke
hardware probes which request large report sizes.

An example of this hardware is the ELON9038 digitizer on the
Microsoft Surface Go as per bug id 206259.
https://bugzilla.kernel.org/show_bug.cgi?id=206259

To eliminate the regression, return 0 instead of -1 when a
large report size is requested, allowing the hardware to
probe properly while size error is output to kernel log.

Commit 8ec321e96e05 does not enforce buffer size limitation
on the size of the incoming report.
Added enforcement by truncation to prevent buffer overflow in
hid_report_raw_event().

Fixes: 8ec321e96e05 ("HID: Fix slab-out-of-bounds read in hid_field_extract")
Reported-and-tested-by: James Smith <sym.i.nem@gmail.com>
Signed-off-by: James Smith <sym.i.nem@gmail.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Armando Visconti <armando.visconti@st.com>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Johan Korsnes <jkorsnes@cisco.com>
Cc: stable@vger.kernel.org
---
Sorry about my earlier email, I'm new to this forum and am still
learning the conventions.

At your suggestion, I examined the code more carefully and I think
that the previous patch (commit 8ec321e96e05) did not solve the buffer
overflow at all, it just killed a tranche of hardware of unknown size
which requests report sizes exceeding 4K.

The problem, and why the previous patch didn't really address the
issue, is that the enforcement occurs at a declarative point in the
code, which is to say, the device is just describing itself, it is not
actually requesting memory or generating a report. A malicious device
could easily describe itself incorrectly then generate a report
exceeding both the size it indicated in hid_add_field() and
HID_MAX_BUFFER_SIZE, overflowing the buffer and causing unintended
behavior.

The correct point to enforce a buffer size constraint is the point
where the report is taken from the device and copied into the hid
handling layer. From my examination of the code, this seems to be in
hid_report_raw_event(). Thus, I placed an enforcement constraint on
the report size in that method, took out the enforcement constraint in
hid_add_field(), because it was causing a hardware regression and not
properly enforcing the boundary constraint, and added user-facing
warnings to notify when hardware is going to be affected by the
introduced boundary constraints.

I also Cc'd Johan Korsnes because he submitted a patch for a related problem.

Thanks,

js
---

--- a/drivers/hid/hid-core.c  2020-01-28 02:04:58.918309900 +0000
+++ b/drivers/hid/hid-core.c  2020-01-29 06:37:22.861190986 +0000
@@ -290,8 +290,12 @@ static int hid_add_field(struct hid_pars

  /* Total size check: Allow for possible report index byte */
  if (report->size > (HID_MAX_BUFFER_SIZE - 1) << 3) {
-   hid_err(parser->device, "report is too long\n");
-   return -1;
+   hid_warn(parser->device,
+       "report is too long and will be truncated: %d > %d\n",
+       report->size,
+       (HID_MAX_BUFFER_SIZE - 1) << 3);
+   parser->global.report_size = report->size =
+     (HID_MAX_BUFFER_SIZE - 1) << 3;
  }

  if (!parser->local.usage_index) /* Ignore padding fields */
@@ -1748,6 +1752,10 @@ int hid_report_raw_event(struct hid_devi
    dbg_hid("report %d is too short, (%d < %d)\n", report->id,
        csize, rsize);
    memset(cdata + csize, 0, rsize - csize);
+ } else if (csize > rsize) {
+   hid_warn(hid, "report %d is too long, truncating (%d > %d)\n",
+       report->id, csize, rsize);
+   report->size = size = rsize;
  }

  if ((hid->claimed & HID_CLAIMED_HIDDEV) && hid->hiddev_report_event)


On Tue, Jan 28, 2020 at 12:44 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi,
>
> On Mon, Jan 27, 2020 at 9:41 PM js <sym.i.nem@gmail.com> wrote:
> >
> > i'm bumping this bug because i haven't heard anything from the
> > maintainers for a week.
>
> Apologies for the delay. I have been in a conference the past 2 weeks
> in Australia, so couldn't handle much of upstream.
> Furthermore, we are currently in the merge window, which means we
> should not push patches to linux-next unless they are absolutely
> needed.
>
> > there's been no change in the git either.
> > what's going on guys? this is a tiny patch for a very simple bug.
> > it should be a fast review and commit to the kernel tree.
>
> Nope, that is not that simple:
>
> - please submit your patches following
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst#n340
> Our tools require the patches to not be attached in an email so we can
> process them
> - this patch affects the core of the HID subsystem, which means we
> should take extra care when dealing with it to not break other systems
> - this patch seems to paper over a security patch
> (8ec321e96e056de84022c032ffea253431a83c3c) by changing the return
> value from an error to "yeah, that's fine". So unless there is a proof
> that this is the correct way, it's going to be a nack from me until
> proven otherwise
> - this patch affects in the end hid-multitouch, and as mentioned in
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-multitouch.c#n26
> I'd like to have a reproducer in
> https://gitlab.freedesktop.org/libevdev/hid-tools so we do not break
> those devices in the future.
>
> So I understand the frustration of having a HW regression, but this
> patch is clearly not the correct solution given what I have here, so I
> can not push it right now.
>
> Cheers,
> Benjamin
>
> >
> > js
> >
> > On Sun, Jan 19, 2020 at 1:14 PM js <sym.i.nem@gmail.com> wrote:
> > >
> > > i posted this bug to bugzilla with the attached patch.
> > > this email is to notify the maintainers.
> > > https://bugzilla.kernel.org/show_bug.cgi?id=206259
> > >
> > > thanks!
> > >
> > > js
> > > ----
> > >
> > > ELAN i2c digitizer on microsoft surface go fails to initialize and
> > > device is non-functional
> > >
> > > initialization fails on 4.19.96:
> > > ----
> > > [    5.507245] hid-generic 0018:04F3:261A.0005: report is too long
> > > [    5.507256] hid-generic 0018:04F3:261A.0005: item 0 1 0 8 parsing failed
> > > [    5.507290] hid-generic: probe of 0018:04F3:261A.0005 failed with error -22
> > > [    5.556409] hid-multitouch 0018:04F3:261A.0005: report is too long
> > > [    5.581641] hid-multitouch 0018:04F3:261A.0005: item 0 1 0 8 parsing failed
> > > [    5.618495] hid-multitouch: probe of 0018:04F3:261A.0005 failed
> > > with error -22
> > >
> > > initialization succeeds on 4.19.95:
> > > ----
> > > [    7.150887] hid-generic 0018:04F3:261A.0001: input,hidraw2: I2C HID
> > > v1.00 Device [ELAN9038:00 04F3:261A] on i2c-ELAN9038:00
> > > [    8.253077] input: ELAN9038:00 04F3:261A as
> > > /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-1/i2c-ELAN9038:00/0018:04F3:261A.0001/input/input20
> > > [    8.253219] input: ELAN9038:00 04F3:261A Pen as
> > > /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-1/i2c-ELAN9038:00/0018:04F3:261A.0001/input/input23
> > > [    8.253330] hid-multitouch 0018:04F3:261A.0001: input,hidraw0: I2C
> > > HID v1.00 Device [ELAN9038:00 04F3:261A] on i2c-ELAN9038:00
> > >
> > > problem seems to be due to this commit:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?h=linux-4.19.y&id=31d06cc8e7caec36bedeb4f90444920431462f61
> >
>