common: motion_sense_fifo: fix iteration bug

Fix potential bug when iterating over FIFO entries for commit.
This bug can manifest when non-timestamp/non-data entries are
inserted into the FIFO.

TEST=Added unit tests
BRANCH=None
BUG=b:138303429,chromium:1023858

Change-Id: I64855db2141f7ad6aafb3f3a84632a1dabef11f4
Signed-off-by: Yuval Peress <peress@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/2012846
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>

2 files changed, 54 insertions, 2 deletions

diff --git a/common/motion_sense_fifo.c b/common/motion_sense_fifo.c
index 3ae4317ed2..ee2ab70a32 100644
--- a/common/motion_sense_fifo.c
+++ b/common/motion_sense_fifo.c
@@ -13,6 +13,7 @@
 #include "util.h"
 #include "math_util.h"
 #include "online_calibration.h"
+#include "stdbool.h"
 
 #define CPRINTS(format, args...) cprints(CC_MOTION_SENSE, format, ## args)
 
@@ -79,6 +80,18 @@ static inline int is_timestamp(
 }
 
 /**
+ * Check whether or not a given sensor data entry contains sensor data or not.
+ *
+ * @param data The data entry to check.
+ * @return True if the entry contains data, false otherwise.
+ */
+static inline bool is_data(const struct ec_response_motion_sensor_data *data)
+{
+	return (data->flags & (MOTIONSENSE_SENSOR_FLAG_TIMESTAMP |
+			       MOTIONSENSE_SENSOR_FLAG_ODR)) == 0;
+}
+
+/**
  * Convenience function to get the head of the fifo. This function makes no
  * guarantee on whether or not the entry is valid.
  *
@@ -468,14 +481,23 @@ commit_data_end:
 		if (data->flags & MOTIONSENSE_SENSOR_FLAG_WAKEUP)
 			wake_up_needed = 1;
 
-		/* Skip timestamp, we don't know the sensor number yet. */
-		if (is_timestamp(data))
+		/*
+		 * Skip non-data entries, we don't know the sensor number yet.
+		 */
+		if (!is_data(data))
 			continue;
 
 		/* Get the sensor number and point to the timestamp entry. */
 		sensor_num = data->sensor_num;
 		data = peek_fifo_staged(i - 1);
 
+		/* Verify we're pointing at a timestamp. */
+		if (!is_timestamp(data)) {
+			CPRINTS("FIFO entries out of order,"
+				" expected timestamp");
+			continue;
+		}
+
 		/*
 		 * If this is the first time we're seeing a timestamp for this
 		 * sensor or the timestamp is after our computed next, skip
diff --git a/test/motion_sense_fifo.c b/test/motion_sense_fifo.c
index 9762fa61c9..bc678b00cb 100644
--- a/test/motion_sense_fifo.c
+++ b/test/motion_sense_fifo.c
@@ -314,6 +314,35 @@ static int test_spread_double_commit_same_timestamp(void)
 	return EC_SUCCESS;
 }
 
+static int test_commit_non_data_or_timestamp_entries(void)
+{
+	const uint32_t now = __hw_clock_source_read();
+	int read_count;
+
+	motion_sensors[0].oversampling_ratio = 1;
+	motion_sensors[0].collection_rate = 20000; /* ns */
+
+	/* Insert non-data entry */
+	data[0].flags = MOTIONSENSE_SENSOR_FLAG_ODR;
+	motion_sense_fifo_stage_data(data, motion_sensors, 3, now - 20500);
+
+	/* Insert data entry */
+	data[0].flags = 0;
+	motion_sense_fifo_stage_data(data, motion_sensors, 3, now - 20500);
+
+	motion_sense_fifo_commit_data();
+	read_count = motion_sense_fifo_read(
+		sizeof(data), CONFIG_ACCEL_FIFO_SIZE, data, &data_bytes_read);
+	TEST_EQ(read_count, 4, "%d");
+	TEST_BITS_SET(data[0].flags, MOTIONSENSE_SENSOR_FLAG_TIMESTAMP);
+	TEST_EQ(data[0].timestamp, now - 20500, "%u");
+	TEST_BITS_SET(data[1].flags, MOTIONSENSE_SENSOR_FLAG_ODR);
+	TEST_BITS_SET(data[2].flags, MOTIONSENSE_SENSOR_FLAG_TIMESTAMP);
+	TEST_EQ(data[2].timestamp, now - 20500, "%u");
+
+	return EC_SUCCESS;
+}
+
 void before_test(void)
 {
 	motion_sense_fifo_commit_data();
@@ -342,6 +371,7 @@ void run_test(void)
 	RUN_TEST(test_spread_data_in_window);
 	RUN_TEST(test_spread_data_by_collection_rate);
 	RUN_TEST(test_spread_double_commit_same_timestamp);
+	RUN_TEST(test_commit_non_data_or_timestamp_entries);
 
 	test_print_result();
 }