Skip to content

Commit

Permalink
Fix packetbuffer handling in UDPEndPointImplOpenThread. (#18851)
Browse files Browse the repository at this point in the history
What the code was doing was:

1. Allocate a buffer of size msgLen.
2. Copy sizeof(IPPacketInfo) + msgLen bytes into it.  This is a buffer overrun
   if the packetbuffer was not over-allocated.
3. memmove the contents of the buffer over by sizeof(IPPacketInfo), point
   Start() at the location the data was moved to.  This would fail if the buffer
   was not even more over-allocated.
4. Read the memory from ((Start() - sizeof(IPPacketInfo) & ~3) (which might
   _under-run_ the buffer depending on how its allocation is aligned in memory),
   and which anyway conceptually represents garbage data that the packetbuffer
   code is well withiin its rights to have overwritten and treat that memory as
   an IPPacketInfo.
5. Advance Start() by sizeof(IPPacketInfo) to point to the actual data.

What the new code does is:

1. Allocate a buffer with msgLen data space and enough reserved space that we
   can place an aligned IPPacketInfo in the reserved space.
2. Copy the incoming data into the data space.
3. Fill in an IPPacketInfo in the reserved space.
4. Later read the IPPacketInfo from the reserved space.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Feb 5, 2024
1 parent 6e29aa6 commit 2313288
Showing 1 changed file with 32 additions and 18 deletions.
50 changes: 32 additions & 18 deletions src/inet/UDPEndPointImplOpenThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,21 @@ namespace Inet {

otInstance * globalOtInstance;

namespace {
// We want to reserve space for an IPPacketInfo in our buffer, but it needs to
// be 4-byte aligned. We ensure the alignment by masking off the low bits of
// the pointer that we get by doing `Start() - sizeof(IPPacketInfo)`. That
// might move it backward by up to kPacketInfoAlignmentBytes, so we need to make
// sure we allocate enough reserved space that this will still be within our
// buffer.
constexpr uint16_t kPacketInfoAlignmentBytes = sizeof(uint32_t) - 1;
constexpr uint16_t kPacketInfoReservedSize = sizeof(IPPacketInfo) + kPacketInfoAlignmentBytes;
} // namespace

void UDPEndPointImplOT::handleUdpReceive(void * aContext, otMessage * aMessage, const otMessageInfo * aMessageInfo)
{
UDPEndPointImplOT * ep = static_cast<UDPEndPointImplOT *>(aContext);
IPPacketInfo pktInfo;
uint16_t msgLen = otMessageGetLength(aMessage);
uint16_t msgLen = otMessageGetLength(aMessage);
System::PacketBufferHandle payload;
#if CHIP_DETAIL_LOGGING
static uint16_t msgReceivedCount = 0;
Expand All @@ -50,38 +60,43 @@ void UDPEndPointImplOT::handleUdpReceive(void * aContext, otMessage * aMessage,
return;
}

pktInfo.SrcAddress = IPAddress::FromOtAddr(aMessageInfo->mPeerAddr);
pktInfo.DestAddress = IPAddress::FromOtAddr(aMessageInfo->mSockAddr);
pktInfo.SrcPort = aMessageInfo->mPeerPort;
pktInfo.DestPort = aMessageInfo->mSockPort;

payload = System::PacketBufferHandle::New(msgLen, 0);
payload = System::PacketBufferHandle::New(msgLen, kPacketInfoReservedSize);

if (payload.IsNull())
{
ChipLogError(Inet, "Failed to allocate a System buffer of size %d for UDP Message reception.", msgLen);
return;
}

IPPacketInfo * pktInfo = GetPacketInfo(payload);
if (pktInfo == nullptr)
{
ChipLogError(Inet, "Failed to pre-allocate reserved space for an IPPacketInfo for UDP Message reception.");
return;
}

pktInfo->SrcAddress = IPAddress::FromOtAddr(aMessageInfo->mPeerAddr);
pktInfo->DestAddress = IPAddress::FromOtAddr(aMessageInfo->mSockAddr);
pktInfo->SrcPort = aMessageInfo->mPeerPort;
pktInfo->DestPort = aMessageInfo->mSockPort;

#if CHIP_DETAIL_LOGGING
pktInfo.SrcAddress.ToString(sourceStr, Inet::IPAddress::kMaxStringLength);
pktInfo.DestAddress.ToString(destStr, Inet::IPAddress::kMaxStringLength);
pktInfo->SrcAddress.ToString(sourceStr, Inet::IPAddress::kMaxStringLength);
pktInfo->DestAddress.ToString(destStr, Inet::IPAddress::kMaxStringLength);

ChipLogDetail(Inet,
"UDP Message Received packet nb : %d SrcAddr : %s[%d] DestAddr "
": %s[%d] Payload Length %d",
++msgReceivedCount, sourceStr, pktInfo.SrcPort, destStr, pktInfo.DestPort, msgLen);
++msgReceivedCount, sourceStr, pktInfo->SrcPort, destStr, pktInfo->DestPort, msgLen);

#endif

memcpy(payload->Start(), &pktInfo, sizeof(IPPacketInfo));

if (otMessageRead(aMessage, 0, payload->Start() + sizeof(IPPacketInfo), msgLen) != msgLen)
if (otMessageRead(aMessage, 0, payload->Start(), msgLen) != msgLen)
{
ChipLogError(Inet, "Failed to copy OpenThread buffer into System Packet buffer");
return;
}
payload->SetDataLength(static_cast<uint16_t>(msgLen + sizeof(IPPacketInfo)));
payload->SetDataLength(static_cast<uint16_t>(msgLen));

ep->Retain();
auto * buf = std::move(payload).UnsafeRelease();
Expand Down Expand Up @@ -160,7 +175,6 @@ void UDPEndPointImplOT::HandleDataReceived(System::PacketBufferHandle && msg)
const IPPacketInfo pktInfoCopy = *pktInfo; // copy the address info so that the app can free the
// PacketBuffer without affecting access to address info.

msg->ConsumeHead(sizeof(IPPacketInfo));
OnMessageReceived(this, std::move(msg), &pktInfoCopy);
}
else
Expand Down Expand Up @@ -279,7 +293,7 @@ CHIP_ERROR UDPEndPointImplOT::IPv6JoinLeaveMulticastGroupImpl(InterfaceId aInter

IPPacketInfo * UDPEndPointImplOT::GetPacketInfo(const System::PacketBufferHandle & aBuffer)
{
if (!aBuffer->EnsureReservedSize(sizeof(IPPacketInfo)))
if (!aBuffer->EnsureReservedSize(kPacketInfoReservedSize))
{
return nullptr;
}
Expand All @@ -288,7 +302,7 @@ IPPacketInfo * UDPEndPointImplOT::GetPacketInfo(const System::PacketBufferHandle
uintptr_t lPacketInfoStart = lStart - sizeof(IPPacketInfo);

// Align to a 4-byte boundary
return reinterpret_cast<IPPacketInfo *>(lPacketInfoStart & ~(sizeof(uint32_t) - 1));
return reinterpret_cast<IPPacketInfo *>(lPacketInfoStart & ~kPacketInfoAlignmentBytes);
}

} // namespace Inet
Expand Down

0 comments on commit 2313288

Please sign in to comment.