Fixes and additions of some of the bounds checks in SystemPacketBuffer::New()

src/system/SystemPacketBuffer.cpp

@@ -508,45 +508,71 @@ void PacketBuffer::AddRef()
 
 PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aReservedSize)
 {
-    // Adding three 16-bit-int sized numbers together will never overflow
-    // assuming int is at least 32 bits.
-    static_assert(INT_MAX >= INT32_MAX, "int is not big enough");
+    // Sanity check for kStructureSize to ensure that it matches the PacketBuffer size.
     static_assert(PacketBuffer::kStructureSize == sizeof(PacketBuffer), "PacketBuffer size mismatch");
-    static_assert(PacketBuffer::kStructureSize < UINT16_MAX, "Check for overflow more carefully");
-    static_assert(SIZE_MAX >= INT_MAX, "Our additions might not fit in size_t");
-    static_assert(PacketBuffer::kMaxSizeWithoutReserve <= UINT32_MAX, "PacketBuffer may have size not fitting uint32_t");
+    // Setting a static upper bound on kStructureSize to ensure the summation of all the sizes does not overflow.
+    static_assert(PacketBuffer::kStructureSize <= UINT16_MAX, "kStructureSize should not exceed UINT16_MAX.");
+    // Setting a static upper bound on the maximum buffer size allocation for regular sized messages (not large).
+    static_assert(PacketBuffer::kMaxSizeWithoutReserve <= UINT16_MAX, "kMaxSizeWithoutReserve should not exceed UINT16_MAX.");
+
+    // Ensure that aAvailableSize is bound within a max and is not big enough to cause overflow during
+    // subsequent addition of all the sizes.
+    if (aAvailableSize > UINT32_MAX)
+    {
+        ChipLogError(chipSystemLayer,
+                     "PacketBuffer: AvailableSize of a buffer cannot exceed UINT32_MAX. aAvailableSize = 0x" ChipLogFormatX64,
+                     ChipLogValueX64(static_cast<uint64_t>(aAvailableSize)));
+        return PacketBufferHandle();
+    }
+
+    // Cast all to uint64_t and add. This cannot overflow because we have
+    // ensured that the maximal value of the summation is
+    // UINT32_MAX + UINT16_MAX + UINT16_MAX, which should always fit in
+    // a uint64_t variable.
+    uint64_t sumOfSizes = static_cast<uint64_t>(aAvailableSize) + static_cast<uint64_t>(aReservedSize) +
+        static_cast<uint64_t>(PacketBuffer::kStructureSize);
+    uint64_t sumOfAvailAndReserved = static_cast<uint64_t>(aAvailableSize) + static_cast<uint64_t>(aReservedSize);
+
+    // Ensure that the sum fits in a size_t so that casting into size_t variables,
+    // viz., lBlockSize and lAllocSize, is safe.
+    if (!CanCastTo<size_t>(sumOfSizes))
+    {
+        ChipLogError(chipSystemLayer,
+                     "PacketBuffer: Sizes of allocation request are invalid. (aAvailableSize = " ChipLogFormatX64
+                     ", aReservedSize = " ChipLogFormatX64 ")",
+                     ChipLogValueX64(static_cast<uint64_t>(aAvailableSize)), ChipLogValueX64(static_cast<uint64_t>(aReservedSize)));
+        return PacketBufferHandle();
+    }
+
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
     // LwIP based APIs have a maximum buffer size of UINT16_MAX. Ensure that
     // limit is met during allocation.
-    VerifyOrDieWithMsg(aAvailableSize + aReservedSize < UINT16_MAX, chipSystemLayer,
-                       "LwIP based systems can handle only up to UINT16_MAX!");
+    VerifyOrDieWithMsg(sumOfAvailAndReserved < UINT16_MAX, chipSystemLayer, "LwIP based systems can handle only up to UINT16_MAX!");
 #endif // CHIP_SYSTEM_CONFIG_USE_LWIP
 
-    // When `aAvailableSize` fits in uint16_t (as tested below) and size_t is at least 32 bits (as asserted above),
-    // these additions will not overflow.
-    const size_t lAllocSize = aReservedSize + aAvailableSize;
-    const size_t lBlockSize = PacketBuffer::kStructureSize + lAllocSize;
+    // sumOfAvailAndReserved is no larger than sumOfSizes, which we checked can be cast to
+    // size_t.
+    const size_t lAllocSize = static_cast<size_t>(sumOfAvailAndReserved);
     PacketBuffer * lPacket;
 
     CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_PacketBufferNew, return PacketBufferHandle());
 
-    // TODO: Change the max to a lower value
-    if (aAvailableSize > UINT32_MAX || lAllocSize > PacketBuffer::kMaxSizeWithoutReserve || lBlockSize > UINT32_MAX)
+    if (lAllocSize > PacketBuffer::kMaxSizeWithoutReserve)
     {
-        ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large.");
+        ChipLogError(chipSystemLayer, "PacketBuffer: allocation exceeding buffer capacity limits.");
         return PacketBufferHandle();
     }
 
 #if CHIP_SYSTEM_CONFIG_USE_LWIP
-
+    // This cast is safe because lAllocSize is no larger than
+    // kMaxSizeWithoutReserve, which fits in uint16_t.
     lPacket = static_cast<PacketBuffer *>(
         pbuf_alloc(PBUF_RAW, static_cast<uint16_t>(lAllocSize), CHIP_SYSTEM_PACKETBUFFER_LWIP_PBUF_TYPE));
 
     SYSTEM_STATS_UPDATE_LWIP_PBUF_COUNTS();
 
 #elif CHIP_SYSTEM_PACKETBUFFER_FROM_CHIP_POOL
 
-    static_cast<void>(lBlockSize);
 #if !CHIP_SYSTEM_CONFIG_NO_LOCKING && CHIP_SYSTEM_CONFIG_FREERTOS_LOCKING
     if (!sBufferPoolMutex.isInitialized())
     {
@@ -565,8 +591,10 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
     UNLOCK_BUF_POOL();
 
 #elif CHIP_SYSTEM_PACKETBUFFER_FROM_CHIP_HEAP
-
-    lPacket = reinterpret_cast<PacketBuffer *>(chip::Platform::MemoryAlloc(lBlockSize));
+    // sumOfSizes is essentially (kStructureSize + lAllocSize) which we already
+    // checked to fit in a size_t.
+    const size_t lBlockSize = static_cast<size_t>(sumOfSizes);
+    lPacket                 = reinterpret_cast<PacketBuffer *>(chip::Platform::MemoryAlloc(lBlockSize));
     SYSTEM_STATS_INCREMENT(chip::System::Stats::kSystemLayer_NumPacketBufs);
 
 #else