From 4480b08b2dd9aa1362d546d7486452b496a1ea29 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 10 Feb 2022 18:30:50 -0600 Subject: [PATCH 1/3] Add testcase for KeyAddrEventQueue bounds checking failure `KeyAddrEventQueue::remove()` fails to confirm that the current queue is empty before decrementing the length and shifting entries in the queue arrays. If `remove()` or `shift()` is called when the queue is empty, `length_` gets decremented from 0 to 255 (because it's unsigned), and then a large section of memory gets shifted, mostly out of bounds of the event queue arrays, and probably wreaking havoc with any number of things. The plugin in this testcase should trigger this bug, and is detectable because it affects the value for the current time. It's not guaranteed to detect this bug, but it seems to be fairly consistent. Signed-off-by: Michael Richters --- tests/issues/1107/1107.ino | 71 +++++++++++++++++++++ tests/issues/1107/sketch.json | 6 ++ tests/issues/1107/test/testcase.cpp | 99 +++++++++++++++++++++++++++++ 3 files changed, 176 insertions(+) create mode 100644 tests/issues/1107/1107.ino create mode 100644 tests/issues/1107/sketch.json create mode 100644 tests/issues/1107/test/testcase.cpp diff --git a/tests/issues/1107/1107.ino b/tests/issues/1107/1107.ino new file mode 100644 index 00000000..c3689c05 --- /dev/null +++ b/tests/issues/1107/1107.ino @@ -0,0 +1,71 @@ +/* -*- mode: c++ -*- + * Copyright (C) 2022 Keyboard.io, Inc. + * + * This program is free software: you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation, version 3. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see . + */ + +#include "Kaleidoscope.h" + +// *INDENT-OFF* +KEYMAPS( + [0] = KEYMAP_STACKED + ( + XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX + ,XXX + + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX ,XXX + ,XXX ,XXX ,XXX ,XXX + ,XXX + ) +) // KEYMAPS( +// *INDENT-ON* + +#include + +namespace kaleidoscope { +namespace plugin { + +class QueueLeaker : public Plugin { + public: + EventHandlerResult afterEachCycle() { + // Shift queue without checking to be sure that queue is not empty. If + // KeyAddrEventQueue uses an unsigned integer for queue length, this will + // result in some bad behavior. + queue_.shift(); + return EventHandlerResult::OK; + } + private: + KeyAddrEventQueue<8> queue_; +}; + +} // namespace plugin +} // namespace kaleidoscope + +kaleidoscope::plugin::QueueLeaker QueueLeaker; + +KALEIDOSCOPE_INIT_PLUGINS(QueueLeaker); + +void setup() { + Kaleidoscope.setup(); +} + +void loop() { + Kaleidoscope.loop(); +} diff --git a/tests/issues/1107/sketch.json b/tests/issues/1107/sketch.json new file mode 100644 index 00000000..8cc86922 --- /dev/null +++ b/tests/issues/1107/sketch.json @@ -0,0 +1,6 @@ +{ + "cpu": { + "fqbn": "keyboardio:virtual:model01", + "port": "" + } +} diff --git a/tests/issues/1107/test/testcase.cpp b/tests/issues/1107/test/testcase.cpp new file mode 100644 index 00000000..ea4b3e28 --- /dev/null +++ b/tests/issues/1107/test/testcase.cpp @@ -0,0 +1,99 @@ +/* -*- mode: c++ -*- + * Copyright (C) 2022 Keyboard.io, Inc. + * + * This program is free software: you can redistribute it and/or modify it under + * the terms of the GNU General Public License as published by the Free Software + * Foundation, version 3. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more + * details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see . + */ + +#include "testing/setup-googletest.h" + +SETUP_GOOGLETEST(); + +namespace kaleidoscope { +namespace testing { +namespace { + +constexpr uint32_t t_0{0}; +constexpr uint32_t t_1{1}; +constexpr uint32_t t_10{10}; + +using ::testing::IsEmpty; + +class KeyAddrEventQueueLeak : public VirtualDeviceTest { + public: + void assertTimeElapses(uint32_t interval) { + // Record time at start + uint32_t start = Kaleidoscope.millisAtCycleStart(); + + // Run cycles until the given amount of time has elapsed + sim_.RunForMillis(interval); + + // Record time at end + uint32_t end = Kaleidoscope.millisAtCycleStart(); + + ASSERT_EQ((end - start), interval); + } +}; + +TEST_F(KeyAddrEventQueueLeak, TimeIsConsistent) { + assertTimeElapses(1); + assertTimeElapses(1); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); + assertTimeElapses(10); +} + +} // namespace +} // namespace testing +} // namespace kaleidoscope From c1bb8b254593233df8176b15ab9492acc7bce657 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 10 Feb 2022 19:22:05 -0600 Subject: [PATCH 2/3] Fix out-of-bounds memory write in KeyAddrEventQueue This change prevents `KeyAddrEventQueue::remove()` from shifting values in memory out of bounds of its arrays if `shift()` is called on an empty queue. It also adds a check to be sure that the entry removed is in the queue. Signed-off-by: Michael Richters --- src/kaleidoscope/KeyAddrEventQueue.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/kaleidoscope/KeyAddrEventQueue.h b/src/kaleidoscope/KeyAddrEventQueue.h index 326b84e5..214e535a 100644 --- a/src/kaleidoscope/KeyAddrEventQueue.h +++ b/src/kaleidoscope/KeyAddrEventQueue.h @@ -103,6 +103,8 @@ class KeyAddrEventQueue { // rather than using a ring buffer because we expect it will be called much // less often than the queue is searched via a for loop. void remove(uint8_t n = 0) { + if (n >= length_ || length_ == 0) + return; // assert(length > n); --length_; for (uint8_t i{n}; i < length_; ++i) { From c3ea0876c147d255611c699e75da2f4bcff41617 Mon Sep 17 00:00:00 2001 From: Michael Richters Date: Thu, 10 Feb 2022 11:51:25 -0600 Subject: [PATCH 3/3] Process Qukeys queue after checking timeout in afterEachCycle() To prevent the possibility of a call to `flushEvent()` when the queue is empty, we call `processQueue()` (which checks for an empty queue) after checking the hold timeout, rather than before. Signed-off-by: Michael Richters --- .../Kaleidoscope-Qukeys/src/kaleidoscope/plugin/Qukeys.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/plugins/Kaleidoscope-Qukeys/src/kaleidoscope/plugin/Qukeys.cpp b/plugins/Kaleidoscope-Qukeys/src/kaleidoscope/plugin/Qukeys.cpp index 72ecc8c4..bc9a63dc 100644 --- a/plugins/Kaleidoscope-Qukeys/src/kaleidoscope/plugin/Qukeys.cpp +++ b/plugins/Kaleidoscope-Qukeys/src/kaleidoscope/plugin/Qukeys.cpp @@ -88,9 +88,6 @@ EventHandlerResult Qukeys::afterEachCycle() { return EventHandlerResult::OK; } - // Process as many events as we can from the queue. - while (processQueue()); - // If we get here, that means that the first event in the queue is a qukey // press. All that's left to do is to check if it's been held long enough that // it has timed out. @@ -101,6 +98,10 @@ EventHandlerResult Qukeys::afterEachCycle() { queue_head_.primary_key : queue_head_.alternate_key; flushEvent(event_key); } + + // Process as many events as we can from the queue. + while (processQueue()); + return EventHandlerResult::OK; }