commit f0e4adc963ff62d226d94ff5fb9591fe3eb1a676
author Brandon Kim <brandonkim@google.com> Thu Jun 16 23:14:25 2022 -0700
committer Brandon Kim <brandonkim@google.com> Fri Jun 17 00:08:48 2022 -0700
tree 31b06167ec6cb19f6d6acd35caa94035419ebc0e
parent 35d4335eda4c41bc5c658c8f1d36190f0e7e625a

buffer: bugfix (v3) fix the checksum

The BIOS is expected to use the XOR checksum which behaves ever so
slightly different compared to summing up the bytes (even though
bitwise XOR is the same as addition).

Fixed the unit test accordingly

Tested: Unit test + tested on a BIOS with real / valid checksummed
payload

Signed-off-by: Brandon Kim <brandonkim@google.com>
Change-Id: I326f814eb66892971ec32f0572618a34825d88b0

include/buffer.hpp

diff --git a/include/buffer.hpp b/include/buffer.hpp
index d86f204..d42b843 100644
--- a/include/buffer.hpp
+++ b/include/buffer.hpp
@@ -165,6 +165,11 @@
      *  @return relative offset for read and write pointers
      */
     size_t getQueueOffset();
+    /** @brief Calculate the checksum by XOR each bytes in the span
+     *  @param[in] entry     - Span to calculate the checksum on
+     *  @return calculated checksum
+     */
+    uint8_t calculateChecksum(std::span<uint8_t> entry);
 
     std::unique_ptr<DataInterface> dataInterface;
     struct CircularBufferHeader cachedBufferHeader = {};

src/buffer.cpp

diff --git a/src/buffer.cpp b/src/buffer.cpp
index 35a6f77..836049f 100644
--- a/src/buffer.cpp
+++ b/src/buffer.cpp
@@ -211,9 +211,12 @@
 
     // Calculate the checksum
     uint8_t* entryHeaderPtr = reinterpret_cast<uint8_t*>(&entryHeader);
-    uint8_t checksum = std::accumulate(
-        entryHeaderPtr, entryHeaderPtr + sizeof(struct QueueEntryHeader), 0);
-    checksum = std::accumulate(std::begin(entry), std::end(entry), checksum);
+    uint8_t checksum =
+        std::accumulate(entryHeaderPtr,
+                        entryHeaderPtr + sizeof(struct QueueEntryHeader), 0,
+                        std::bit_xor<void>()) ^
+        std::accumulate(entry.begin(), entry.end(), 0, std::bit_xor<void>());
+
     if (checksum != 0)
     {
         throw std::runtime_error(fmt::format(

test/buffer_test.cpp

diff --git a/test/buffer_test.cpp b/test/buffer_test.cpp
index 2c928d8..6f886f0 100644
--- a/test/buffer_test.cpp
+++ b/test/buffer_test.cpp
@@ -427,9 +427,10 @@
     static constexpr size_t entryHeaderSize = sizeof(struct QueueEntryHeader);
     static constexpr uint16_t testSequenceId = 0;
     static constexpr uint16_t testEntrySize = 0x20;
-    // Calculated checksum for the header (0x100 - 0 - 0x20 - 0x01) & 0xff
-    static constexpr uint8_t testChecksum = 0xdf;
     static constexpr uint8_t testRdeCommandType = 0x01;
+    // Calculated checksum for the header
+    static constexpr uint8_t testChecksum =
+        (testSequenceId ^ testEntrySize ^ testRdeCommandType);
     size_t testOffset = 0x20;
 
     struct QueueEntryHeader testEntryHeader
@@ -453,10 +454,9 @@
 TEST_F(BufferEntryTest, ReadEntryChecksumFail)
 {
     InSequence s;
-    // We expect this will bump checksum up by "testEntrySize" = 0x20
-    std::vector<uint8_t> testEntryVector(testEntrySize, 1);
+    std::vector<uint8_t> testEntryVector(testEntrySize);
     // Offset the checksum by 1
-    testEntryHeader.checksum -= (0x20 - 1);
+    testEntryHeader.checksum += 1;
     uint8_t* testEntryHeaderPtr = reinterpret_cast<uint8_t*>(&testEntryHeader);
     std::vector<uint8_t> testEntryHeaderVector(
         testEntryHeaderPtr, testEntryHeaderPtr + entryHeaderSize);
@@ -466,8 +466,9 @@
         try {
             bufferImpl->readEntry(testOffset);
         } catch (const std::runtime_error& e) {
+            // Calculation: testChecksum (0x21) XOR (0x22) = 3
             EXPECT_STREQ(e.what(),
-                         "[readEntry] Checksum was '1', expected '0'");
+                         "[readEntry] Checksum was '3', expected '0'");
             throw;
         },
         std::runtime_error);
@@ -476,10 +477,9 @@
 TEST_F(BufferEntryTest, ReadEntryPass)
 {
     InSequence s;
-    // We expect this will bump checksum up by "testEntrySize" = 0xff * 0x20 =
-    // 0x1fe0 -> 0x1fe0 & 0xff = 0xe0.
+    // We expect this will bump checksum up by "testEntrySize" = 0xff ^ 0xff ...
+    // (20 times) = 0 therefore leave the checksum as is
     std::vector<uint8_t> testEntryVector(testEntrySize, 0xff);
-    testEntryHeader.checksum -= (0xe0);
     uint8_t* testEntryHeaderPtr = reinterpret_cast<uint8_t*>(&testEntryHeader);
     std::vector<uint8_t> testEntryHeaderVector(
         testEntryHeaderPtr, testEntryHeaderPtr + entryHeaderSize);