[2/2] libcamera: Harden control serializer size and input validation
diff mbox series

Message ID 20260723174644.6580-3-magdum.foss@gmail.com
State Superseded
Headers show
Series
  • libcamera: Control name serialization v3 and input validation hardening
Related show

Commit Message

Magdum July 23, 2026, 5:46 p.m. UTC 
Add overflow-safe size computations before writing 32-bit wire fields,
centralize control-name size accounting, and validate deserialized
local control direction values.

Strengthen tests with alignment-safe packet mutation, deterministic
bad-terminator corruption, and max-length control-name boundary
coverage.

Document binarySize() overflow sentinel semantics in the internal
serializer header.

Signed-off-by: Magdum <magdum.foss@gmail.com>
---
 src/libcamera/control_serializer.cpp         | 222 ++++++++++++++++---
 test/serialization/control_serialization.cpp |  77 ++++++-
 2 files changed, 268 insertions(+), 31 deletions(-)

Comments

Barnabás Pőcze July 24, 2026, 1:36 p.m. UTC | #1 
Hi

2026. 07. 23. 19:46 keltezéssel, Magdum írta:
> Add overflow-safe size computations before writing 32-bit wire fields,
> centralize control-name size accounting, and validate deserialized
> local control direction values.
> 
> Strengthen tests with alignment-safe packet mutation, deterministic
> bad-terminator corruption, and max-length control-name boundary
> coverage.
> 
> Document binarySize() overflow sentinel semantics in the internal
> serializer header.
> 
> Signed-off-by: Magdum <magdum.foss@gmail.com>
> ---
>   src/libcamera/control_serializer.cpp         | 222 ++++++++++++++++---
>   test/serialization/control_serialization.cpp |  77 ++++++-
>   2 files changed, 268 insertions(+), 31 deletions(-)
> 
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index b3c44ed0..076e78c5 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -8,6 +8,7 @@
>   #include "libcamera/internal/control_serializer.h"
>   
>   #include <algorithm>
> +#include <limits>
>   #include <memory>
>   #include <vector>
>   
> @@ -50,6 +51,58 @@ bool idMapRequiresLocalIds(enum ipa_controls_id_map_type idMapType)
>   	return idMapType == IPA_CONTROL_ID_MAP_V4L2;
>   }
>   
> +size_t serializedControlNameSize(const ControlId *id,
> +				 enum ipa_controls_id_map_type idMapType)
> +{
> +	return idMapRequiresLocalIds(idMapType) ? id->name().size() + 1 : 1;
> +}

I feel like this function could go into the previous patch.


> +
> +bool fitsU32(size_t value)
> +{
> +	return value <= std::numeric_limits<uint32_t>::max();
> +}
> +
> +bool safeMulSizeT(size_t lhs, size_t rhs, size_t *out)
> +{
> +	if (!lhs || !rhs) {
> +		*out = 0;
> +		return true;
> +	}
> +
> +	if (lhs > std::numeric_limits<size_t>::max() / rhs)
> +		return false;

You can adjust this:

   if (rhs && lhs > std::numeric_limits<size_t>::max() / rhs)
     return false;

and drop the previous `if`.


> +
> +	*out = lhs * rhs;
> +	return true;
> +}
> +
> +bool safeAddSizeT(size_t lhs, size_t rhs, size_t *out)
> +{
> +	if (lhs > std::numeric_limits<size_t>::max() - rhs)
> +		return false;
> +
> +	*out = lhs + rhs;
> +	return true;

You can just say `return *out >= lhs;` and drop the `if`.


> +}

But in any case I am a bit wary of these ad-hoc implementations here. Given that
only clang and gcc are supported as compilers, I think it would be preferable to
use the overflow checking builtins: https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
Specifically `__builtin_{add,mul}_overflow`, possibly hidden inside some trivial
wrappers in `utils.h`.


> +
> +bool isValidDirection(uint8_t direction)
> +{
> +	constexpr uint8_t kDirectionIn =
> +		static_cast<uint8_t>(ControlId::Direction::In);
> +	constexpr uint8_t kDirectionOut =
> +		static_cast<uint8_t>(ControlId::Direction::Out);

You can use `utils::to_underlying()`.


> +	constexpr uint8_t kDirectionInOut = kDirectionIn | kDirectionOut;
> +
> +	switch (direction) {
> +	case kDirectionIn:
> +	case kDirectionOut:
> +	case kDirectionInOut:

You can just say `case (kDirectionIn | kDirectionOut):` here


> +		return true;
> +	default:
> +		return false;
> +	}

But you can also simplify the function greatly, e.g.:

   constexpr unsigned int valid = utils::to_underlying(ControlId::Direction::In) | utils::to_underlying(ControlId::Direction::Out);

   return (direction & valid) && !(direction & ~valid);


> +}
> +
>   } /* namespace */
>   
> [...]
Paul Elder July 27, 2026, 9:18 a.m. UTC | #2 
Hi Magdum,

Quoting Magdum (2026-07-24 02:46:44)
> Add overflow-safe size computations before writing 32-bit wire fields,
> centralize control-name size accounting, and validate deserialized
> local control direction values.
> 
> Strengthen tests with alignment-safe packet mutation, deterministic
> bad-terminator corruption, and max-length control-name boundary
> coverage.
> 
> Document binarySize() overflow sentinel semantics in the internal
> serializer header.
> 
> Signed-off-by: Magdum <magdum.foss@gmail.com>
> ---
>  src/libcamera/control_serializer.cpp         | 222 ++++++++++++++++---
>  test/serialization/control_serialization.cpp |  77 ++++++-
>  2 files changed, 268 insertions(+), 31 deletions(-)
> 
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index b3c44ed0..076e78c5 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -8,6 +8,7 @@
>  #include "libcamera/internal/control_serializer.h"
>  
>  #include <algorithm>
> +#include <limits>
>  #include <memory>
>  #include <vector>
>  
> @@ -50,6 +51,58 @@ bool idMapRequiresLocalIds(enum ipa_controls_id_map_type idMapType)
>         return idMapType == IPA_CONTROL_ID_MAP_V4L2;
>  }
>  
> +size_t serializedControlNameSize(const ControlId *id,
> +                                enum ipa_controls_id_map_type idMapType)
> +{
> +       return idMapRequiresLocalIds(idMapType) ? id->name().size() + 1 : 1;
> +}

This looks like it belongs in the previous patch.

> +
> +bool fitsU32(size_t value)
> +{
> +       return value <= std::numeric_limits<uint32_t>::max();
> +}
> +
> +bool safeMulSizeT(size_t lhs, size_t rhs, size_t *out)
> +{
> +       if (!lhs || !rhs) {
> +               *out = 0;
> +               return true;
> +       }
> +
> +       if (lhs > std::numeric_limits<size_t>::max() / rhs)
> +               return false;
> +
> +       *out = lhs * rhs;
> +       return true;
> +}
> +
> +bool safeAddSizeT(size_t lhs, size_t rhs, size_t *out)
> +{
> +       if (lhs > std::numeric_limits<size_t>::max() - rhs)
> +               return false;
> +
> +       *out = lhs + rhs;
> +       return true;
> +}
> +
> +bool isValidDirection(uint8_t direction)
> +{
> +       constexpr uint8_t kDirectionIn =
> +               static_cast<uint8_t>(ControlId::Direction::In);
> +       constexpr uint8_t kDirectionOut =
> +               static_cast<uint8_t>(ControlId::Direction::Out);
> +       constexpr uint8_t kDirectionInOut = kDirectionIn | kDirectionOut;
> +
> +       switch (direction) {
> +       case kDirectionIn:
> +       case kDirectionOut:
> +       case kDirectionInOut:
> +               return true;
> +       default:
> +               return false;
> +       }
> +}

I think Barnabás covered all the points for these helper functions.

> +
>  } /* namespace */
>  
>  /**
> @@ -184,16 +237,26 @@ size_t ControlSerializer::binarySize(const ControlInfo &info)
>   */
>  size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap)
>  {
> -       size_t size = sizeof(struct ipa_controls_header)
> -                   + infoMap.size() * sizeof(struct ipa_control_info_entry);

I think a comment here preserving the above arithmetric will help readability.

Same for the ControlList binarySize().

> +       size_t entriesSize;
> +       if (!safeMulSizeT(infoMap.size(), sizeof(struct ipa_control_info_entry),
> +                         &entriesSize))
> +               return std::numeric_limits<size_t>::max();
> +
> +       size_t size;
> +       if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize, &size))
> +               return std::numeric_limits<size_t>::max();
>         enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap());
>  
>         for (const auto &ctrl : infoMap) {
> -               size += binarySize(ctrl.second);
> -
> -               size += idMapRequiresLocalIds(idMapType)
> -                               ? ctrl.first->name().size() + 1
> -                               : 1;
> +               size_t nextSize;
> +               if (!safeAddSizeT(size, binarySize(ctrl.second), &nextSize))

Can't we just override size here instead of using nextSize? If it fails we're
going to bail anyway and if it succeeds then we only care about the new value
and not about the old value.

> +                       return std::numeric_limits<size_t>::max();
> +               size = nextSize;
> +
> +               size_t nameSize = serializedControlNameSize(ctrl.first, idMapType);
> +               if (!safeAddSizeT(size, nameSize, &nextSize))

Same here. And every similar occurrence below (there are a lot).


Paul

> +                       return std::numeric_limits<size_t>::max();
> +               size = nextSize;
>         }
>  
>         return size;
> @@ -210,10 +273,21 @@ size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap)
>   */
>  size_t ControlSerializer::binarySize(const ControlList &list)
>  {
> -       size_t size = sizeof(struct ipa_controls_header) + list.size() * sizeof(struct ipa_control_list_entry);
> +       size_t entriesSize;
> +       if (!safeMulSizeT(list.size(), sizeof(struct ipa_control_list_entry),
> +                         &entriesSize))
> +               return std::numeric_limits<size_t>::max();
>  
> -       for (const auto &ctrl : list)
> -               size += binarySize(ctrl.second);
> +       size_t size;
> +       if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize, &size))
> +               return std::numeric_limits<size_t>::max();
> +
> +       for (const auto &ctrl : list) {
> +               size_t nextSize;
> +               if (!safeAddSizeT(size, binarySize(ctrl.second), &nextSize))
> +                       return std::numeric_limits<size_t>::max();
> +               size = nextSize;
> +       }
>  
>         return size;
>  }
> @@ -260,23 +334,70 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,
>         enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap());
>  
>         /* Compute entries and data required sizes. */
> -       size_t entriesSize = infoMap.size()
> -                          * sizeof(struct ipa_control_info_entry);
> +       size_t entriesSize;
> +       if (!safeMulSizeT(infoMap.size(), sizeof(struct ipa_control_info_entry),
> +                         &entriesSize)) {
> +               LOG(Serializer, Error)
> +                       << "ControlInfoMap entries size overflows";
> +               return -E2BIG;
> +       }
> +
>         size_t valuesSize = 0;
>         for (const auto &ctrl : infoMap) {
> -               valuesSize += binarySize(ctrl.second);
> -               valuesSize += idMapRequiresLocalIds(idMapType)
> -                                     ? ctrl.first->name().size() + 1
> -                                     : 1;
> +               size_t valueSize = binarySize(ctrl.second);
> +               size_t nextValuesSize;
> +               if (!safeAddSizeT(valuesSize, valueSize, &nextValuesSize)) {
> +                       LOG(Serializer, Error)
> +                               << "ControlInfoMap values size overflows";
> +                       return -E2BIG;
> +               }
> +               valuesSize = nextValuesSize;
> +
> +               size_t nameSize = serializedControlNameSize(ctrl.first, idMapType);
> +               if (!safeAddSizeT(valuesSize, nameSize, &nextValuesSize)) {
> +                       LOG(Serializer, Error)
> +                               << "ControlInfoMap names size overflows";
> +                       return -E2BIG;
> +               }
> +               valuesSize = nextValuesSize;
> +       }
> +
> +       if (!fitsU32(infoMap.size()) || !fitsU32(entriesSize) ||
> +           !fitsU32(valuesSize)) {
> +               LOG(Serializer, Error)
> +                       << "ControlInfoMap serialization size exceeds wire limits";
> +               return -E2BIG;
> +       }
> +
> +       size_t totalSize;
> +       if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize,
> +                         &totalSize) ||
> +           !safeAddSizeT(totalSize, valuesSize, &totalSize)) {
> +               LOG(Serializer, Error)
> +                       << "ControlInfoMap packet size overflows";
> +               return -E2BIG;
> +       }
> +
> +       size_t dataOffset;
> +       if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize,
> +                         &dataOffset)) {
> +               LOG(Serializer, Error)
> +                       << "ControlInfoMap data offset overflows";
> +               return -E2BIG;
> +       }
> +       if (!fitsU32(totalSize) || !fitsU32(dataOffset)) {
> +               LOG(Serializer, Error)
> +                       << "ControlInfoMap packet header exceeds wire limits";
> +               return -E2BIG;
>         }
>  
>         /* Prepare the packet header. */
>         struct ipa_controls_header hdr = {};
>         hdr.version = IPA_CONTROLS_FORMAT_VERSION;
>         hdr.handle = serial_;
> -       hdr.entries = infoMap.size();
> -       hdr.size = sizeof(hdr) + entriesSize + valuesSize;
> -       hdr.data_offset = sizeof(hdr) + entriesSize;
> +       hdr.entries = static_cast<uint32_t>(infoMap.size());
> +       hdr.size = static_cast<uint32_t>(totalSize);
> +       hdr.data_offset = static_cast<uint32_t>(dataOffset);
>         hdr.id_map_type = idMapType;
>  
>         buffer.write(&hdr);
> @@ -384,18 +505,62 @@ int ControlSerializer::serialize(const ControlList &list,
>         else
>                 idMapType = IPA_CONTROL_ID_MAP_V4L2;
>  
> -       size_t entriesSize = list.size() * sizeof(struct ipa_control_list_entry);
> +       size_t entriesSize;
> +       if (!safeMulSizeT(list.size(), sizeof(struct ipa_control_list_entry),
> +                         &entriesSize)) {
> +               LOG(Serializer, Error)
> +                       << "ControlList entries size overflows";
> +               return -E2BIG;
> +       }
> +
>         size_t valuesSize = 0;
> -       for (const auto &ctrl : list)
> -               valuesSize += binarySize(ctrl.second);
> +       for (const auto &ctrl : list) {
> +               size_t nextValuesSize;
> +               if (!safeAddSizeT(valuesSize, binarySize(ctrl.second),
> +                                 &nextValuesSize)) {
> +                       LOG(Serializer, Error)
> +                               << "ControlList values size overflows";
> +                       return -E2BIG;
> +               }
> +               valuesSize = nextValuesSize;
> +       }
> +
> +       if (!fitsU32(list.size()) || !fitsU32(entriesSize) ||
> +           !fitsU32(valuesSize)) {
> +               LOG(Serializer, Error)
> +                       << "ControlList serialization size exceeds wire limits";
> +               return -E2BIG;
> +       }
> +
> +       size_t totalSize;
> +       if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize,
> +                         &totalSize) ||
> +           !safeAddSizeT(totalSize, valuesSize, &totalSize)) {
> +               LOG(Serializer, Error)
> +                       << "ControlList packet size overflows";
> +               return -E2BIG;
> +       }
> +
> +       size_t dataOffset;
> +       if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize,
> +                         &dataOffset)) {
> +               LOG(Serializer, Error)
> +                       << "ControlList data offset overflows";
> +               return -E2BIG;
> +       }
> +       if (!fitsU32(totalSize) || !fitsU32(dataOffset)) {
> +               LOG(Serializer, Error)
> +                       << "ControlList packet header exceeds wire limits";
> +               return -E2BIG;
> +       }
>  
>         /* Prepare the packet header. */
>         struct ipa_controls_header hdr = {};
>         hdr.version = IPA_CONTROLS_FORMAT_VERSION;
>         hdr.handle = infoMapHandle;
> -       hdr.entries = list.size();
> -       hdr.size = sizeof(hdr) + entriesSize + valuesSize;
> -       hdr.data_offset = sizeof(hdr) + entriesSize;
> +       hdr.entries = static_cast<uint32_t>(list.size());
> +       hdr.size = static_cast<uint32_t>(totalSize);
> +       hdr.data_offset = static_cast<uint32_t>(dataOffset);
>         hdr.id_map_type = idMapType;
>  
>         buffer.write(&hdr);
> @@ -578,6 +743,13 @@ ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
>  
>                 /* If we're using a local id map, populate it with the restored name. */
>                 if (localIdMap) {
> +                       if (!isValidDirection(entry->direction)) {
> +                               LOG(Serializer, Error)
> +                                       << "Control direction is invalid: "
> +                                       << static_cast<unsigned int>(entry->direction);
> +                               return {};
> +                       }
> +
>                         std::string ctrlName(reinterpret_cast<const char *>(nameData),
>                                              entry->name_len);
>  
> diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp
> index b1638db9..0e15988e 100644
> --- a/test/serialization/control_serialization.cpp
> +++ b/test/serialization/control_serialization.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <iostream>
> +#include <string.h>
>  
>  #include <libcamera/camera.h>
>  #include <libcamera/control_ids.h>
> @@ -224,11 +225,13 @@ protected:
>  
>                 /* Reject malformed packets with over-sized names. */
>                 vector<uint8_t> badNameLenData = infoData;
> -               auto *badNameLenHeader =
> -                       reinterpret_cast<ipa_controls_header *>(badNameLenData.data());
> -               auto *badNameLenEntry = reinterpret_cast<ipa_control_info_entry *>(
> -                       badNameLenData.data() + sizeof(*badNameLenHeader));
> -               badNameLenEntry->name_len = 2048;
> +               ipa_control_info_entry badNameLenEntry;
> +               std::memcpy(&badNameLenEntry,
> +                           badNameLenData.data() + sizeof(ipa_controls_header),
> +                           sizeof(badNameLenEntry));
> +               badNameLenEntry.name_len = 2048;
> +               std::memcpy(badNameLenData.data() + sizeof(ipa_controls_header),
> +                           &badNameLenEntry, sizeof(badNameLenEntry));
>  
>                 ControlSerializer badNameLenDeserializer(ControlSerializer::Role::Worker);
>                 buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badNameLenData.data()),
> @@ -240,7 +243,30 @@ protected:
>  
>                 /* Reject malformed packets with non-null-terminated names. */
>                 vector<uint8_t> badTermData = infoData;
> -               badTermData.back() = 'X';
> +               ipa_controls_header badTermHeader;
> +               ipa_control_info_entry badTermEntry;
> +               std::memcpy(&badTermHeader, badTermData.data(), sizeof(badTermHeader));
> +               std::memcpy(&badTermEntry,
> +                           badTermData.data() + sizeof(ipa_controls_header),
> +                           sizeof(badTermEntry));
> +
> +               if (badTermEntry.id != kV4L2TestControlId ||
> +                   badTermEntry.type != ControlTypeInteger32 ||
> +                   badTermEntry.def.type != ControlTypeInteger32 ||
> +                   badTermEntry.def.is_array || badTermEntry.def.count != 1 ||
> +                   badTermEntry.name_len != kV4L2ControlName.size()) {
> +                       cerr << "Malformed test packet layout for bad terminator test" << endl;
> +                       return TestFail;
> +               }
> +
> +               size_t nameStart = badTermHeader.data_offset + badTermEntry.def.offset +
> +                                  sizeof(int32_t);
> +               size_t termOffset = nameStart + badTermEntry.name_len;
> +               if (termOffset >= badTermData.size()) {
> +                       cerr << "Malformed test packet while preparing bad terminator" << endl;
> +                       return TestFail;
> +               }
> +               badTermData[termOffset] = 'X';
>  
>                 ControlSerializer badTermDeserializer(ControlSerializer::Role::Worker);
>                 buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badTermData.data()),
> @@ -278,6 +304,45 @@ protected:
>                         return TestFail;
>                 }
>  
> +               /* Accept names at the configured length limit. */
> +               vector<unique_ptr<ControlId>> maxNameControlIds;
> +               ControlIdMap maxNameIdMap;
> +               string maxName(1024, 'm');
> +
> +               maxNameControlIds.emplace_back(std::make_unique<ControlId>(
> +                       0x009a2003, maxName, "v4l2", ControlTypeInteger32,
> +                       ControlId::Direction::In));
> +               maxNameIdMap.emplace(0x009a2003, maxNameControlIds.back().get());
> +
> +               ControlInfoMap::Map maxNameInfo;
> +               maxNameInfo.emplace(maxNameControlIds.back().get(),
> +                                   ControlInfo(ControlValue(int32_t{ 0 }),
> +                                               ControlValue(int32_t{ 255 }),
> +                                               ControlValue(int32_t{ 16 })));
> +               ControlSerializer maxNameSerializer(ControlSerializer::Role::Proxy);
> +               ControlSerializer maxNameDeserializer(ControlSerializer::Role::Worker);
> +
> +               size = maxNameSerializer.binarySize(maxNameInfoMap);
> +               infoData.resize(size);
> +               buffer = ByteStreamBuffer(infoData.data(), infoData.size());
> +
> +               ret = maxNameSerializer.serialize(maxNameInfoMap, buffer);
> +               if (ret < 0 || buffer.overflow()) {
> +                       cerr << "Max-length control name should serialize successfully" << endl;
> +                       return TestFail;
> +               }
> +
> +               buffer = ByteStreamBuffer(const_cast<const uint8_t *>(infoData.data()),
> +                                         infoData.size());
> +               ControlInfoMap maxNameInfoMapDes =
> +                       maxNameDeserializer.deserialize<ControlInfoMap>(buffer);
> +               auto maxNameIdIt = maxNameInfoMapDes.idmap().find(0x009a2003);
> +               if (maxNameIdIt == maxNameInfoMapDes.idmap().end() ||
> +                   maxNameIdIt->second->name() != maxName) {
> +                       cerr << "Max-length control name round-trip failed" << endl;
> +                       return TestFail;
> +               }
> +
>                 return TestPass;
>         }
>  };
> -- 
> 2.43.0
>

Patch
diff mbox series 

diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
index b3c44ed0..076e78c5 100644
--- a/src/libcamera/control_serializer.cpp
+++ b/src/libcamera/control_serializer.cpp
@@ -8,6 +8,7 @@ 
 #include "libcamera/internal/control_serializer.h"
 
 #include <algorithm>
+#include <limits>
 #include <memory>
 #include <vector>
 
@@ -50,6 +51,58 @@  bool idMapRequiresLocalIds(enum ipa_controls_id_map_type idMapType)
 	return idMapType == IPA_CONTROL_ID_MAP_V4L2;
 }
 
+size_t serializedControlNameSize(const ControlId *id,
+				 enum ipa_controls_id_map_type idMapType)
+{
+	return idMapRequiresLocalIds(idMapType) ? id->name().size() + 1 : 1;
+}
+
+bool fitsU32(size_t value)
+{
+	return value <= std::numeric_limits<uint32_t>::max();
+}
+
+bool safeMulSizeT(size_t lhs, size_t rhs, size_t *out)
+{
+	if (!lhs || !rhs) {
+		*out = 0;
+		return true;
+	}
+
+	if (lhs > std::numeric_limits<size_t>::max() / rhs)
+		return false;
+
+	*out = lhs * rhs;
+	return true;
+}
+
+bool safeAddSizeT(size_t lhs, size_t rhs, size_t *out)
+{
+	if (lhs > std::numeric_limits<size_t>::max() - rhs)
+		return false;
+
+	*out = lhs + rhs;
+	return true;
+}
+
+bool isValidDirection(uint8_t direction)
+{
+	constexpr uint8_t kDirectionIn =
+		static_cast<uint8_t>(ControlId::Direction::In);
+	constexpr uint8_t kDirectionOut =
+		static_cast<uint8_t>(ControlId::Direction::Out);
+	constexpr uint8_t kDirectionInOut = kDirectionIn | kDirectionOut;
+
+	switch (direction) {
+	case kDirectionIn:
+	case kDirectionOut:
+	case kDirectionInOut:
+		return true;
+	default:
+		return false;
+	}
+}
+
 } /* namespace */
 
 /**
@@ -184,16 +237,26 @@  size_t ControlSerializer::binarySize(const ControlInfo &info)
  */
 size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap)
 {
-	size_t size = sizeof(struct ipa_controls_header)
-		    + infoMap.size() * sizeof(struct ipa_control_info_entry);
+	size_t entriesSize;
+	if (!safeMulSizeT(infoMap.size(), sizeof(struct ipa_control_info_entry),
+			  &entriesSize))
+		return std::numeric_limits<size_t>::max();
+
+	size_t size;
+	if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize, &size))
+		return std::numeric_limits<size_t>::max();
 	enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap());
 
 	for (const auto &ctrl : infoMap) {
-		size += binarySize(ctrl.second);
-
-		size += idMapRequiresLocalIds(idMapType)
-				? ctrl.first->name().size() + 1
-				: 1;
+		size_t nextSize;
+		if (!safeAddSizeT(size, binarySize(ctrl.second), &nextSize))
+			return std::numeric_limits<size_t>::max();
+		size = nextSize;
+
+		size_t nameSize = serializedControlNameSize(ctrl.first, idMapType);
+		if (!safeAddSizeT(size, nameSize, &nextSize))
+			return std::numeric_limits<size_t>::max();
+		size = nextSize;
 	}
 
 	return size;
@@ -210,10 +273,21 @@  size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap)
  */
 size_t ControlSerializer::binarySize(const ControlList &list)
 {
-	size_t size = sizeof(struct ipa_controls_header) + list.size() * sizeof(struct ipa_control_list_entry);
+	size_t entriesSize;
+	if (!safeMulSizeT(list.size(), sizeof(struct ipa_control_list_entry),
+			  &entriesSize))
+		return std::numeric_limits<size_t>::max();
 
-	for (const auto &ctrl : list)
-		size += binarySize(ctrl.second);
+	size_t size;
+	if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize, &size))
+		return std::numeric_limits<size_t>::max();
+
+	for (const auto &ctrl : list) {
+		size_t nextSize;
+		if (!safeAddSizeT(size, binarySize(ctrl.second), &nextSize))
+			return std::numeric_limits<size_t>::max();
+		size = nextSize;
+	}
 
 	return size;
 }
@@ -260,23 +334,70 @@  int ControlSerializer::serialize(const ControlInfoMap &infoMap,
 	enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap());
 
 	/* Compute entries and data required sizes. */
-	size_t entriesSize = infoMap.size()
-			   * sizeof(struct ipa_control_info_entry);
+	size_t entriesSize;
+	if (!safeMulSizeT(infoMap.size(), sizeof(struct ipa_control_info_entry),
+			  &entriesSize)) {
+		LOG(Serializer, Error)
+			<< "ControlInfoMap entries size overflows";
+		return -E2BIG;
+	}
+
 	size_t valuesSize = 0;
 	for (const auto &ctrl : infoMap) {
-		valuesSize += binarySize(ctrl.second);
-		valuesSize += idMapRequiresLocalIds(idMapType)
-				      ? ctrl.first->name().size() + 1
-				      : 1;
+		size_t valueSize = binarySize(ctrl.second);
+		size_t nextValuesSize;
+		if (!safeAddSizeT(valuesSize, valueSize, &nextValuesSize)) {
+			LOG(Serializer, Error)
+				<< "ControlInfoMap values size overflows";
+			return -E2BIG;
+		}
+		valuesSize = nextValuesSize;
+
+		size_t nameSize = serializedControlNameSize(ctrl.first, idMapType);
+		if (!safeAddSizeT(valuesSize, nameSize, &nextValuesSize)) {
+			LOG(Serializer, Error)
+				<< "ControlInfoMap names size overflows";
+			return -E2BIG;
+		}
+		valuesSize = nextValuesSize;
+	}
+
+	if (!fitsU32(infoMap.size()) || !fitsU32(entriesSize) ||
+	    !fitsU32(valuesSize)) {
+		LOG(Serializer, Error)
+			<< "ControlInfoMap serialization size exceeds wire limits";
+		return -E2BIG;
+	}
+
+	size_t totalSize;
+	if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize,
+			  &totalSize) ||
+	    !safeAddSizeT(totalSize, valuesSize, &totalSize)) {
+		LOG(Serializer, Error)
+			<< "ControlInfoMap packet size overflows";
+		return -E2BIG;
+	}
+
+	size_t dataOffset;
+	if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize,
+			  &dataOffset)) {
+		LOG(Serializer, Error)
+			<< "ControlInfoMap data offset overflows";
+		return -E2BIG;
+	}
+	if (!fitsU32(totalSize) || !fitsU32(dataOffset)) {
+		LOG(Serializer, Error)
+			<< "ControlInfoMap packet header exceeds wire limits";
+		return -E2BIG;
 	}
 
 	/* Prepare the packet header. */
 	struct ipa_controls_header hdr = {};
 	hdr.version = IPA_CONTROLS_FORMAT_VERSION;
 	hdr.handle = serial_;
-	hdr.entries = infoMap.size();
-	hdr.size = sizeof(hdr) + entriesSize + valuesSize;
-	hdr.data_offset = sizeof(hdr) + entriesSize;
+	hdr.entries = static_cast<uint32_t>(infoMap.size());
+	hdr.size = static_cast<uint32_t>(totalSize);
+	hdr.data_offset = static_cast<uint32_t>(dataOffset);
 	hdr.id_map_type = idMapType;
 
 	buffer.write(&hdr);
@@ -384,18 +505,62 @@  int ControlSerializer::serialize(const ControlList &list,
 	else
 		idMapType = IPA_CONTROL_ID_MAP_V4L2;
 
-	size_t entriesSize = list.size() * sizeof(struct ipa_control_list_entry);
+	size_t entriesSize;
+	if (!safeMulSizeT(list.size(), sizeof(struct ipa_control_list_entry),
+			  &entriesSize)) {
+		LOG(Serializer, Error)
+			<< "ControlList entries size overflows";
+		return -E2BIG;
+	}
+
 	size_t valuesSize = 0;
-	for (const auto &ctrl : list)
-		valuesSize += binarySize(ctrl.second);
+	for (const auto &ctrl : list) {
+		size_t nextValuesSize;
+		if (!safeAddSizeT(valuesSize, binarySize(ctrl.second),
+				  &nextValuesSize)) {
+			LOG(Serializer, Error)
+				<< "ControlList values size overflows";
+			return -E2BIG;
+		}
+		valuesSize = nextValuesSize;
+	}
+
+	if (!fitsU32(list.size()) || !fitsU32(entriesSize) ||
+	    !fitsU32(valuesSize)) {
+		LOG(Serializer, Error)
+			<< "ControlList serialization size exceeds wire limits";
+		return -E2BIG;
+	}
+
+	size_t totalSize;
+	if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize,
+			  &totalSize) ||
+	    !safeAddSizeT(totalSize, valuesSize, &totalSize)) {
+		LOG(Serializer, Error)
+			<< "ControlList packet size overflows";
+		return -E2BIG;
+	}
+
+	size_t dataOffset;
+	if (!safeAddSizeT(sizeof(struct ipa_controls_header), entriesSize,
+			  &dataOffset)) {
+		LOG(Serializer, Error)
+			<< "ControlList data offset overflows";
+		return -E2BIG;
+	}
+	if (!fitsU32(totalSize) || !fitsU32(dataOffset)) {
+		LOG(Serializer, Error)
+			<< "ControlList packet header exceeds wire limits";
+		return -E2BIG;
+	}
 
 	/* Prepare the packet header. */
 	struct ipa_controls_header hdr = {};
 	hdr.version = IPA_CONTROLS_FORMAT_VERSION;
 	hdr.handle = infoMapHandle;
-	hdr.entries = list.size();
-	hdr.size = sizeof(hdr) + entriesSize + valuesSize;
-	hdr.data_offset = sizeof(hdr) + entriesSize;
+	hdr.entries = static_cast<uint32_t>(list.size());
+	hdr.size = static_cast<uint32_t>(totalSize);
+	hdr.data_offset = static_cast<uint32_t>(dataOffset);
 	hdr.id_map_type = idMapType;
 
 	buffer.write(&hdr);
@@ -578,6 +743,13 @@  ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
 
 		/* If we're using a local id map, populate it with the restored name. */
 		if (localIdMap) {
+			if (!isValidDirection(entry->direction)) {
+				LOG(Serializer, Error)
+					<< "Control direction is invalid: "
+					<< static_cast<unsigned int>(entry->direction);
+				return {};
+			}
+
 			std::string ctrlName(reinterpret_cast<const char *>(nameData),
 					     entry->name_len);
 
diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp
index b1638db9..0e15988e 100644
--- a/test/serialization/control_serialization.cpp
+++ b/test/serialization/control_serialization.cpp
@@ -6,6 +6,7 @@ 
  */
 
 #include <iostream>
+#include <string.h>
 
 #include <libcamera/camera.h>
 #include <libcamera/control_ids.h>
@@ -224,11 +225,13 @@  protected:
 
 		/* Reject malformed packets with over-sized names. */
 		vector<uint8_t> badNameLenData = infoData;
-		auto *badNameLenHeader =
-			reinterpret_cast<ipa_controls_header *>(badNameLenData.data());
-		auto *badNameLenEntry = reinterpret_cast<ipa_control_info_entry *>(
-			badNameLenData.data() + sizeof(*badNameLenHeader));
-		badNameLenEntry->name_len = 2048;
+		ipa_control_info_entry badNameLenEntry;
+		std::memcpy(&badNameLenEntry,
+			    badNameLenData.data() + sizeof(ipa_controls_header),
+			    sizeof(badNameLenEntry));
+		badNameLenEntry.name_len = 2048;
+		std::memcpy(badNameLenData.data() + sizeof(ipa_controls_header),
+			    &badNameLenEntry, sizeof(badNameLenEntry));
 
 		ControlSerializer badNameLenDeserializer(ControlSerializer::Role::Worker);
 		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badNameLenData.data()),
@@ -240,7 +243,30 @@  protected:
 
 		/* Reject malformed packets with non-null-terminated names. */
 		vector<uint8_t> badTermData = infoData;
-		badTermData.back() = 'X';
+		ipa_controls_header badTermHeader;
+		ipa_control_info_entry badTermEntry;
+		std::memcpy(&badTermHeader, badTermData.data(), sizeof(badTermHeader));
+		std::memcpy(&badTermEntry,
+			    badTermData.data() + sizeof(ipa_controls_header),
+			    sizeof(badTermEntry));
+
+		if (badTermEntry.id != kV4L2TestControlId ||
+		    badTermEntry.type != ControlTypeInteger32 ||
+		    badTermEntry.def.type != ControlTypeInteger32 ||
+		    badTermEntry.def.is_array || badTermEntry.def.count != 1 ||
+		    badTermEntry.name_len != kV4L2ControlName.size()) {
+			cerr << "Malformed test packet layout for bad terminator test" << endl;
+			return TestFail;
+		}
+
+		size_t nameStart = badTermHeader.data_offset + badTermEntry.def.offset +
+				   sizeof(int32_t);
+		size_t termOffset = nameStart + badTermEntry.name_len;
+		if (termOffset >= badTermData.size()) {
+			cerr << "Malformed test packet while preparing bad terminator" << endl;
+			return TestFail;
+		}
+		badTermData[termOffset] = 'X';
 
 		ControlSerializer badTermDeserializer(ControlSerializer::Role::Worker);
 		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badTermData.data()),
@@ -278,6 +304,45 @@  protected:
 			return TestFail;
 		}
 
+		/* Accept names at the configured length limit. */
+		vector<unique_ptr<ControlId>> maxNameControlIds;
+		ControlIdMap maxNameIdMap;
+		string maxName(1024, 'm');
+
+		maxNameControlIds.emplace_back(std::make_unique<ControlId>(
+			0x009a2003, maxName, "v4l2", ControlTypeInteger32,
+			ControlId::Direction::In));
+		maxNameIdMap.emplace(0x009a2003, maxNameControlIds.back().get());
+
+		ControlInfoMap::Map maxNameInfo;
+		maxNameInfo.emplace(maxNameControlIds.back().get(),
+				    ControlInfo(ControlValue(int32_t{ 0 }),
+						ControlValue(int32_t{ 255 }),
+						ControlValue(int32_t{ 16 })));
+		ControlSerializer maxNameSerializer(ControlSerializer::Role::Proxy);
+		ControlSerializer maxNameDeserializer(ControlSerializer::Role::Worker);
+
+		size = maxNameSerializer.binarySize(maxNameInfoMap);
+		infoData.resize(size);
+		buffer = ByteStreamBuffer(infoData.data(), infoData.size());
+
+		ret = maxNameSerializer.serialize(maxNameInfoMap, buffer);
+		if (ret < 0 || buffer.overflow()) {
+			cerr << "Max-length control name should serialize successfully" << endl;
+			return TestFail;
+		}
+
+		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(infoData.data()),
+					  infoData.size());
+		ControlInfoMap maxNameInfoMapDes =
+			maxNameDeserializer.deserialize<ControlInfoMap>(buffer);
+		auto maxNameIdIt = maxNameInfoMapDes.idmap().find(0x009a2003);
+		if (maxNameIdIt == maxNameInfoMapDes.idmap().end() ||
+		    maxNameIdIt->second->name() != maxName) {
+			cerr << "Max-length control name round-trip failed" << endl;
+			return TestFail;
+		}
+
 		return TestPass;
 	}
 };