Skip to content

Commit ccd95c0

Browse files
committed
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.
1 parent 3600a50 commit ccd95c0

File tree

1 file changed

+35
-17
lines changed

1 file changed

+35
-17
lines changed

src/system/SystemPacketBuffer.cpp

+35-17
Original file line numberDiff line numberDiff line change
@@ -508,45 +508,63 @@ void PacketBuffer::AddRef()
508508

509509
PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aReservedSize)
510510
{
511-
// Adding three 16-bit-int sized numbers together will never overflow
512-
// assuming int is at least 32 bits.
511+
// Ensure that SIZE_MAX is at least 32 bits
513512
static_assert(INT_MAX >= INT32_MAX, "int is not big enough");
513+
static_assert(SIZE_MAX >= INT_MAX, "Our additions may not fit in size_t");
514+
515+
// Sanity check for kStructureSize and putting an upper bound.
514516
static_assert(PacketBuffer::kStructureSize == sizeof(PacketBuffer), "PacketBuffer size mismatch");
515-
static_assert(PacketBuffer::kStructureSize < UINT16_MAX, "Check for overflow more carefully");
516-
static_assert(SIZE_MAX >= INT_MAX, "Our additions might not fit in size_t");
517-
static_assert(PacketBuffer::kMaxSizeWithoutReserve <= UINT32_MAX, "PacketBuffer may have size not fitting uint32_t");
517+
static_assert(PacketBuffer::kStructureSize < UINT16_MAX, "kStructureSize should not exceed UINT16_MAX");
518+
static_assert(PacketBuffer::kMaxSizeWithoutReserve <= UINT16_MAX, "PacketBuffer may have size not fitting uint16_t");
519+
520+
// Have a bounds check for aAvailableSize to ensure that
521+
// the next overflow check is done correctly.
522+
if (aAvailableSize > UINT32_MAX)
523+
{
524+
ChipLogError(chipSystemLayer, "PacketBuffer: AvailableSize of a buffer cannot exceed UINT32_MAX");
525+
return PacketBufferHandle();
526+
}
527+
528+
// Cast all to uint64_t and add to consider the widest possible result.
529+
uint64_t sumOfSizes = static_cast<uint64_t>(aAvailableSize) + static_cast<uint64_t>(aReservedSize) +
530+
static_cast<uint64_t>(PacketBuffer::kStructureSize);
531+
uint64_t sumOfAvailAndReserved = static_cast<uint64_t>(aAvailableSize) + static_cast<uint64_t>(aReservedSize);
532+
533+
// Ensure that the sum does not overflow a size_t whose max value is at
534+
// least INT_MAX.
535+
if (sumOfSizes > std::numeric_limits<size_t>::max())
536+
{
537+
ChipLogError(chipSystemLayer, "PacketBuffer: Sizes of allocation request are invalid");
538+
return PacketBufferHandle();
539+
}
540+
518541
#if CHIP_SYSTEM_CONFIG_USE_LWIP
519542
// LwIP based APIs have a maximum buffer size of UINT16_MAX. Ensure that
520543
// limit is met during allocation.
521-
VerifyOrDieWithMsg(aAvailableSize + aReservedSize < UINT16_MAX, chipSystemLayer,
522-
"LwIP based systems can handle only up to UINT16_MAX!");
544+
VerifyOrDieWithMsg(sumOfAvailAndReserved < UINT16_MAX, chipSystemLayer, "LwIP based systems can handle only up to UINT16_MAX!");
523545
#endif // CHIP_SYSTEM_CONFIG_USE_LWIP
524546

525-
// When `aAvailableSize` fits in uint16_t (as tested below) and size_t is at least 32 bits (as asserted above),
526-
// these additions will not overflow.
527-
const size_t lAllocSize = aReservedSize + aAvailableSize;
528-
const size_t lBlockSize = PacketBuffer::kStructureSize + lAllocSize;
547+
// The above check against the size_t limit ensures that the below sum will not
548+
// overflow.
549+
const size_t lAllocSize = static_cast<size_t>(sumOfAvailAndReserved);
529550
PacketBuffer * lPacket;
530551

531552
CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_PacketBufferNew, return PacketBufferHandle());
532553

533-
// TODO: Change the max to a lower value
534-
if (aAvailableSize > UINT32_MAX || lAllocSize > PacketBuffer::kMaxSizeWithoutReserve || lBlockSize > UINT32_MAX)
554+
if (lAllocSize > PacketBuffer::kMaxSizeWithoutReserve)
535555
{
536-
ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large.");
556+
ChipLogError(chipSystemLayer, "PacketBuffer: allocation exceeding buffer capacity limits.");
537557
return PacketBufferHandle();
538558
}
539559

540560
#if CHIP_SYSTEM_CONFIG_USE_LWIP
541-
542561
lPacket = static_cast<PacketBuffer *>(
543562
pbuf_alloc(PBUF_RAW, static_cast<uint16_t>(lAllocSize), CHIP_SYSTEM_PACKETBUFFER_LWIP_PBUF_TYPE));
544563

545564
SYSTEM_STATS_UPDATE_LWIP_PBUF_COUNTS();
546565

547566
#elif CHIP_SYSTEM_PACKETBUFFER_FROM_CHIP_POOL
548567

549-
static_cast<void>(lBlockSize);
550568
#if !CHIP_SYSTEM_CONFIG_NO_LOCKING && CHIP_SYSTEM_CONFIG_FREERTOS_LOCKING
551569
if (!sBufferPoolMutex.isInitialized())
552570
{
@@ -565,7 +583,7 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
565583
UNLOCK_BUF_POOL();
566584

567585
#elif CHIP_SYSTEM_PACKETBUFFER_FROM_CHIP_HEAP
568-
586+
const size_t lBlockSize = PacketBuffer::kStructureSize + lAllocSize;
569587
lPacket = reinterpret_cast<PacketBuffer *>(chip::Platform::MemoryAlloc(lBlockSize));
570588
SYSTEM_STATS_INCREMENT(chip::System::Stats::kSystemLayer_NumPacketBufs);
571589

0 commit comments

Comments
 (0)