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

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

[js]

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

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

[Alan Stern]

On Tue, 4 Feb 2020, js wrote:

> 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.

Such behavior would not overflow anything.  The driver never transfers 
more than HID_MAX_BUFFER_SIZE, no matter how data the device wants to 
send.  The only effect would be a truncated report (which probably 
would lead to 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.

This is not an unreasonable approach, although I do not think you have 
described it fairly.

On the other hand, how often does it happen that a device sends report 
messages that are considerably smaller than the value given in the 
descriptor?  I can't tell from the Bugzilla report exactly what the 
ELON9038 digitizer and other devices are doing.

Alan Stern

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

[Johan Korsnes (jkorsnes)]

On 2/4/20 1:28 PM, js wrote:
> 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);

With your patch I assume we're still vulnerable to the off-by-one
memset() for which I proposed a fix[0]. If so, I suggest my patch is
applied first, or simply merged with this patch. With your patch we no
longer abort at probe if a report is too long. We are therefore more
likely to end up with a kernel Oops and ensuing crash if we receive a
report with size greater than HID_MAX_BUFFER_SIZE.

[0] https://lore.kernel.org/linux-usb/20200117120836.2354966-1-jkorsnes@cisco.com/

Johan

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

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

[Benjamin Tissoires]

On Tue, Feb 4, 2020 at 6:10 PM Johan Korsnes (jkorsnes)
<jkorsnes@cisco.com> wrote:
>
> On 2/4/20 1:28 PM, js wrote:
> > 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);
>
> With your patch I assume we're still vulnerable to the off-by-one
> memset() for which I proposed a fix[0]. If so, I suggest my patch is
> applied first, or simply merged with this patch. With your patch we no
> longer abort at probe if a report is too long. We are therefore more
> likely to end up with a kernel Oops and ensuing crash if we receive a
> report with size greater than HID_MAX_BUFFER_SIZE.
>
> [0] https://lore.kernel.org/linux-usb/20200117120836.2354966-1-jkorsnes@cisco.com/

Hi Johan,

can you please fix your process to also include the linux-input ML and
myself to HID related patches?

It doesn't matter for this one, as I see it in the HID tree, but I
wasn't aware of it nor the other one ("HID: core: increase HID report
buffer size to 8KiB"). And I like being aware of HID patches :)

The main reason is that whenever a patch hit linux-input, I run a
series of test with https://gitlab.freedesktop.org/libevdev/hid-tools,
and the 2 fixes you sent are some very strong candidates for
regression tests.

Can you send me your report descriptors with the `hid-recorder` tool
in the hid-tools project, and I'll add your device in the test suite?

Cheers,
Benjamin

>
> Johan
>
> > + } 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
> >>>
> >>
>

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

[Johan Korsnes (jkorsnes)]

On 2/14/20 11:49 AM, Benjamin Tissoires wrote:
> On Tue, Feb 4, 2020 at 6:10 PM Johan Korsnes (jkorsnes)
> <jkorsnes@cisco.com> wrote:
>>
>> On 2/4/20 1:28 PM, js wrote:
>>> 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);
>>
>> With your patch I assume we're still vulnerable to the off-by-one
>> memset() for which I proposed a fix[0]. If so, I suggest my patch is
>> applied first, or simply merged with this patch. With your patch we no
>> longer abort at probe if a report is too long. We are therefore more
>> likely to end up with a kernel Oops and ensuing crash if we receive a
>> report with size greater than HID_MAX_BUFFER_SIZE.
>>
>> [0] https://lore.kernel.org/linux-usb/20200117120836.2354966-1-jkorsnes@cisco.com/
> 
> Hi Johan,
> 
> can you please fix your process to also include the linux-input ML and
> myself to HID related patches?
> 

Hi Benjamin,

Sorry about that -- I should have run get_maintainer.pl.

> It doesn't matter for this one, as I see it in the HID tree, but I
> wasn't aware of it nor the other one ("HID: core: increase HID report
> buffer size to 8KiB"). And I like being aware of HID patches :)
> 
> The main reason is that whenever a patch hit linux-input, I run a
> series of test with https://gitlab.freedesktop.org/libevdev/hid-tools,
> and the 2 fixes you sent are some very strong candidates for
> regression tests.
> 

Absolutely, that makes sense.

> Can you send me your report descriptors with the `hid-recorder` tool
> in the hid-tools project, and I'll add your device in the test suite?
> 

Thank you for the offer; I think it would be great to have this product
in the test suite. I've sent an email to the manufacturer today where I
ask for permission to share report descriptor, VID and PID. I will let
you know as soon as I get a reply.

Johan

> Cheers,
> Benjamin
> 
>>
>> Johan
>>
>>> + } 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
>>>>>
>>>>
>>
>