Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent TLVWrite from possibly performing buffer overrun if trying to reserve #30714

Merged
merged 11 commits into from
Dec 14, 2023
10 changes: 10 additions & 0 deletions src/lib/core/TLVBackingStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,16 @@ class DLL_EXPORT TLVBackingStore
*
*/
virtual CHIP_ERROR FinalizeBuffer(TLVWriter & writer, uint8_t * bufStart, uint32_t bufLen) = 0;

/**
* Returns whether call to GetNewBuffer will always fail.
*
* There are some implementations of TLVBackingStore that provide some level of utility, such as access to pool
* of particular kind of buffer and/or reserving space for headers. Some implementation allow the caller to
* specify that they only intend to use a single buffer. It is useful for TLVWriter to know if this is the case.
*
*/
virtual bool GetNewBufferWillAlwaysFail() { return false; }
tehampson marked this conversation as resolved.
Show resolved Hide resolved
};

} // namespace TLV
Expand Down
14 changes: 14 additions & 0 deletions src/lib/core/TLVWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,20 @@ CHIP_ERROR TLVWriter::Finalize()
return err;
}

CHIP_ERROR TLVWriter::ReserveBuffer(uint32_t aBufferSize)
{
VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mRemainingLen >= aBufferSize, CHIP_ERROR_NO_MEMORY);

if (mBackingStore)
{
VerifyOrReturnError(mBackingStore->GetNewBufferWillAlwaysFail(), CHIP_ERROR_INCORRECT_STATE);
}
mReservedSize += aBufferSize;
mRemainingLen -= aBufferSize;
tehampson marked this conversation as resolved.
Show resolved Hide resolved
return CHIP_NO_ERROR;
}

CHIP_ERROR TLVWriter::PutBoolean(Tag tag, bool v)
{
return WriteElementHead((v) ? TLVElementType::BooleanTrue : TLVElementType::BooleanFalse, tag, 0);
Expand Down
13 changes: 5 additions & 8 deletions src/lib/core/TLVWriter.h
Original file line number Diff line number Diff line change
Expand Up @@ -144,15 +144,12 @@ class DLL_EXPORT TLVWriter
* @retval #CHIP_NO_ERROR Successfully reserved required buffer size.
* @retval #CHIP_ERROR_INCORRECT_STATE If the TLVWriter was not initialized.
* @retval #CHIP_ERROR_NO_MEMORY The reserved buffer size cannot fits into the remaining buffer size.
* @retval #CHIP_ERROR_INCORRECT_STATE
* Uses TLVBackingStore and is in a state where it might allocate
* additional non-contigious memory, thus making it difficult/impossible
* to properly reserve space.
*/
CHIP_ERROR ReserveBuffer(uint32_t aBufferSize)
{
VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mRemainingLen >= aBufferSize, CHIP_ERROR_NO_MEMORY);
mReservedSize += aBufferSize;
mRemainingLen -= aBufferSize;
return CHIP_NO_ERROR;
}
CHIP_ERROR ReserveBuffer(uint32_t aBufferSize);

/**
* Release previously reserved buffer.
Expand Down
6 changes: 6 additions & 0 deletions src/system/TLVPacketBufferBackingStore.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ class TLVPacketBufferBackingStore : public chip::TLV::TLVBackingStore
CHIP_ERROR OnInit(chip::TLV::TLVWriter & writer, uint8_t *& bufStart, uint32_t & bufLen) override;
CHIP_ERROR GetNewBuffer(chip::TLV::TLVWriter & writer, uint8_t *& bufStart, uint32_t & bufLen) override;
CHIP_ERROR FinalizeBuffer(chip::TLV::TLVWriter & writer, uint8_t * bufStart, uint32_t bufLen) override;
virtual bool GetNewBufferWillAlwaysFail() override
{
// For non-chained buffers, caller is given one chunk of contiguous memory. All calls to
// GetNewBuffer will fail with CHIP_ERROR_NO_MEMORY.
return !mUseChainedBuffers;
}

protected:
chip::System::PacketBufferHandle mHeadBuffer;
Expand Down
114 changes: 112 additions & 2 deletions src/system/tests/TestTLVPacketBufferBackingStore.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ using namespace ::chip;

namespace {

void WriteUntilRemainingLessThan(nlTestSuite * inSuite, PacketBufferTLVWriter & writer, const uint32_t remainingSize)
{
uint32_t lengthRemaining = writer.GetRemainingFreeLength();
while (lengthRemaining >= remainingSize)
{
NL_TEST_ASSERT(inSuite, writer.Put(TLV::AnonymousTag(), static_cast<uint8_t>(7)) == CHIP_NO_ERROR);
lengthRemaining = writer.GetRemainingFreeLength();
}
}

class TLVPacketBufferBackingStoreTest
{
public:
Expand All @@ -43,6 +53,9 @@ class TLVPacketBufferBackingStoreTest

static void BasicEncodeDecode(nlTestSuite * inSuite, void * inContext);
static void MultiBufferEncode(nlTestSuite * inSuite, void * inContext);
static void NonChainedBufferCanReserve(nlTestSuite * inSuite, void * inContext);
static void TestWriterReserveUnreserveDoesNotOverflow(nlTestSuite * inSuite, void * inContext);
static void TestWriterReserve(nlTestSuite * inSuite, void * inContext);
};

int TLVPacketBufferBackingStoreTest::TestSetup(void * inContext)
Expand Down Expand Up @@ -238,14 +251,111 @@ void TLVPacketBufferBackingStoreTest::MultiBufferEncode(nlTestSuite * inSuite, v
NL_TEST_ASSERT(inSuite, error == CHIP_END_OF_TLV);
}

void TLVPacketBufferBackingStoreTest::NonChainedBufferCanReserve(nlTestSuite * inSuite, void * inContext)
{
// Start with a too-small buffer.
uint32_t smallSize = 5;
uint32_t smallerSizeToReserver = smallSize - 1;

auto buffer = PacketBufferHandle::New(smallSize, /* aReservedSize = */ 0);

PacketBufferTLVWriter writer;
writer.Init(std::move(buffer), /* useChainedBuffers = */ false);

CHIP_ERROR error = writer.ReserveBuffer(smallerSizeToReserver);
NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR);
}

// This test previously was created to show that there was an overflow bug, now this test mainly
// just checks that you cannot reserve this type of TLVBackingStorage buffer.
void TLVPacketBufferBackingStoreTest::TestWriterReserveUnreserveDoesNotOverflow(nlTestSuite * inSuite, void * inContext)
{
// Start with a too-small buffer.
uint32_t smallSize = 100;
uint32_t smallerSizeToReserver = smallSize - 1;

auto buffer = PacketBufferHandle::New(smallSize, 0);

PacketBufferTLVWriter writer;
writer.Init(std::move(buffer), /* useChainedBuffers = */ true);

CHIP_ERROR error = writer.ReserveBuffer(smallerSizeToReserver);
if (error == CHIP_NO_ERROR)
{
uint32_t lengthRemaining = writer.GetRemainingFreeLength();
NL_TEST_ASSERT(inSuite, lengthRemaining == 1);
// Lets try to overflow by getting next buffer in the chain,
// unreserving then writing until the end of the current buffer.
error = writer.Put(TLV::AnonymousTag(), static_cast<uint8_t>(7));
NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR);

lengthRemaining = writer.GetRemainingFreeLength();
NL_TEST_ASSERT(inSuite, lengthRemaining > smallerSizeToReserver);

WriteUntilRemainingLessThan(inSuite, writer, 2);

lengthRemaining = writer.GetRemainingFreeLength();
NL_TEST_ASSERT(inSuite, lengthRemaining != 0);
NL_TEST_ASSERT(inSuite, lengthRemaining < smallerSizeToReserver);

error = writer.UnreserveBuffer(smallerSizeToReserver);
NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR);

lengthRemaining = writer.GetRemainingFreeLength();
NL_TEST_ASSERT(inSuite, lengthRemaining > smallerSizeToReserver);

// This is where we get overflow.
WriteUntilRemainingLessThan(inSuite, writer, 2);

// If we get here then the overflow condition we were expecting did not happen. If that is the case,
// either we have fixed reservation for chained buffers, or expected failure didn't hit on this
// platform.
//
// If there is a fix please add reservation for chained buffers, please make sure you account for
// what happens if TLVWriter::WriteData fails to get a new buffer but we are not at max size, do
// you actually have space for what was supposed to be reserved.
NL_TEST_ASSERT(inSuite, false);
}

// We no longer allow non-contigous buffers to be reserved.
NL_TEST_ASSERT(inSuite, error == CHIP_ERROR_INCORRECT_STATE);
}

void TLVPacketBufferBackingStoreTest::TestWriterReserve(nlTestSuite * inSuite, void * inContext)
{
// Start with a too-small buffer.
uint32_t smallSize = 5;
uint32_t smallerSizeToReserver = smallSize - 1;

auto buffer = PacketBufferHandle::New(smallSize, 0);

PacketBufferTLVWriter writer;
writer.Init(std::move(buffer), /* useChainedBuffers = */ false);

CHIP_ERROR error = writer.ReserveBuffer(smallerSizeToReserver);
NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR);

error = writer.Put(TLV::AnonymousTag(), static_cast<uint8_t>(7));
NL_TEST_ASSERT(inSuite, error == CHIP_ERROR_NO_MEMORY);

error = writer.UnreserveBuffer(smallerSizeToReserver);
NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR);

error = writer.Put(TLV::AnonymousTag(), static_cast<uint8_t>(7));
NL_TEST_ASSERT(inSuite, error == CHIP_NO_ERROR);
}

/**
* Test Suite. It lists all the test functions.
*/
// clang-format off
const nlTest sTests[] =
{
NL_TEST_DEF("BasicEncodeDecode", TLVPacketBufferBackingStoreTest::BasicEncodeDecode),
NL_TEST_DEF("MultiBufferEncode", TLVPacketBufferBackingStoreTest::MultiBufferEncode),
NL_TEST_DEF("BasicEncodeDecode", TLVPacketBufferBackingStoreTest::BasicEncodeDecode),
NL_TEST_DEF("MultiBufferEncode", TLVPacketBufferBackingStoreTest::MultiBufferEncode),
NL_TEST_DEF("NonChainedBufferCanReserve", TLVPacketBufferBackingStoreTest::NonChainedBufferCanReserve),
NL_TEST_DEF("TestWriterReserveUnreserveDoesNotOverflow", TLVPacketBufferBackingStoreTest::TestWriterReserveUnreserveDoesNotOverflow),
NL_TEST_DEF("TestWriterReserve", TLVPacketBufferBackingStoreTest::TestWriterReserve),

NL_TEST_SENTINEL()
};
Expand Down
Loading