RDKEMW-28168: Bound-check client-controlled sizes in shm metadata parsing

media/server/main/include/DataReaderFactory.h

@@ -31,8 +31,8 @@ class DataReaderFactory : public IDataReaderFactory
     DataReaderFactory() = default;
     ~DataReaderFactory() override = default;
     std::shared_ptr<IDataReader> createDataReader(const MediaSourceType &mediaSourceType, std::uint8_t *buffer,
-                                                  std::uint32_t dataOffset, std::uint32_t numFrames,
-                                                  bool isBufferFull) const override;
+                                                  std::uint32_t dataOffset, std::uint32_t regionLen,
+                                                  std::uint32_t numFrames, bool isBufferFull) const override;
 };
 } // namespace firebolt::rialto::server
 

media/server/main/include/DataReaderV1.h

@@ -56,7 +56,7 @@ class DataReaderV1 : public IDataReader
 
 public:
     DataReaderV1(const MediaSourceType &mediaSourceType, std::uint8_t *buffer, std::uint32_t metadataOffset,
-                 std::uint32_t numFrames, bool isBufferFull);
+                 std::uint32_t numFrames, bool isBufferFull, std::uint32_t regionStart, std::uint64_t regionEnd);
     ~DataReaderV1() override = default;
 
     IMediaPipeline::MediaSegmentVector readData() const override;
@@ -71,15 +71,26 @@ class DataReaderV1 : public IDataReader
     std::uint32_t m_metadataOffset;
     std::uint32_t m_numFrames;
     bool m_isBufferFull;
+    std::uint32_t m_regionStart;
+    std::uint64_t m_regionEnd;
 
     template <typename SegmentType> std::unique_ptr<SegmentType> createSegment(const MetadataV1 &metadata) const
     {
+        // All sizes and offsets below are read from shared memory written by the
+        // client - bound-check them against the region before use.
+        if (metadata.offset < m_regionStart || metadata.offset > m_regionEnd ||
+            metadata.length > m_regionEnd - metadata.offset)
+        {
+            return nullptr;
+        }
         std::unique_ptr<SegmentType> mediaSegment{std::make_unique<SegmentType>(0, metadata.timePosition,
                                                                                 metadata.sampleDuration,
                                                                                 metadata.extra1, metadata.extra2)};
         mediaSegment->setData(metadata.length, m_buffer + metadata.offset);
+        const std::uint32_t kExtraDataSize{std::min<std::uint32_t>(metadata.extraDataSize,
+                                                                 sizeof(metadata.extraData))};
         std::vector<std::uint8_t> extraDataVec;
-        std::copy(&metadata.extraData[0], &metadata.extraData[metadata.extraDataSize], std::back_inserter(extraDataVec));
+        std::copy(&metadata.extraData[0], &metadata.extraData[kExtraDataSize], std::back_inserter(extraDataVec));
         mediaSegment->setExtraData(extraDataVec);
         mediaSegment->setEncrypted(false); // temporary, only clear content playback should be supported now
         return mediaSegment;

media/server/main/include/DataReaderV2.h

@@ -30,7 +30,7 @@ class DataReaderV2 : public IDataReader
 {
 public:
     DataReaderV2(const MediaSourceType &mediaSourceType, std::uint8_t *buffer, std::uint32_t dataOffset,
-                 std::uint32_t numFrames, bool isBufferFull);
+                 std::uint32_t numFrames, bool isBufferFull, std::uint64_t regionEnd);
     ~DataReaderV2() override = default;
 
     IMediaPipeline::MediaSegmentVector readData() const override;
@@ -42,6 +42,7 @@ class DataReaderV2 : public IDataReader
     std::uint32_t m_dataOffset;
     std::uint32_t m_numFrames;
     bool m_isBufferFull;
+    std::uint64_t m_regionEnd;
 };
 } // namespace firebolt::rialto::server
 

media/server/main/include/IDataReaderFactory.h

@@ -38,8 +38,8 @@ class IDataReaderFactory
     virtual ~IDataReaderFactory() = default;
 
     virtual std::shared_ptr<IDataReader> createDataReader(const MediaSourceType &mediaSourceType, std::uint8_t *data,
-                                                          std::uint32_t dataOffset, std::uint32_t numFrames,
-                                                          bool isBufferFull) const = 0;
+                                                          std::uint32_t dataOffset, std::uint32_t regionLen,
+                                                          std::uint32_t numFrames, bool isBufferFull) const = 0;
 };
 } // namespace firebolt::rialto::server
 

media/server/main/source/DataReaderFactory.cpp

@@ -36,21 +36,30 @@ namespace firebolt::rialto::server
 {
 std::shared_ptr<IDataReader> DataReaderFactory::createDataReader(const MediaSourceType &mediaSourceType,
                                                                  std::uint8_t *buffer, std::uint32_t dataOffset,
-                                                                 std::uint32_t numFrames, bool isBufferFull) const
+                                                                 std::uint32_t regionLen, std::uint32_t numFrames,
+                                                                 bool isBufferFull) const
 {
+    // The region end bounds every client-controlled size read from shared memory.
+    const std::uint64_t kRegionEnd{static_cast<std::uint64_t>(dataOffset) + regionLen};
+    if (regionLen < common::VERSION_SIZE_BYTES)
+    {
+        return nullptr;
+    }
+
     // Version is always first 4 bytes of data
     std::uint8_t *metadata = buffer + dataOffset;
     std::uint32_t version = readLEUint32(metadata);
     if (1 == version)
     {
         std::uint32_t metadataOffsetWithoutVersion = dataOffset + common::VERSION_SIZE_BYTES;
         return std::make_shared<DataReaderV1>(mediaSourceType, buffer, metadataOffsetWithoutVersion, numFrames,
-                                              isBufferFull);
+                                              isBufferFull, dataOffset, kRegionEnd);
     }
     if (2 == version)
     {
         std::uint32_t v2DataOffset = dataOffset + getMaxMetadataBytes();
-        return std::make_shared<DataReaderV2>(mediaSourceType, buffer, v2DataOffset, numFrames, isBufferFull);
+        return std::make_shared<DataReaderV2>(mediaSourceType, buffer, v2DataOffset, numFrames, isBufferFull,
+                                              kRegionEnd);
     }
     return nullptr;
 }

media/server/main/source/DataReaderV1.cpp

@@ -21,16 +21,29 @@
 #include "RialtoServerLogging.h"
 #include "ShmCommon.h"
 #include "TypeConverters.h"
+#include <cinttypes>
+#include <cstring>
 
 namespace firebolt::rialto::server
 {
 DataReaderV1::DataReaderV1(const MediaSourceType &mediaSourceType, std::uint8_t *buffer, std::uint32_t metadataOffset,
-                           std::uint32_t numFrames, bool isBufferFull)
+                           std::uint32_t numFrames, bool isBufferFull, std::uint32_t regionStart,
+                           std::uint64_t regionEnd)
     : m_mediaSourceType{mediaSourceType}, m_buffer{buffer}, m_metadataOffset{metadataOffset}, m_numFrames{numFrames},
-      m_isBufferFull{isBufferFull}
+      m_isBufferFull{isBufferFull}, m_regionStart{regionStart}, m_regionEnd{regionEnd}
 {
     RIALTO_SERVER_LOG_DEBUG("Detected Metadata in Version 1. Media source type: %s",
                             common::convertMediaSourceType(m_mediaSourceType));
+
+    // numFrames comes from the client's IPC call - clamp it to the number of
+    // metadata records that fit in the region before it can drive an OOB walk.
+    const std::uint64_t kAvail{m_regionEnd > m_metadataOffset ? m_regionEnd - m_metadataOffset : 0};
+    const std::uint64_t kMaxFrames{kAvail / common::METADATA_V1_SIZE_PER_FRAME_BYTES};
+    if (m_numFrames > kMaxFrames)
+    {
+        RIALTO_SERVER_LOG_ERROR("numFrames %u exceeds region capacity %" PRIu64 " - clamping", m_numFrames, kMaxFrames);
+        m_numFrames = static_cast<std::uint32_t>(kMaxFrames);
+    }
     static_assert(sizeof(DataReaderV1::MetadataV1) == common::METADATA_V1_SIZE_PER_FRAME_BYTES,
                   "Size of firebolt::rialto::server::DataReaderV1::MetadataV1 is not equal to "
                   "firebolt::rialto::common::METADATA_V1_SIZE_PER_FRAME_BYTES");
@@ -44,11 +57,17 @@ IMediaPipeline::MediaSegmentVector DataReaderV1::readData() const
     {
         if (m_mediaSourceType == MediaSourceType::AUDIO)
         {
-            mediaSegments.emplace_back(createSegment<IMediaPipeline::MediaSegmentAudio>(metadata));
+            if (auto segment = createSegment<IMediaPipeline::MediaSegmentAudio>(metadata))
+                mediaSegments.emplace_back(std::move(segment));
+            else
+                RIALTO_SERVER_LOG_ERROR("Skipping audio segment with out-of-range data offset/length");
         }
         else if (m_mediaSourceType == MediaSourceType::VIDEO)
         {
-            mediaSegments.emplace_back(createSegment<IMediaPipeline::MediaSegmentVideo>(metadata));
+            if (auto segment = createSegment<IMediaPipeline::MediaSegmentVideo>(metadata))
+                mediaSegments.emplace_back(std::move(segment));
+            else
+                RIALTO_SERVER_LOG_ERROR("Skipping video segment with out-of-range data offset/length");
         }
     }
     return mediaSegments;
@@ -66,8 +85,10 @@ std::vector<DataReaderV1::MetadataV1> DataReaderV1::readMetadata() const
     for (std::uint32_t frame = 0; frame < m_numFrames; ++frame)
     {
         std::uint8_t *frameMetadataOffset = regionMetadataOffset + (frame * common::METADATA_V1_SIZE_PER_FRAME_BYTES);
-        DataReaderV1::MetadataV1 *metadataPtr = reinterpret_cast<DataReaderV1::MetadataV1 *>(frameMetadataOffset);
-        result.emplace_back(*metadataPtr);
+        // memcpy rather than a reinterpret_cast: the metadata region is not guaranteed aligned.
+        DataReaderV1::MetadataV1 metadata;
+        std::memcpy(&metadata, frameMetadataOffset, sizeof(metadata));
+        result.emplace_back(metadata);
     }
     return result;
 }

media/server/main/source/DataReaderV2.cpp

@@ -22,6 +22,7 @@
 #include "ShmCommon.h"
 #include "TypeConverters.h"
 #include "metadata.pb.h"
+#include <cstring>
 
 namespace
 {
@@ -203,9 +204,9 @@ createSegment(const firebolt::rialto::MediaSegmentMetadata &metadata, const fire
 namespace firebolt::rialto::server
 {
 DataReaderV2::DataReaderV2(const MediaSourceType &mediaSourceType, std::uint8_t *buffer, std::uint32_t dataOffset,
-                           std::uint32_t numFrames, bool isBufferFull)
+                           std::uint32_t numFrames, bool isBufferFull, std::uint64_t regionEnd)
     : m_mediaSourceType{mediaSourceType}, m_buffer{buffer}, m_dataOffset{dataOffset}, m_numFrames{numFrames},
-      m_isBufferFull{isBufferFull}
+      m_isBufferFull{isBufferFull}, m_regionEnd{regionEnd}
 {
     RIALTO_SERVER_LOG_DEBUG("Detected Metadata in Version 2. Media source type: %s",
                             common::convertMediaSourceType(m_mediaSourceType));
@@ -214,13 +215,28 @@ DataReaderV2::DataReaderV2(const MediaSourceType &mediaSourceType, std::uint8_t
 IMediaPipeline::MediaSegmentVector DataReaderV2::readData() const
 {
     IMediaPipeline::MediaSegmentVector mediaSegments;
+    uint8_t *const kDataEnd{m_buffer + m_regionEnd};
     uint8_t *currentReadPosition{m_buffer + m_dataOffset};
     for (auto i = 0U; i < m_numFrames; ++i)
     {
-        std::uint32_t *metadataSize{reinterpret_cast<uint32_t *>(currentReadPosition)};
-        currentReadPosition += sizeof(uint32_t);
+        // All sizes below are written by the client into shared memory - bound
+        // every advance against the region end before use.
+        if (kDataEnd - currentReadPosition < static_cast<ptrdiff_t>(sizeof(std::uint32_t)))
+        {
+            RIALTO_SERVER_LOG_ERROR("Truncated frame %u metadata size field", i);
+            return IMediaPipeline::MediaSegmentVector{};
+        }
+        // memcpy rather than a reinterpret_cast: the data is not guaranteed aligned.
+        std::uint32_t metadataSize;
+        std::memcpy(&metadataSize, currentReadPosition, sizeof(metadataSize));
+        currentReadPosition += sizeof(metadataSize);
+        if (kDataEnd - currentReadPosition < static_cast<ptrdiff_t>(metadataSize))
+        {
+            RIALTO_SERVER_LOG_ERROR("Truncated frame %u metadata", i);
+            return IMediaPipeline::MediaSegmentVector{};
+        }
         MediaSegmentMetadata metadata;
-        if (!metadata.ParseFromArray(currentReadPosition, *metadataSize))
+        if (!metadata.ParseFromArray(currentReadPosition, metadataSize))
         {
             RIALTO_SERVER_LOG_ERROR("Metadata parsing failed!");
             return IMediaPipeline::MediaSegmentVector{};
@@ -231,7 +247,12 @@ IMediaPipeline::MediaSegmentVector DataReaderV2::readData() const
             RIALTO_SERVER_LOG_ERROR("Segment parsing failed!");
             return IMediaPipeline::MediaSegmentVector{};
         }
-        currentReadPosition += *metadataSize;
+        currentReadPosition += metadataSize;
+        if (kDataEnd - currentReadPosition < static_cast<ptrdiff_t>(metadata.length()))
+        {
+            RIALTO_SERVER_LOG_ERROR("Truncated frame %u media data", i);
+            return IMediaPipeline::MediaSegmentVector{};
+        }
         newSegment->setData(metadata.length(), currentReadPosition);
         currentReadPosition += metadata.length();
         mediaSegments.emplace_back(std::move(newSegment));

media/server/main/source/MediaPipelineServerInternal.cpp

@@ -818,10 +818,13 @@ bool MediaPipelineServerInternal::haveDataInternal(MediaSourceStatus status, uin
     }
 
     std::uint32_t regionOffset = 0;
+    std::uint32_t regionLen = 0;
     try
     {
         regionOffset =
             m_shmBuffer->getDataOffset(ISharedMemoryBuffer::MediaPlaybackType::GENERIC, m_sessionId, mediaSourceType);
+        regionLen =
+            m_shmBuffer->getMaxDataLen(ISharedMemoryBuffer::MediaPlaybackType::GENERIC, m_sessionId, mediaSourceType);
     }
     catch (const std::runtime_error &e)
     {
@@ -830,12 +833,21 @@ bool MediaPipelineServerInternal::haveDataInternal(MediaSourceStatus status, uin
         return false;
     }
 
+    // numFrames is a client-supplied IPC field - clamp it to the frame count the
+    // request actually provisioned, or it can drive an OOB walk in the reader.
+    if (numFrames > kMaxNumFrames)
+    {
+        RIALTO_SERVER_LOG_ERROR("haveData numFrames %u exceeds requested max %u - clamping", numFrames, kMaxNumFrames);
+        numFrames = kMaxNumFrames;
+    }
+
     if (0 != numFrames)
     {
         const bool kIsBufferFull = kMaxNumFrames == numFrames || status == MediaSourceStatus::NO_SPACE_FOR_SAMPLES ||
                                    status == MediaSourceStatus::EOS;
         std::shared_ptr<IDataReader> dataReader =
-            m_dataReaderFactory->createDataReader(mediaSourceType, buffer, regionOffset, numFrames, kIsBufferFull);
+            m_dataReaderFactory->createDataReader(mediaSourceType, buffer, regionOffset, regionLen, numFrames,
+                                                  kIsBufferFull);
         if (!dataReader)
         {
             RIALTO_SERVER_LOG_ERROR("Metadata version not supported for %s request id: %u",

tests/unittests/media/server/main/dataReader/DataReaderV1Tests.cpp

@@ -99,7 +99,7 @@ class DataReaderV1Tests : public testing::Test
     {
         ASSERT_TRUE(m_shm);
         std::uint8_t *buffer = m_shm->getBuffer();
-        m_sut = std::make_unique<DataReaderV1>(kVideoMediaSourceType, buffer, 4, kNumFrames, kIsBufferFull);
+        m_sut = std::make_unique<DataReaderV1>(kVideoMediaSourceType, buffer, 4, kNumFrames, kIsBufferFull, 0, UINT64_MAX);
         auto result = m_sut->readData();
         EXPECT_EQ(kIsBufferFull, m_sut->isBufferFull());
         ASSERT_EQ(1, result.size());
@@ -143,7 +143,7 @@ class DataReaderV1Tests : public testing::Test
         std::uint32_t regionOffset =
             m_shm->getDataOffset(ISharedMemoryBuffer::MediaPlaybackType::GENERIC, kSessionId, kAudioMediaSourceType);
         m_sut =
-            std::make_unique<DataReaderV1>(kAudioMediaSourceType, buffer, regionOffset + 4, kNumFrames, kIsBufferFull);
+            std::make_unique<DataReaderV1>(kAudioMediaSourceType, buffer, regionOffset + 4, kNumFrames, kIsBufferFull, 0, UINT64_MAX);
         auto result = m_sut->readData();
         ASSERT_EQ(1, result.size());
         EXPECT_EQ(kIsBufferFull, m_sut->isBufferFull());

tests/unittests/media/server/main/dataReader/DataReaderV2Tests.cpp

@@ -297,7 +297,7 @@ class DataReaderV2Tests : public testing::Test
 
     std::unique_ptr<IMediaPipeline::MediaSegment> readData(const firebolt::rialto::MediaSourceType &sourceType)
     {
-        m_sut = std::make_unique<DataReaderV2>(sourceType, m_shm, kMetaDataSize, kNumFrames, kIsBufferFull);
+        m_sut = std::make_unique<DataReaderV2>(sourceType, m_shm, kMetaDataSize, kNumFrames, kIsBufferFull, UINT64_MAX);
         EXPECT_EQ(m_sut->isBufferFull(), kIsBufferFull);
         auto result = m_sut->readData();
         if (result.size() != 1)