Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] HID: core: fix off-by-one memset in hid_report_raw_event()
@ 2020-01-15 12:33 Johan Korsnes
  2020-01-15 12:33 ` [PATCH 2/2] HID: core: increase HID report buffer size to 8KiB Johan Korsnes
  0 siblings, 1 reply; 5+ messages in thread
From: Johan Korsnes @ 2020-01-15 12:33 UTC (permalink / raw)
  To: linux-usb; +Cc: Johan Korsnes, Armando Visconti, Jiri Kosina, Alan Stern

In case a report is greater than HID_MAX_BUFFER_SIZE it is truncated,
but the report-number byte is not correctly handled. This results in a
off-by-one in the following memset, causing a kernel Oops and ensuing
system crash.

Note: With commit 8ec321e96e05 ("HID: Fix slab-out-of-bounds read in
hid_field_extract") I no longer hit the kernel Oops as we instead fail
"controlled" at probe if there is a report too long in the HID
report descriptor. I still believe this is worth fixing though, as
hid_report_raw_event() should have its logic correct. But if we fail at
probe for too large reports, a better solution might be to remove this
truncation logic entirely.

Fixes: 966922f26c7f ("HID: fix a crash in hid_report_raw_event()
                     function.")
Signed-off-by: Johan Korsnes <jkorsnes@cisco.com>
Cc: Armando Visconti <armando.visconti@st.com>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: Alan Stern <stern@rowland.harvard.edu>
---
 drivers/hid/hid-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 851fe54ea59e..359616e3efbb 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1741,7 +1741,9 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
 
 	rsize = ((report->size - 1) >> 3) + 1;
 
-	if (rsize > HID_MAX_BUFFER_SIZE)
+	if (report_enum->numbered && rsize >= HID_MAX_BUFFER_SIZE)
+		rsize = HID_MAX_BUFFER_SIZE - 1;
+	else if (rsize > HID_MAX_BUFFER_SIZE)
 		rsize = HID_MAX_BUFFER_SIZE;
 
 	if (csize < rsize) {
-- 
2.24.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 2/2] HID: core: increase HID report buffer size to 8KiB
  2020-01-15 12:33 [PATCH 1/2] HID: core: fix off-by-one memset in hid_report_raw_event() Johan Korsnes
@ 2020-01-15 12:33 ` Johan Korsnes
  2020-01-15 15:21   ` Alan Stern
  0 siblings, 1 reply; 5+ messages in thread
From: Johan Korsnes @ 2020-01-15 12:33 UTC (permalink / raw)
  To: linux-usb; +Cc: Johan Korsnes, Alan Stern, Armando Visconti, Jiri Kosina

We have a touch device that reports its opens and shorts test results
in HID buffers of size 8184 bytes. With this patch we're able to
successfully obtain these reports.

Alan Stern: Your commit 8ec321e96e05 ("HID: Fix slab-out-of-bounds
read in hid_field_extract") states:

"This patch fixes the problem by rejecting any report whose total
length exceeds the HID_MAX_BUFFER_SIZE limit (minus one byte to allow
for a possible report index). In theory a device could have a report
longer than that, but if there was such a thing we wouldn't handle it
correctly anyway."

Is this something we have to worry about when increasing the buffer
size? Or are you referring to the fact that we previously truncated
the reports if they exceeded max buffer size?

Signed-off-by: Johan Korsnes <jkorsnes@cisco.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Armando Visconti <armando.visconti@st.com>
Cc: Jiri Kosina <jkosina@suse.cz>
---
 include/linux/hid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/hid.h b/include/linux/hid.h
index cd41f209043f..875f71132b14 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -492,7 +492,7 @@ struct hid_report_enum {
 };
 
 #define HID_MIN_BUFFER_SIZE	64		/* make sure there is at least a packet size of space */
-#define HID_MAX_BUFFER_SIZE	4096		/* 4kb */
+#define HID_MAX_BUFFER_SIZE	8192		/* 8kb */
 #define HID_CONTROL_FIFO_SIZE	256		/* to init devices with >100 reports */
 #define HID_OUTPUT_FIFO_SIZE	64
 
-- 
2.24.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] HID: core: increase HID report buffer size to 8KiB
  2020-01-15 12:33 ` [PATCH 2/2] HID: core: increase HID report buffer size to 8KiB Johan Korsnes
@ 2020-01-15 15:21   ` Alan Stern
  2020-01-15 15:50     ` Johan Korsnes (jkorsnes)
  0 siblings, 1 reply; 5+ messages in thread
From: Alan Stern @ 2020-01-15 15:21 UTC (permalink / raw)
  To: Johan Korsnes; +Cc: linux-usb, Armando Visconti, Jiri Kosina

On Wed, 15 Jan 2020, Johan Korsnes wrote:

> We have a touch device that reports its opens and shorts test results
> in HID buffers of size 8184 bytes. With this patch we're able to
> successfully obtain these reports.
> 
> Alan Stern: Your commit 8ec321e96e05 ("HID: Fix slab-out-of-bounds
> read in hid_field_extract") states:
> 
> "This patch fixes the problem by rejecting any report whose total
> length exceeds the HID_MAX_BUFFER_SIZE limit (minus one byte to allow
> for a possible report index). In theory a device could have a report
> longer than that, but if there was such a thing we wouldn't handle it
> correctly anyway."
> 
> Is this something we have to worry about when increasing the buffer
> size? Or are you referring to the fact that we previously truncated
> the reports if they exceeded max buffer size?

The latter.  And after this patch we will still truncate reports that
exceed the max buffer size, no "previously" about it.

(Incidentally, these last three paragraphs don't belong in the patch
description; nobody will care about them once the patch has been
merged.  You should have put them below the "---" separator line.)

Alan Stern


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] HID: core: increase HID report buffer size to 8KiB
  2020-01-15 15:21   ` Alan Stern
@ 2020-01-15 15:50     ` Johan Korsnes (jkorsnes)
  2020-01-17 10:15       ` Jiri Kosina
  0 siblings, 1 reply; 5+ messages in thread
From: Johan Korsnes (jkorsnes) @ 2020-01-15 15:50 UTC (permalink / raw)
  To: Alan Stern; +Cc: linux-usb, Armando Visconti, Jiri Kosina

On 1/15/20 4:21 PM, Alan Stern wrote:
> On Wed, 15 Jan 2020, Johan Korsnes wrote:
> 
>> We have a touch device that reports its opens and shorts test results
>> in HID buffers of size 8184 bytes. With this patch we're able to
>> successfully obtain these reports.
>>
>> Alan Stern: Your commit 8ec321e96e05 ("HID: Fix slab-out-of-bounds
>> read in hid_field_extract") states:
>>
>> "This patch fixes the problem by rejecting any report whose total
>> length exceeds the HID_MAX_BUFFER_SIZE limit (minus one byte to allow
>> for a possible report index). In theory a device could have a report
>> longer than that, but if there was such a thing we wouldn't handle it
>> correctly anyway."
>>
>> Is this something we have to worry about when increasing the buffer
>> size? Or are you referring to the fact that we previously truncated
>> the reports if they exceeded max buffer size?
> 
> The latter.  And after this patch we will still truncate reports that
> exceed the max buffer size, no "previously" about it.
> 
> (Incidentally, these last three paragraphs don't belong in the patch
> description; nobody will care about them once the patch has been
> merged.  You should have put them below the "---" separator line.)
> 

Right. If this patch is of interest I can submit a second version
with a cleaned-up patch description.

Regards,
Johan

> Alan Stern
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] HID: core: increase HID report buffer size to 8KiB
  2020-01-15 15:50     ` Johan Korsnes (jkorsnes)
@ 2020-01-17 10:15       ` Jiri Kosina
  0 siblings, 0 replies; 5+ messages in thread
From: Jiri Kosina @ 2020-01-17 10:15 UTC (permalink / raw)
  To: Johan Korsnes (jkorsnes); +Cc: Alan Stern, linux-usb, Armando Visconti

On Wed, 15 Jan 2020, Johan Korsnes (jkorsnes) wrote:

> >> We have a touch device that reports its opens and shorts test results
> >> in HID buffers of size 8184 bytes. With this patch we're able to
> >> successfully obtain these reports.
> >>
> >> Alan Stern: Your commit 8ec321e96e05 ("HID: Fix slab-out-of-bounds
> >> read in hid_field_extract") states:
> >>
> >> "This patch fixes the problem by rejecting any report whose total
> >> length exceeds the HID_MAX_BUFFER_SIZE limit (minus one byte to allow
> >> for a possible report index). In theory a device could have a report
> >> longer than that, but if there was such a thing we wouldn't handle it
> >> correctly anyway."
> >>
> >> Is this something we have to worry about when increasing the buffer
> >> size? Or are you referring to the fact that we previously truncated
> >> the reports if they exceeded max buffer size?
> > 
> > The latter.  And after this patch we will still truncate reports that
> > exceed the max buffer size, no "previously" about it.
> > 
> > (Incidentally, these last three paragraphs don't belong in the patch
> > description; nobody will care about them once the patch has been
> > merged.  You should have put them below the "---" separator line.)
> > 
> 
> Right. If this patch is of interest I can submit a second version
> with a cleaned-up patch description.

Please do; I'll be happy to merge it afterwards together with the first 
fix.

Thanks,

-- 
Jiri Kosina
SUSE Labs


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 12:33 [PATCH 1/2] HID: core: fix off-by-one memset in hid_report_raw_event() Johan Korsnes
2020-01-15 12:33 ` [PATCH 2/2] HID: core: increase HID report buffer size to 8KiB Johan Korsnes
2020-01-15 15:21   ` Alan Stern
2020-01-15 15:50     ` Johan Korsnes (jkorsnes)
2020-01-17 10:15       ` Jiri Kosina

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git