fix(storage/nvs): Fixed hadling of inconsistent values in NVS entry header

feat(storage/nvs): Added test cases for damaged entries with correct CRC

components/nvs_flash/src/nvs_page.cpp

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -10,8 +10,7 @@
 #include <cstring>
 #include "nvs_internal.h"
 
-namespace nvs
+namespace nvs {
-{
 
 Page::Page() : mPartition(nullptr) { }
 
@@ -46,7 +45,9 @@ esp_err_t Page::load(Partition *partition, uint32_t sectorNumber)
         const int BLOCK_SIZE = 128;
         uint32_t* block = new (std::nothrow) uint32_t[BLOCK_SIZE];
 
-        if (!block) return ESP_ERR_NO_MEM;
+        if (!block) {
+            return ESP_ERR_NO_MEM;
+        }
 
         for (uint32_t i = 0; i < SPI_FLASH_SEC_SIZE; i += 4 * BLOCK_SIZE) {
             rc = mPartition->read_raw(mBaseAddress + i, block, 4 * BLOCK_SIZE);
@@ -67,7 +68,7 @@ esp_err_t Page::load(Partition *partition, uint32_t sectorNumber)
     } else {
         mState = header.mState;
         mSeqNumber = header.mSeqNumber;
-        if(header.mVersion < NVS_VERSION) {
+        if (header.mVersion < NVS_VERSION) {
             return ESP_ERR_NVS_NEW_VERSION_FOUND;
         } else {
             mVersion = header.mVersion;
@@ -92,7 +93,7 @@ esp_err_t Page::load(Partition *partition, uint32_t sectorNumber)
     return ESP_OK;
 }
 
-esp_err_t Page::writeEntry(const Item& item)
+esp_err_t Page::writeEntry(const Item &item)
 {
     uint32_t phyAddr;
     esp_err_t err = getEntryAddress(mNextFreeEntry, &phyAddr);
@@ -101,7 +102,6 @@ esp_err_t Page::writeEntry(const Item& item)
     }
     err = mPartition->write(phyAddr, &item, sizeof(item));
 
-
     if (err != ESP_OK) {
         mState = PageState::INVALID;
         return err;
@@ -283,12 +283,12 @@ esp_err_t Page::readItem(uint8_t nsIndex, ItemType datatype, const char* key, vo
             return rc;
         }
         size_t willCopy = ENTRY_SIZE;
-        willCopy = (left < willCopy)?left:willCopy;
+        willCopy = (left < willCopy) ? left : willCopy;
         memcpy(dst, ditem.rawData, willCopy);
         left -= willCopy;
         dst += willCopy;
     }
-    if (Item::calculateCrc32(reinterpret_cast<uint8_t*>(data), item.varLength.dataSize) != item.varLength.dataCrc32) {
+    if (Item::calculateCrc32(reinterpret_cast<uint8_t * >(data), item.varLength.dataSize) != item.varLength.dataCrc32) {
         rc = eraseEntryAndSpan(index);
         if (rc != ESP_OK) {
             return rc;
@@ -336,14 +336,14 @@ esp_err_t Page::cmpItem(uint8_t nsIndex, ItemType datatype, const char* key, con
             return rc;
         }
         size_t willCopy = ENTRY_SIZE;
-        willCopy = (left < willCopy)?left:willCopy;
+        willCopy = (left < willCopy) ? left : willCopy;
         if (memcmp(dst, ditem.rawData, willCopy)) {
             return ESP_ERR_NVS_CONTENT_DIFFERS;
         }
         left -= willCopy;
         dst += willCopy;
     }
-    if (Item::calculateCrc32(reinterpret_cast<const uint8_t*>(data), item.varLength.dataSize) != item.varLength.dataCrc32) {
+    if (Item::calculateCrc32(reinterpret_cast<const uint8_t * >(data), item.varLength.dataSize) != item.varLength.dataCrc32) {
         return ESP_ERR_NVS_NOT_FOUND;
     }
 
@@ -386,7 +386,7 @@ esp_err_t Page::eraseEntryAndSpan(size_t index)
         if (rc != ESP_OK) {
             return rc;
         }
-        if (item.calculateCrc32() != item.crc32) {
+        if (!item.checkHeaderConsistency(index)) {
             mHashList.erase(index);
             rc = alterEntryState(index, EntryState::ERASED);
             --mUsedEntryCount;
@@ -460,7 +460,7 @@ esp_err_t Page::updateFirstUsedEntry(size_t index, size_t span)
     return ESP_OK;
 }
 
-esp_err_t Page::copyItems(Page& other)
+esp_err_t Page::copyItems(Page &other)
 {
     if (mFirstUsedEntry == INVALID_ENTRY) {
         return ESP_ERR_NVS_NOT_FOUND;
@@ -508,7 +508,10 @@ esp_err_t Page::copyItems(Page& other)
         NVS_ASSERT_OR_RETURN(end <= ENTRY_COUNT, ESP_FAIL);
 
         for (size_t i = readEntryIndex + 1; i < end; ++i) {
-            readEntry(i, entry);
+            err = readEntry(i, entry);
+            if (err != ESP_OK) {
+                return err;
+            }
             err = other.writeEntry(entry);
             if (err != ESP_OK) {
                 return err;
@@ -599,8 +602,7 @@ esp_err_t Page::mLoadEntryTable()
                     --mUsedEntryCount;
                 }
                 ++mErasedEntryCount;
-            }
+            } else {
-            else {
                 break;
             }
         }
@@ -642,7 +644,7 @@ esp_err_t Page::mLoadEntryTable()
                 return err;
             }
 
-            if (item.crc32 != item.calculateCrc32()) {
+            if (!item.checkHeaderConsistency(i)) {
                 err = eraseEntryAndSpan(i);
                 if (err != ESP_OK) {
                     mState = PageState::INVALID;
@@ -722,7 +724,7 @@ esp_err_t Page::mLoadEntryTable()
                 return err;
             }
 
-            if (item.crc32 != item.calculateCrc32()) {
+            if (!item.checkHeaderConsistency(i)) {
                 err = eraseEntryAndSpan(i);
                 if (err != ESP_OK) {
                     mState = PageState::INVALID;
@@ -762,7 +764,6 @@ esp_err_t Page::mLoadEntryTable()
     return ESP_OK;
 }
 
-
 esp_err_t Page::initialize()
 {
     NVS_ASSERT_OR_RETURN(mState == PageState::UNINITIALIZED, ESP_FAIL);
@@ -810,7 +811,7 @@ esp_err_t Page::alterEntryRangeState(size_t begin, size_t end, EntryState state)
     esp_err_t err;
     for (ptrdiff_t i = end - 1; i >= static_cast<ptrdiff_t>(begin); --i) {
         err = mEntryTable.set(i, state);
-        if (err != ESP_OK){
+        if (err != ESP_OK) {
             return err;
         }
         size_t nextWordIndex;
@@ -844,7 +845,7 @@ esp_err_t Page::alterPageState(PageState state)
     return ESP_OK;
 }
 
-esp_err_t Page::readEntry(size_t index, Item& dst) const
+esp_err_t Page::readEntry(size_t index, Item &dst) const
 {
     uint32_t phyAddr;
     esp_err_t rc = getEntryAddress(index, &phyAddr);
@@ -858,7 +859,7 @@ esp_err_t Page::readEntry(size_t index, Item& dst) const
     return ESP_OK;
 }
 
-esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, size_t &itemIndex, Item& item, uint8_t chunkIdx, VerOffset chunkStart)
+esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, size_t &itemIndex, Item &item, uint8_t chunkIdx, VerOffset chunkStart)
 {
     if (mState == PageState::CORRUPT || mState == PageState::INVALID || mState == PageState::UNINITIALIZED) {
         return ESP_ERR_NVS_NOT_FOUND;
@@ -910,8 +911,7 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si
             return rc;
         }
 
-        auto crc32 = item.calculateCrc32();
+        if (!item.checkHeaderConsistency(i)) {
-        if (item.crc32 != crc32) {
             rc = eraseEntryAndSpan(i);
             if (rc != ESP_OK) {
                 mState = PageState::INVALID;
@@ -975,7 +975,6 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si
             continue;
         }
 
-
         if (datatype != ItemType::ANY && item.datatype != datatype) {
             if (key == nullptr && nsIndex == NS_ANY && chunkIdx == CHUNK_ANY) {
                 continue; // continue for bruteforce search on blob indices.
@@ -992,7 +991,7 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si
     return ESP_ERR_NVS_NOT_FOUND;
 }
 
-esp_err_t Page::getSeqNumber(uint32_t& seqNumber) const
+esp_err_t Page::getSeqNumber(uint32_t &seqNumber) const
 {
     if (mState != PageState::UNINITIALIZED && mState != PageState::INVALID && mState != PageState::CORRUPT) {
         seqNumber = mSeqNumber;
@@ -1001,7 +1000,6 @@ esp_err_t Page::getSeqNumber(uint32_t& seqNumber) const
     return ESP_ERR_NVS_NOT_INITIALIZED;
 }
 
-
 esp_err_t Page::setSeqNumber(uint32_t seqNumber)
 {
     if (mState != PageState::UNINITIALIZED) {
@@ -1060,7 +1058,7 @@ size_t Page::getVarDataTailroom() const
         return 0;
     }
     /* Skip one entry for blob data item processing the data */
-    return ((mNextFreeEntry < (ENTRY_COUNT-1)) ? ((ENTRY_COUNT - mNextFreeEntry - 1) * ENTRY_SIZE): 0);
+    return ((mNextFreeEntry < (ENTRY_COUNT - 1)) ? ((ENTRY_COUNT - mNextFreeEntry - 1) * ENTRY_SIZE) : 0);
 }
 
 const char* Page::pageStateToName(PageState ps)
@@ -1111,7 +1109,7 @@ void Page::debugDump() const
             readEntry(i, item);
             if (skip == 0) {
                 printf("W ns=%2" PRIu8 " type=%2" PRIu8 " span=%3" PRIu8 " key=\"%s\" chunkIdx=%" PRIu8 " len=%" PRIi32 "\n",
-                    item.nsIndex, static_cast<uint8_t>(item.datatype), item.span, item.key, item.chunkIndex, (item.span != 1)?(static_cast<int32_t>(item.varLength.dataSize)):(-1));
+                       item.nsIndex, static_cast<uint8_t>(item.datatype), item.span, item.key, item.chunkIndex, (item.span != 1) ? (static_cast<int32_t>(item.varLength.dataSize)) : (-1));
                 if (item.span > 0 && item.span <= ENTRY_COUNT - i) {
                     skip = item.span - 1;
                 } else {

components/nvs_flash/src/nvs_types.cpp

@@ -1,22 +1,16 @@
-// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD
+/*
-//
+ * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
-// Licensed under the Apache License, Version 2.0 (the "License");
+ *
-// you may not use this file except in compliance with the License.
+ * SPDX-License-Identifier: Apache-2.0
-// You may obtain a copy of the License at
+ */
-
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
 #include "nvs_types.hpp"
-
+#include "nvs_page.hpp"
+#include "esp_log.h"
 #include "esp_rom_crc.h"
 
-namespace nvs
+#define TAG "nvs"
-{
+
+namespace nvs {
 uint32_t Item::calculateCrc32() const
 {
     uint32_t result = 0xffffffff;
@@ -46,4 +40,114 @@ uint32_t Item::calculateCrc32(const uint8_t* data, size_t size)
     return result;
 }
 
+bool Item::checkHeaderConsistency(const uint8_t entryIndex) const
+{
+    // calculate and check the crc32
+    if (crc32 != calculateCrc32()) {
+        ESP_LOGD(TAG, "CRC32 mismatch for entry %d", entryIndex);
+        return false;
+    }
+
+    // validate the datatype and check the rest of the header fields
+    switch (datatype) {
+    // Entries occupying just one entry
+    case ItemType::U8:
+    case ItemType::I8:
+    case ItemType::U16:
+    case ItemType::I16:
+    case ItemType::U32:
+    case ItemType::I32:
+    case ItemType::U64:
+    case ItemType::I64: {
+        if (span != 1) {
+            ESP_LOGD(TAG, "Invalid span %u for datatype %#04x", (unsigned int)span, (unsigned int)datatype);
+            return false;
+        }
+        break;
+    }
+
+    // Special case for BLOB_IDX
+    case ItemType::BLOB_IDX: {
+        // span must be 1
+        if (span != 1) {
+            ESP_LOGD(TAG, "Invalid span %u for BLOB_IDX", (unsigned int)span);
+            return false;
+        }
+
+        // chunkIndex must be CHUNK_ANY
+        if (chunkIndex != CHUNK_ANY) {
+            ESP_LOGD(TAG, "Invalid chunk index %u for BLOB_IDX", (unsigned int)chunkIndex);
+            return false;
+        }
+
+        // check maximum data length
+        // the maximal data length is determined by:
+        // maximum number of chunks. Chunks are stored in uin8_t, but are logically divided into two "VerOffset" ranges of values (0 based and 128 based)
+        // maximum theoretical number of entries in the chunk (Page::ENTRY_COUNT - 1) and the number of bytes entry can store (Page::ENTRY_SIZE)
+        const uint32_t maxDataSize = (uint32_t)((UINT8_MAX / 2) * (Page::ENTRY_COUNT - 1) * Page::ENTRY_SIZE);
+        if (blobIndex.dataSize > maxDataSize) {
+            ESP_LOGD(TAG, "Data size %u bytes exceeds maximum possible size %u bytes for BLOB_IDX", (unsigned int)blobIndex.dataSize, (unsigned int)maxDataSize);
+            return false;
+        }
+        break;
+    }
+
+    // Entries with variable length data
+    case ItemType::SZ:
+    case ItemType::BLOB:
+    case ItemType::BLOB_DATA: {
+        uint16_t maxAvailableVDataSize;
+        uint8_t maxAvailablePageSpan;
+        uint8_t spanCalcFromLen;
+
+        // for BLOB_DATA, chunkIndex must NOT be CHUNK_ANY as this value is used to search ALL chunks in findItem
+        if (datatype == ItemType::BLOB_DATA) {
+            // chunkIndex must not be CHUNK_ANY
+            if (chunkIndex == CHUNK_ANY) {
+                ESP_LOGD(TAG, "Invalid chunk index %u for BLOB_DATA", (unsigned int)chunkIndex);
+                return false;
+            }
+        }
+
+        // variable length and span checks
+
+        //  based on the entryIndex determine the maximum variable length capacity in bytes to the end of the page
+        maxAvailableVDataSize = ((Page::ENTRY_COUNT - entryIndex) - 1) * Page::ENTRY_SIZE;
+
+        // check if the variable data length is not exceeding the maximum capacity available till the end of the page
+        if (varLength.dataSize > maxAvailableVDataSize) {
+            ESP_LOGD(TAG, "Variable data length %u bytes exceeds page boundary. Maximum calculated from the current entry position within page is %u bytes for datatype %#04x ", (unsigned int)varLength.dataSize, (unsigned int)maxAvailableVDataSize, (unsigned int)datatype);
+            return false;
+        }
+
+        // based on the entryIndex determine the maximum possible span up to the end of the page
+        maxAvailablePageSpan = Page::ENTRY_COUNT - entryIndex;
+
+        // this check ensures no data is read beyond the end of the page
+        if (span > maxAvailablePageSpan) {
+            ESP_LOGD(TAG, "Span %u exceeds page boundary. Maximum calculated from the current entry position within page is %u for datatype %#04x ", (unsigned int)span, (unsigned int)maxAvailablePageSpan, (unsigned int)datatype);
+            return false;
+        }
+
+        // here we have both span and varLength.dataSize within the page boundary. Check if these values are consistent
+        spanCalcFromLen = (uint8_t)(((size_t) varLength.dataSize + Page::ENTRY_SIZE - 1) / Page::ENTRY_SIZE);
+        spanCalcFromLen ++; // add overhead entry
+
+        // this check ensures that the span is equal to the number of entries required to store the data plus the overhead entry
+        if (span != spanCalcFromLen) {
+            ESP_LOGD(TAG, "Span %i does not match span %u calculated from variable data length %u bytes for datatype %#04x", (unsigned int)span, (unsigned int)spanCalcFromLen, (unsigned int)varLength.dataSize, (unsigned int)datatype);
+            return false;
+        }
+        break;
+    }
+
+    // Invalid datatype
+    default: {
+        ESP_LOGD(TAG, "Invalid datatype %#04x", (unsigned int)datatype);
+        return false;
+    }
+    }
+    return true;
+}
+
 } // namespace nvs

components/nvs_flash/src/nvs_types.hpp

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -108,6 +108,14 @@ public:
         dst = *reinterpret_cast<T*>(data);
         return ESP_OK;
     }
+
+    // Returns true if item's header:
+    // crc32 matches the calculated crc32
+    // and datatype is one of the supported types
+    // and span is within the allowed range for the datatype and below the maximum calculated from the entryIndex
+    //
+    // Parameter entryIndex is used to calculate the maximum span for the given entry
+    bool checkHeaderConsistency(const uint8_t entryIndex) const;
 };
 
 } // namespace nvs

tools/ci/check_copyright_ignore.txt

@@ -499,7 +499,6 @@ components/nvs_flash/src/nvs_pagemanager.hpp
 components/nvs_flash/src/nvs_partition_lookup.cpp
 components/nvs_flash/src/nvs_partition_lookup.hpp
 components/nvs_flash/src/nvs_test_api.h
-components/nvs_flash/src/nvs_types.cpp
 components/nvs_flash/test_nvs_host/main.cpp
 components/nvs_flash/test_nvs_host/sdkconfig.h
 components/nvs_flash/test_nvs_host/test_intrusive_list.cpp