| Message ID | 20200115123344.4650-2-jkorsnes@cisco.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] HID: core: fix off-by-one memset in hid_report_raw_event() | expand |
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
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 >
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,
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
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(-)