From d43c6d4939161a2250c086c6f92d951a3003dc61 Mon Sep 17 00:00:00 2001 From: jackctj117 Date: Fri, 20 Mar 2026 15:41:22 -0600 Subject: [PATCH] Fix integer overflow-prone offset+len bounds checks in NVM read handlers --- src/wh_nvm_flash.c | 10 ++++++---- src/wh_server_nvm.c | 9 +++++---- test/wh_test_clientserver.c | 23 +++++++++++++++++++++++ 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/src/wh_nvm_flash.c b/src/wh_nvm_flash.c index 3d83ab476..49ab385bb 100644 --- a/src/wh_nvm_flash.c +++ b/src/wh_nvm_flash.c @@ -500,7 +500,8 @@ static int nfPartition_CheckDataRange(whNvmFlashContext* context, return WH_ERROR_BADARGS; } - if (byte_offset + byte_count > maxOffset) { + /* Use overflow-safe comparison */ + if (byte_offset > maxOffset || byte_count > maxOffset - byte_offset) { return WH_ERROR_BADARGS; } @@ -665,13 +666,14 @@ static int nfObject_ReadDataBytes(whNvmFlashContext* context, int partition, start = context->directory.objects[object_index].state.start; startOffset = nfPartition_DataOffset(context, partition) + start; - /* object bounds checks, do both to avoid integer overflow checks */ + /* object bounds checks, use overflow-safe comparison */ if (byte_offset >= (context->directory.objects[object_index].metadata.len)) { return WH_ERROR_BADARGS; } - if ((byte_offset + byte_count) > - (context->directory.objects[object_index].metadata.len)) { + if (byte_count > + (uint32_t)(context->directory.objects[object_index].metadata.len) - + byte_offset) { return WH_ERROR_BADARGS; } diff --git a/src/wh_server_nvm.c b/src/wh_server_nvm.c index 9c675b62a..c8e20efab 100644 --- a/src/wh_server_nvm.c +++ b/src/wh_server_nvm.c @@ -67,8 +67,8 @@ static int _HandleNvmRead(whServerContext* server, uint8_t* out_data, if (offset >= meta.len) return WH_ERROR_BADARGS; - /* Clamp length to object size */ - if ((offset + len) > meta.len) { + /* Clamp length to object size, use overflow-safe comparison */ + if (len > meta.len - offset) { len = meta.len - offset; } @@ -427,8 +427,9 @@ int wh_Server_HandleNvmRequest(whServerContext* server, if (rc == 0) { read_len = req.data_len; - /* Clamp length to object size */ - if ((req.offset + read_len) > meta.len) { + /* Clamp length to object size, use overflow-safe + * comparison */ + if (read_len > meta.len - req.offset) { read_len = meta.len - req.offset; } } diff --git a/test/wh_test_clientserver.c b/test/wh_test_clientserver.c index aece93d66..f9a425682 100644 --- a/test/wh_test_clientserver.c +++ b/test/wh_test_clientserver.c @@ -562,6 +562,29 @@ static int _testOutOfBoundsNvmReads(whClientContext* client, wh_Client_NvmReadResponse(client, &server_rc, &len, buffer)); WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_BADARGS); + /* Test with large offset (UINT16_MAX), should fail since offset >= + * meta.len. Regression test for integer overflow safety in the + * offset+len check */ + off = UINT16_MAX; + len = 1; + WH_TEST_RETURN_ON_FAIL(wh_Client_NvmReadRequest(client, id, off, len)); + WH_TEST_RETURN_ON_FAIL(wh_Server_HandleRequestMessage(server)); + WH_TEST_RETURN_ON_FAIL( + wh_Client_NvmReadResponse(client, &server_rc, &len, buffer)); + WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_BADARGS); + + /* Test clamping with offset at midpoint and len exceeding remaining object + * size. Verifies the overflow-safe comparison (len > meta.len - offset) + * correctly clamps when offset + len would exceed meta.len */ + off = meta.len / 2; + len = meta.len; + WH_TEST_RETURN_ON_FAIL(wh_Client_NvmReadRequest(client, id, off, len)); + WH_TEST_RETURN_ON_FAIL(wh_Server_HandleRequestMessage(server)); + WH_TEST_RETURN_ON_FAIL( + wh_Client_NvmReadResponse(client, &server_rc, &len, buffer)); + WH_TEST_ASSERT_RETURN(server_rc == WH_ERROR_OK); + WH_TEST_ASSERT_RETURN(len == meta.len - meta.len / 2); + return WH_ERROR_OK; }