From 59af2be74f3d4397704c5958c83bb7fc2af6d540 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Tue, 12 Jan 2021 01:42:48 +0100 Subject: [PATCH 1/3] HIDUniversal, HIDComposite: Don't overflow hidInterfaces[] or epInfo[] If a connected device has more than 3 (maxHidInterfaces) HID interfaces, which is not unusual with modern keyboards, EndpointXtract() wrote beyond the hidInterfaces[] array and corrupted bHasReportId, PID + VID. The same could happen with the epInfo[] array. Now this is fixed by checking bNumIface/bNMumEP before adding new elements to those arrays. --- hidcomposite.cpp | 4 +++- hiduniversal.cpp | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/hidcomposite.cpp b/hidcomposite.cpp index 3f0a21c2..62cc9d0a 100644 --- a/hidcomposite.cpp +++ b/hidcomposite.cpp @@ -306,6 +306,8 @@ void HIDComposite::EndpointXtract(uint8_t conf, uint8_t iface, uint8_t alt, uint // Fill in interface structure in case of new interface if(!piface) { + if(bNumIface >= maxHidInterfaces) + return; // don't overflow hidInterfaces[] piface = hidInterfaces + bNumIface; piface->bmInterface = iface; piface->bmAltSet = alt; @@ -319,7 +321,7 @@ void HIDComposite::EndpointXtract(uint8_t conf, uint8_t iface, uint8_t alt, uint if(!SelectInterface(iface, proto)) index = 0; - if(index) { + if(index && bNumEP < totalEndpoints) { // Fill in the endpoint info structure epInfo[bNumEP].epAddr = (pep->bEndpointAddress & 0x0F); epInfo[bNumEP].maxPktSize = (uint8_t)pep->wMaxPacketSize; diff --git a/hiduniversal.cpp b/hiduniversal.cpp index 49309df4..4ced3b14 100644 --- a/hiduniversal.cpp +++ b/hiduniversal.cpp @@ -308,6 +308,8 @@ void HIDUniversal::EndpointXtract(uint8_t conf, uint8_t iface, uint8_t alt, uint // Fill in interface structure in case of new interface if(!piface) { + if(bNumIface >= maxHidInterfaces) + return; // don't overflow hidInterfaces[] piface = hidInterfaces + bNumIface; piface->bmInterface = iface; piface->bmAltSet = alt; @@ -318,7 +320,7 @@ void HIDUniversal::EndpointXtract(uint8_t conf, uint8_t iface, uint8_t alt, uint if((pep->bmAttributes & bmUSB_TRANSFER_TYPE) == USB_TRANSFER_TYPE_INTERRUPT) index = (pep->bEndpointAddress & 0x80) == 0x80 ? epInterruptInIndex : epInterruptOutIndex; - if(index) { + if(index && bNumEP < totalEndpoints) { // Fill in the endpoint info structure epInfo[bNumEP].epAddr = (pep->bEndpointAddress & 0x0F); epInfo[bNumEP].maxPktSize = (uint8_t)pep->wMaxPacketSize; From 6d7984ade21f34d30c484ee4cc34ff1bc8951829 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Wed, 13 Jan 2021 07:14:36 +0100 Subject: [PATCH 2/3] Log info when not adding Interface/Endpoint because max is reached --- hidcomposite.cpp | 18 +++++++++++++++--- hiduniversal.cpp | 18 +++++++++++++++--- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/hidcomposite.cpp b/hidcomposite.cpp index 62cc9d0a..6cb866d3 100644 --- a/hidcomposite.cpp +++ b/hidcomposite.cpp @@ -306,8 +306,13 @@ void HIDComposite::EndpointXtract(uint8_t conf, uint8_t iface, uint8_t alt, uint // Fill in interface structure in case of new interface if(!piface) { - if(bNumIface >= maxHidInterfaces) - return; // don't overflow hidInterfaces[] + if(bNumIface >= maxHidInterfaces) { + // don't overflow hidInterfaces[] + Notify(PSTR("\r\n HIDComposite::EndpointXtract(): Not adding HID interface because we already have "), 0x80); + Notify(bNumIface, 0x80); + Notify(PSTR(" interfaces and can't hold more. "), 0x80); + return; + } piface = hidInterfaces + bNumIface; piface->bmInterface = iface; piface->bmAltSet = alt; @@ -321,7 +326,14 @@ void HIDComposite::EndpointXtract(uint8_t conf, uint8_t iface, uint8_t alt, uint if(!SelectInterface(iface, proto)) index = 0; - if(index && bNumEP < totalEndpoints) { + if(index) { + if(bNumEP >= totalEndpoints) { + // don't overflow epInfo[] either + Notify(PSTR("\r\n HIDComposite::EndpointXtract(): Not adding endpoint info because we already have "), 0x80); + Notify(bNumEP, 0x80); + Notify(PSTR(" endpoints and can't hold more. "), 0x80); + return; + } // Fill in the endpoint info structure epInfo[bNumEP].epAddr = (pep->bEndpointAddress & 0x0F); epInfo[bNumEP].maxPktSize = (uint8_t)pep->wMaxPacketSize; diff --git a/hiduniversal.cpp b/hiduniversal.cpp index 4ced3b14..bbd06af7 100644 --- a/hiduniversal.cpp +++ b/hiduniversal.cpp @@ -308,8 +308,13 @@ void HIDUniversal::EndpointXtract(uint8_t conf, uint8_t iface, uint8_t alt, uint // Fill in interface structure in case of new interface if(!piface) { - if(bNumIface >= maxHidInterfaces) - return; // don't overflow hidInterfaces[] + if(bNumIface >= maxHidInterfaces) { + // don't overflow hidInterfaces[] + Notify(PSTR("\r\n HIDUniversal::EndpointXtract(): Not adding HID interface because we already have "), 0x80); + Notify(bNumIface, 0x80); + Notify(PSTR(" interfaces and can't hold more. "), 0x80); + return; + } piface = hidInterfaces + bNumIface; piface->bmInterface = iface; piface->bmAltSet = alt; @@ -320,7 +325,14 @@ void HIDUniversal::EndpointXtract(uint8_t conf, uint8_t iface, uint8_t alt, uint if((pep->bmAttributes & bmUSB_TRANSFER_TYPE) == USB_TRANSFER_TYPE_INTERRUPT) index = (pep->bEndpointAddress & 0x80) == 0x80 ? epInterruptInIndex : epInterruptOutIndex; - if(index && bNumEP < totalEndpoints) { + if(index) { + if(bNumEP >= totalEndpoints) { + // don't overflow epInfo[] either + Notify(PSTR("\r\n HIDUniversal::EndpointXtract(): Not adding endpoint info because we already have "), 0x80); + Notify(bNumEP, 0x80); + Notify(PSTR(" endpoints and can't hold more. "), 0x80); + return; + } // Fill in the endpoint info structure epInfo[bNumEP].epAddr = (pep->bEndpointAddress & 0x0F); epInfo[bNumEP].maxPktSize = (uint8_t)pep->wMaxPacketSize; From 3658dc68f3736765246eedec7a70a6dd79884365 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Wed, 13 Jan 2021 07:44:36 +0100 Subject: [PATCH 3/3] Increase USBHID::maxHidInterfaces from 3 to 5 It's not that uncommon to have more than three HID interfaces in a USB device - even an Arduino Leonardo (or Pro Micro) always has two just for the CDC; if you add a Boot Keyboard HID interface and one generic HID interface for consumer control (multimedia) keys you already have four.. Many commercial gaming keyboards also have more then three interfaces (I don't think they really need that many but it is what it is). --- usbhid.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/usbhid.h b/usbhid.h index cf74f57d..e85ca575 100644 --- a/usbhid.h +++ b/usbhid.h @@ -149,7 +149,7 @@ protected: static const uint8_t epInterruptInIndex = 1; // InterruptIN endpoint index static const uint8_t epInterruptOutIndex = 2; // InterruptOUT endpoint index - static const uint8_t maxHidInterfaces = 3; + static const uint8_t maxHidInterfaces = 5; static const uint8_t maxEpPerInterface = 2; static const uint8_t totalEndpoints = (maxHidInterfaces * maxEpPerInterface + 1); // We need to make room for the control endpoint