[v2,2/2] libcamera: Harden control serializer size and input validation

27507 diff mbox series

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 malformed
name-offset corruption, and max-length control-name boundary
coverage.

Signed-off-by: Magdum <magdum.foss@gmail.com>
---
<<I feel like this function could go into the previous patch.
Agreed. Moved serializedControlNameSize into the patch that first introduces it.

<<You can adjust this:
<<   if (rhs && lhs > std::numeric_limits<size_t>::max() / rhs)
<<     return false;
<< and drop the previous `if`.
Good simplification — the rhs && guard makes the explicit zero-case branch unnecessary since lhs * 0 = 0 falls through correctly anyway.

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

<< 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`.
Good call — replaced both ad-hoc implementations with __builtin_add_overflow / __builtin_mul_overflow wrapped as utils::addOverflow / utils::mulOverflow in utils.h, following the pattern of the existing utils helpers.

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

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

<<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);
Done

========================================================================
CHANGELOG / COMMENTS FROM REPLY.TXT:
========================================================================

========================================================================
 include/libcamera/base/utils.h               |  12 ++
 src/libcamera/control_serializer.cpp         | 196 ++++++++++++++++---
 test/serialization/control_serialization.cpp |  86 ++++++--
 3 files changed, 257 insertions(+), 37 deletions(-)

On Sat, Jul 25, 2026 at 03:14:07PM +0200, Magdum wrote:
> 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 malformed
> name-offset corruption, and max-length control-name boundary
> coverage.
> 
> Signed-off-by: Magdum <magdum.foss@gmail.com>
> ---
> <<I feel like this function could go into the previous patch.
> Agreed. Moved serializedControlNameSize into the patch that first introduces it.

The proper way to reply to review comments is to reply to the e-mail
from the reviewer, not adding comments in the next version.

> <<You can adjust this:
> <<   if (rhs && lhs > std::numeric_limits<size_t>::max() / rhs)
> <<     return false;
> << and drop the previous `if`.
> Good simplification — the rhs && guard makes the explicit zero-case branch unnecessary since lhs * 0 = 0 falls through correctly anyway.
> 
> <<You can just say `return *out >= lhs;` and drop the `if`.
> Done
> 
> << 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`.
> Good call — replaced both ad-hoc implementations with __builtin_add_overflow / __builtin_mul_overflow wrapped as utils::addOverflow / utils::mulOverflow in utils.h, following the pattern of the existing utils helpers.
> 
> <<You can use `utils::to_underlying()`.
> Done
> 
> <<You can just say `case (kDirectionIn | kDirectionOut):` here
> Done
> 
> <<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);
> Done
> 
> ========================================================================
> CHANGELOG / COMMENTS FROM REPLY.TXT:
> ========================================================================

What is REPLY.TXT ?

This makes me feel you're using generative AI to write those patches.
Writing an AI policy for libcamera is on my todo list. It will forbid
the use of generative AI to contribute to the project. As such, I don't
think this series is acceptable.

> ========================================================================
>  include/libcamera/base/utils.h               |  12 ++
>  src/libcamera/control_serializer.cpp         | 196 ++++++++++++++++---
>  test/serialization/control_serialization.cpp |  86 ++++++--
>  3 files changed, 257 insertions(+), 37 deletions(-)
> 
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index 9835fd798..d73db428c 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -449,6 +449,18 @@ constexpr details::defopt_t defopt;
>  std::ostream &operator<<(std::ostream &os, const Duration &d);
>  #endif
>  
> +template<typename T>
> +bool addOverflow(T lhs, T rhs, T *result)
> +{
> +	return __builtin_add_overflow(lhs, rhs, result);
> +}
> +
> +template<typename T>
> +bool mulOverflow(T lhs, T rhs, T *result)
> +{
> +	return __builtin_mul_overflow(lhs, rhs, result);
> +}
> +
>  } /* namespace utils */
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
> index a420d4700..ceafff326 100644
> --- a/src/libcamera/control_serializer.cpp
> +++ b/src/libcamera/control_serializer.cpp
> @@ -8,11 +8,13 @@
>  #include "libcamera/internal/control_serializer.h"
>  
>  #include <algorithm>
> +#include <limits>
>  #include <memory>
>  #include <vector>
>  
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/span.h>
> +#include <libcamera/base/utils.h>
>  
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
> @@ -53,7 +55,31 @@ bool idMapRequiresLocalIds(enum ipa_controls_id_map_type idMapType)
>  size_t serializedControlNameSize(const ControlId *id,
>  		enum ipa_controls_id_map_type idMapType)
>  {
> -	return idMapRequiresLocalIds(idMapType) ? id->name().size() + 1 : 1;
> +	return idMapRequiresLocalIds(idMapType) ? id->name().size() : 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)
> +{
> +	return !utils::mulOverflow(lhs, rhs, out);
> +}
> +
> +bool safeAddSizeT(size_t lhs, size_t rhs, size_t *out)
> +{
> +	return !utils::addOverflow(lhs, rhs, out);
> +}
> +
> +bool isValidDirection(uint8_t direction)
> +{
> +	constexpr unsigned int valid =
> +		utils::to_underlying(ControlId::Direction::In) |
> +		utils::to_underlying(ControlId::Direction::Out);
> +
> +	return (direction & valid) && !(direction & ~valid);
>  }
>  
>  } /* namespace */
> @@ -190,14 +216,27 @@ 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 += serializedControlNameSize(ctrl.first, idMapType);
> +		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;
> @@ -214,10 +253,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();
> +
> +	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 += binarySize(ctrl.second);
> +	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;
>  }
> @@ -264,23 +314,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()
> -				      : 0;
> +		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);
> @@ -389,18 +486,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);
> @@ -586,6 +727,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 b1638db9f..1fb8cc8b7 100644
> --- a/test/serialization/control_serialization.cpp
> +++ b/test/serialization/control_serialization.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <iostream>
> +#include <cstring>
>  
>  #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()),
> @@ -238,15 +241,31 @@ protected:
>  			return TestFail;
>  		}
>  
> -		/* Reject malformed packets with non-null-terminated names. */
> -		vector<uint8_t> badTermData = infoData;
> -		badTermData.back() = 'X';
> +		/* Reject malformed packets with inconsistent name offsets. */
> +		vector<uint8_t> badNameOffsetData = infoData;
> +		ipa_control_info_entry badNameOffsetEntry;
> +		std::memcpy(&badNameOffsetEntry,
> +			    badNameOffsetData.data() + sizeof(ipa_controls_header),
> +			    sizeof(badNameOffsetEntry));
> +
> +		if (badNameOffsetEntry.id != kV4L2TestControlId ||
> +		    badNameOffsetEntry.type != ControlTypeInteger32 ||
> +		    badNameOffsetEntry.def.type != ControlTypeInteger32 ||
> +		    badNameOffsetEntry.def.is_array || badNameOffsetEntry.def.count != 1 ||
> +		    badNameOffsetEntry.name_len != kV4L2ControlName.size()) {
> +			cerr << "Malformed test packet layout for bad name offset test" << endl;
> +			return TestFail;
> +		}
> +
> +		badNameOffsetEntry.name_offset += 1;
> +		std::memcpy(badNameOffsetData.data() + sizeof(ipa_controls_header),
> +			    &badNameOffsetEntry, sizeof(badNameOffsetEntry));
>  
> -		ControlSerializer badTermDeserializer(ControlSerializer::Role::Worker);
> -		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badTermData.data()),
> -					  badTermData.size());
> -		if (!badTermDeserializer.deserialize<ControlInfoMap>(buffer).empty()) {
> -			cerr << "Control name without null terminator should be rejected" << endl;
> +		ControlSerializer badNameOffsetDeserializer(ControlSerializer::Role::Worker);
> +		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badNameOffsetData.data()),
> +					  badNameOffsetData.size());
> +		if (!badNameOffsetDeserializer.deserialize<ControlInfoMap>(buffer).empty()) {
> +			cerr << "Control with inconsistent name offset should be rejected" << endl;
>  			return TestFail;
>  		}
>  
> @@ -278,6 +297,47 @@ 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 })));
> +		ControlInfoMap maxNameInfoMap(std::move(maxNameInfo), maxNameIdMap);
> +
> +		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;
>  	}
>  };