From 5468b1c47714e0cf9e14eb147e0b6e33e0bf3e1a Mon Sep 17 00:00:00 2001 From: Pradip De Date: Sat, 11 May 2024 14:12:27 -0700 Subject: [PATCH] Fixes and additions of some of the bounds checks in SystemPacketBuffer::New(). With the data length types changed to size_t, the bounds checks for the lengths need to be appropriately modified so that we ensure that overflow does not happen and the buffer allocation is performed correctly as requested by the caller of the API. --- src/system/SystemPacketBuffer.cpp | 66 ++++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 19 deletions(-) diff --git a/src/system/SystemPacketBuffer.cpp b/src/system/SystemPacketBuffer.cpp index 59e1e822901db0..5796ead56c6ac1 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 do 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 addtion 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