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

[Alan Stern <stern@rowland.harvard.edu>]

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