Skip to content

Commit 8a50f1f

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 6aee3fd commit 8a50f1f

File tree

1 file changed

+47
-19
lines changed

1 file changed

+47
-19
lines changed

src/system/SystemPacketBuffer.cpp

+47-19
Original file line numberDiff line numberDiff line change
@@ -508,45 +508,71 @@ 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.
513-
static_assert(INT_MAX >= INT32_MAX, "int is not big enough");
511+
// Sanity check for kStructureSize to ensure that it matches the PacketBuffer size.
514512
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");
513+
// Setting a static upper bound on kStructureSize to ensure the summation of all the sizes do not overflow.
514+
static_assert(PacketBuffer::kStructureSize <= UINT16_MAX, "kStructureSize should not exceed UINT16_MAX.");
515+
// Setting a static upper bound on the maximum buffer size allocation for regular sized messages(not large).
516+
static_assert(PacketBuffer::kMaxSizeWithoutReserve <= UINT16_MAX, "kMaxSizeWithoutReserve should not exceed UINT16_MAX.");
517+
518+
// Ensure that aAvailableSize is bound within a max and is not big enough to cause overflow during
519+
// subsequent addtion of all the sizes.
520+
if (aAvailableSize > UINT32_MAX)
521+
{
522+
ChipLogError(chipSystemLayer,
523+
"PacketBuffer: AvailableSize of a buffer cannot exceed UINT32_MAX. aAvailableSize = 0x" ChipLogFormatX64,
524+
ChipLogValueX64(static_cast<uint64_t>(aAvailableSize)));
525+
return PacketBufferHandle();
526+
}
527+
528+
// Cast all to uint64_t and add. This cannot overflow because we have
529+
// ensured that the maximal value of the summation is
530+
// UINT32_MAX + UINT16_MAX + UINT16_MAX, which should always fit in
531+
// a uint64_t variable.
532+
uint64_t sumOfSizes = static_cast<uint64_t>(aAvailableSize) + static_cast<uint64_t>(aReservedSize) +
533+
static_cast<uint64_t>(PacketBuffer::kStructureSize);
534+
uint64_t sumOfAvailAndReserved = static_cast<uint64_t>(aAvailableSize) + static_cast<uint64_t>(aReservedSize);
535+
536+
// Ensure that the sum fits in a size_t so that casting into size_t variables,
537+
// viz., lBlockSize and lAllocSize, is safe.
538+
if (!CanCastTo<size_t>(sumOfSizes))
539+
{
540+
ChipLogError(chipSystemLayer,
541+
"PacketBuffer: Sizes of allocation request are invalid. (aAvailableSize = " ChipLogFormatX64
542+
", aReservedSize = " ChipLogFormatX64 ")",
543+
ChipLogValueX64(static_cast<uint64_t>(aAvailableSize)), ChipLogValueX64(static_cast<uint64_t>(aReservedSize)));
544+
return PacketBufferHandle();
545+
}
546+
518547
#if CHIP_SYSTEM_CONFIG_USE_LWIP
519548
// LwIP based APIs have a maximum buffer size of UINT16_MAX. Ensure that
520549
// limit is met during allocation.
521-
VerifyOrDieWithMsg(aAvailableSize + aReservedSize < UINT16_MAX, chipSystemLayer,
522-
"LwIP based systems can handle only up to UINT16_MAX!");
550+
VerifyOrDieWithMsg(sumOfAvailAndReserved < UINT16_MAX, chipSystemLayer, "LwIP based systems can handle only up to UINT16_MAX!");
523551
#endif // CHIP_SYSTEM_CONFIG_USE_LWIP
524552

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;
553+
// sumOfAvailAndReserved is no larger than sumOfSizes, which we checked can be cast to
554+
// size_t.
555+
const size_t lAllocSize = static_cast<size_t>(sumOfAvailAndReserved);
529556
PacketBuffer * lPacket;
530557

531558
CHIP_SYSTEM_FAULT_INJECT(FaultInjection::kFault_PacketBufferNew, return PacketBufferHandle());
532559

533-
// TODO: Change the max to a lower value
534-
if (aAvailableSize > UINT32_MAX || lAllocSize > PacketBuffer::kMaxSizeWithoutReserve || lBlockSize > UINT32_MAX)
560+
if (lAllocSize > PacketBuffer::kMaxSizeWithoutReserve)
535561
{
536-
ChipLogError(chipSystemLayer, "PacketBuffer: allocation too large.");
562+
ChipLogError(chipSystemLayer, "PacketBuffer: allocation exceeding buffer capacity limits.");
537563
return PacketBufferHandle();
538564
}
539565

540566
#if CHIP_SYSTEM_CONFIG_USE_LWIP
541-
567+
// This cast is safe because lAllocSize is no larger than
568+
// kMaxSizeWithoutReserve, which fits in uint16_t.
542569
lPacket = static_cast<PacketBuffer *>(
543570
pbuf_alloc(PBUF_RAW, static_cast<uint16_t>(lAllocSize), CHIP_SYSTEM_PACKETBUFFER_LWIP_PBUF_TYPE));
544571

545572
SYSTEM_STATS_UPDATE_LWIP_PBUF_COUNTS();
546573

547574
#elif CHIP_SYSTEM_PACKETBUFFER_FROM_CHIP_POOL
548575

549-
static_cast<void>(lBlockSize);
550576
#if !CHIP_SYSTEM_CONFIG_NO_LOCKING && CHIP_SYSTEM_CONFIG_FREERTOS_LOCKING
551577
if (!sBufferPoolMutex.isInitialized())
552578
{
@@ -565,8 +591,10 @@ PacketBufferHandle PacketBufferHandle::New(size_t aAvailableSize, uint16_t aRese
565591
UNLOCK_BUF_POOL();
566592

567593
#elif CHIP_SYSTEM_PACKETBUFFER_FROM_CHIP_HEAP
568-
569-
lPacket = reinterpret_cast<PacketBuffer *>(chip::Platform::MemoryAlloc(lBlockSize));
594+
// sumOfSizes is essentially (kStructureSize + lAllocSize) which we already
595+
// checked to fit in a size_t.
596+
const size_t lBlockSize = static_cast<size_t>(sumOfSizes);
597+
lPacket = reinterpret_cast<PacketBuffer *>(chip::Platform::MemoryAlloc(lBlockSize));
570598
SYSTEM_STATS_INCREMENT(chip::System::Stats::kSystemLayer_NumPacketBufs);
571599

572600
#else

0 commit comments

Comments
 (0)