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@xxxxxxxxx>
> Signed-off-by: James Smith <sym.i.nem@xxxxxxxxx>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Cc: Armando Visconti <armando.visconti@xxxxxx>
> Cc: Jiri Kosina <jkosina@xxxxxxx>
> Cc: Johan Korsnes <jkorsnes@xxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
> ---
> 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@xxxxxxxxx/
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@xxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On Mon, Jan 27, 2020 at 9:41 PM js <sym.i.nem@xxxxxxxxx> 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@xxxxxxxxx> 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
>>>
>>
