diff --git a/src/system/SystemPacketBuffer.cpp b/src/system/SystemPacketBuffer.cpp index 59e1e822901db0..f8dda8b01ab84e 100644 --- a/src/system/SystemPacketBuffer.cpp +++ b/src/system/SystemPacketBuffer.cpp @@ -508,37 +508,64 @@ 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(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(aAvailableSize) + static_cast(aReservedSize) + + static_cast(PacketBuffer::kStructureSize); + uint64_t sumOfAvailAndReserved = static_cast(aAvailableSize) + static_cast(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(sumOfSizes)) + { + ChipLogError(chipSystemLayer, + "PacketBuffer: Sizes of allocation request are invalid. (aAvailableSize = " ChipLogFormatX64 + ", aReservedSize = " ChipLogFormatX64 ")", + ChipLogValueX64(static_cast(aAvailableSize)), ChipLogValueX64(static_cast(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(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( pbuf_alloc(PBUF_RAW, static_cast(lAllocSize), CHIP_SYSTEM_PACKETBUFFER_LWIP_PBUF_TYPE)); @@ -546,7 +573,6 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese #elif CHIP_SYSTEM_PACKETBUFFER_FROM_CHIP_POOL - static_cast(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(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(sumOfSizes); + lPacket = reinterpret_cast(chip::Platform::MemoryAlloc(lBlockSize)); SYSTEM_STATS_INCREMENT(chip::System::Stats::kSystemLayer_NumPacketBufs); #else