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