[1/2] libcamera: Serialize local control names in IPA format v3

27494 diff mbox series

diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h
index 6af962ff..7a7873d9 100644
--- a/include/libcamera/ipa/ipa_controls.h
+++ b/include/libcamera/ipa/ipa_controls.h
@@ -15,7 +15,7 @@  namespace libcamera {
 extern "C" {
 #endif
 
-#define IPA_CONTROLS_FORMAT_VERSION	2
+#define IPA_CONTROLS_FORMAT_VERSION 3
 
 enum ipa_controls_id_map_type {
 	IPA_CONTROL_ID_MAP_CONTROLS,
@@ -50,7 +50,9 @@  struct ipa_control_info_entry {
 	uint32_t id;
 	uint32_t type;
 	uint8_t direction;
-	uint8_t padding[7];
+	uint8_t padding[3];
+	/* Length of the control name without the terminating null byte. */
+	uint32_t name_len;
 	struct ipa_control_value_entry min;
 	struct ipa_control_value_entry max;
 	struct ipa_control_value_entry def;
diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp
index c0285cc6..b3c44ed0 100644
--- a/src/libcamera/control_serializer.cpp
+++ b/src/libcamera/control_serializer.cpp
@@ -31,6 +31,27 @@  namespace libcamera {
 
 LOG_DEFINE_CATEGORY(Serializer)
 
+namespace {
+
+constexpr uint32_t kMaxControlNameLength = 1024;
+
+enum ipa_controls_id_map_type idMapTypeFor(const ControlIdMap &idmap)
+{
+	if (&idmap == &controls::controls)
+		return IPA_CONTROL_ID_MAP_CONTROLS;
+	if (&idmap == &properties::properties)
+		return IPA_CONTROL_ID_MAP_PROPERTIES;
+
+	return IPA_CONTROL_ID_MAP_V4L2;
+}
+
+bool idMapRequiresLocalIds(enum ipa_controls_id_map_type idMapType)
+{
+	return idMapType == IPA_CONTROL_ID_MAP_V4L2;
+}
+
+} /* namespace */
+
 /**
  * \class ControlSerializer
  * \brief Serializer and deserializer for control-related classes
@@ -165,10 +186,16 @@  size_t ControlSerializer::binarySize(const ControlInfoMap &infoMap)
 {
 	size_t size = sizeof(struct ipa_controls_header)
 		    + infoMap.size() * sizeof(struct ipa_control_info_entry);
+	enum ipa_controls_id_map_type idMapType = idMapTypeFor(infoMap.idmap());
 
-	for (const auto &ctrl : infoMap)
+	for (const auto &ctrl : infoMap) {
 		size += binarySize(ctrl.second);
 
+		size += idMapRequiresLocalIds(idMapType)
+				? ctrl.first->name().size() + 1
+				: 1;
+	}
+
 	return size;
 }
 
@@ -183,8 +210,7 @@  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 size = sizeof(struct ipa_controls_header) + list.size() * sizeof(struct ipa_control_list_entry);
 
 	for (const auto &ctrl : list)
 		size += binarySize(ctrl.second);
@@ -231,24 +257,21 @@  int ControlSerializer::serialize(const ControlInfoMap &infoMap,
 		return 0;
 	}
 
+	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 valuesSize = 0;
-	for (const auto &ctrl : infoMap)
+	for (const auto &ctrl : infoMap) {
 		valuesSize += binarySize(ctrl.second);
-
-	const ControlIdMap *idmap = &infoMap.idmap();
-	enum ipa_controls_id_map_type idMapType;
-	if (idmap == &controls::controls)
-		idMapType = IPA_CONTROL_ID_MAP_CONTROLS;
-	else if (idmap == &properties::properties)
-		idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;
-	else
-		idMapType = IPA_CONTROL_ID_MAP_V4L2;
+		valuesSize += idMapRequiresLocalIds(idMapType)
+				      ? ctrl.first->name().size() + 1
+				      : 1;
+	}
 
 	/* Prepare the packet header. */
-	struct ipa_controls_header hdr;
+	struct ipa_controls_header hdr = {};
 	hdr.version = IPA_CONTROLS_FORMAT_VERSION;
 	hdr.handle = serial_;
 	hdr.entries = infoMap.size();
@@ -267,21 +290,29 @@  int ControlSerializer::serialize(const ControlInfoMap &infoMap,
 	 */
 	serial_ += 2;
 
-	/*
-	 * Serialize all entries.
-	 * \todo Serialize the control name too
-	 */
+	/* Serialize all entries. */
 	ByteStreamBuffer entries = buffer.carveOut(entriesSize);
 	ByteStreamBuffer values = buffer.carveOut(valuesSize);
+	static const std::string emptyName;
 
 	for (const auto &ctrl : infoMap) {
 		const ControlId *id = ctrl.first;
 		const ControlInfo &info = ctrl.second;
+		const std::string &name = idMapRequiresLocalIds(idMapType)
+						  ? id->name()
+						  : emptyName;
+
+		if (name.size() > kMaxControlNameLength) {
+			LOG(Serializer, Error)
+				<< "Control name too long: " << name.size();
+			return -EINVAL;
+		}
 
-		struct ipa_control_info_entry entry;
+		struct ipa_control_info_entry entry = {};
 		entry.id = id->id();
 		entry.type = id->type();
 		entry.direction = static_cast<ControlId::DirectionFlags::Type>(id->direction());
+		entry.name_len = static_cast<uint32_t>(name.size());
 
 		populateControlValueEntry(entry.min, info.min(), values.offset());
 		store(info.min(), values);
@@ -292,6 +323,10 @@  int ControlSerializer::serialize(const ControlInfoMap &infoMap,
 		populateControlValueEntry(entry.def, info.def(), values.offset());
 		store(info.def(), values);
 
+		values.write(Span<const uint8_t>(
+			reinterpret_cast<const uint8_t *>(name.c_str()),
+			name.size() + 1));
+
 		entries.write(&entry);
 	}
 
@@ -355,7 +390,7 @@  int ControlSerializer::serialize(const ControlList &list,
 		valuesSize += binarySize(ctrl.second);
 
 	/* Prepare the packet header. */
-	struct ipa_controls_header hdr;
+	struct ipa_controls_header hdr = {};
 	hdr.version = IPA_CONTROLS_FORMAT_VERSION;
 	hdr.handle = infoMapHandle;
 	hdr.entries = list.size();
@@ -485,25 +520,6 @@  ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
 
 		ControlType type = static_cast<ControlType>(entry->type);
 
-		/* If we're using a local id map, populate it. */
-		if (localIdMap) {
-			ControlId::DirectionFlags flags{
-				static_cast<ControlId::Direction>(entry->direction)
-			};
-
-			/**
-			 * \todo Find a way to preserve the control name for
-			 * debugging purpose.
-			 */
-			controlIds_.emplace_back(std::make_unique<ControlId>(entry->id,
-									     "", "local", type,
-									     flags));
-			(*localIdMap)[entry->id] = controlIds_.back().get();
-		}
-
-		const ControlId *controlId = idMap->at(entry->id);
-		ASSERT(controlId);
-
 		const ipa_control_value_entry &min_entry = entry->min;
 		const ipa_control_value_entry &max_entry = entry->max;
 		const ipa_control_value_entry &def_entry = entry->def;
@@ -538,6 +554,45 @@  ControlInfoMap ControlSerializer::deserialize<ControlInfoMap>(ByteStreamBuffer &
 			loadControlValue(values, static_cast<ControlType>(def_entry.type),
 					 def_entry.is_array, def_entry.count);
 
+		/*
+		 * Deserialize the null-terminated control name from the values
+		 * section. Reject unreasonably long names to guard against
+		 * malformed packets.
+		 */
+		if (entry->name_len > kMaxControlNameLength) {
+			LOG(Serializer, Error)
+				<< "Control name too long: " << entry->name_len;
+			return {};
+		}
+
+		const auto *nameData = values.read<const uint8_t>(entry->name_len + 1);
+		if (!nameData) {
+			LOG(Serializer, Error) << "Out of data reading control name";
+			return {};
+		}
+
+		if (nameData[entry->name_len] != '\0') {
+			LOG(Serializer, Error) << "Control name is not null-terminated";
+			return {};
+		}
+
+		/* If we're using a local id map, populate it with the restored name. */
+		if (localIdMap) {
+			std::string ctrlName(reinterpret_cast<const char *>(nameData),
+					     entry->name_len);
+
+			ControlId::DirectionFlags flags{
+				static_cast<ControlId::Direction>(entry->direction)
+			};
+
+			controlIds_.emplace_back(std::make_unique<ControlId>(entry->id,
+									     ctrlName, "local",
+									     type, flags));
+			(*localIdMap)[entry->id] = controlIds_.back().get();
+		}
+
+		const ControlId *controlId = idMap->at(entry->id);
+		ASSERT(controlId);
 
 		/* Create and store the ControlInfo. */
 		ctrls.emplace(controlId, ControlInfo(min, max, def));
diff --git a/test/serialization/control_serialization.cpp b/test/serialization/control_serialization.cpp
index 06c572b7..b1638db9 100644
--- a/test/serialization/control_serialization.cpp
+++ b/test/serialization/control_serialization.cpp
@@ -11,6 +11,8 @@ 
 #include <libcamera/control_ids.h>
 #include <libcamera/controls.h>
 
+#include <libcamera/ipa/ipa_controls.h>
+
 #include "libcamera/internal/byte_stream_buffer.h"
 #include "libcamera/internal/control_serializer.h"
 
@@ -169,6 +171,113 @@  protected:
 			return TestFail;
 		}
 
+		/* Build a local (V4L2-like) ControlInfoMap and verify name round-trip. */
+		vector<unique_ptr<ControlId>> v4l2ControlIds;
+		ControlIdMap v4l2IdMap;
+		constexpr uint32_t kV4L2TestControlId = 0x009a2001;
+		const string kV4L2ControlName = "V4L2_CID_TEST_GAIN";
+
+		v4l2ControlIds.emplace_back(std::make_unique<ControlId>(
+			kV4L2TestControlId, kV4L2ControlName, "v4l2",
+			ControlTypeInteger32, ControlId::Direction::In));
+		v4l2IdMap.emplace(kV4L2TestControlId, v4l2ControlIds.back().get());
+
+		ControlInfoMap::Map v4l2Info;
+		v4l2Info.emplace(v4l2ControlIds.back().get(),
+				 ControlInfo(ControlValue(int32_t{ 0 }),
+					     ControlValue(int32_t{ 255 }),
+					     ControlValue(int32_t{ 16 })));
+		ControlInfoMap v4l2InfoMap(std::move(v4l2Info), v4l2IdMap);
+
+		ControlSerializer v4l2Serializer(ControlSerializer::Role::Proxy);
+		ControlSerializer v4l2Deserializer(ControlSerializer::Role::Worker);
+
+		size = v4l2Serializer.binarySize(v4l2InfoMap);
+		infoData.resize(size);
+		buffer = ByteStreamBuffer(infoData.data(), infoData.size());
+
+		ret = v4l2Serializer.serialize(v4l2InfoMap, buffer);
+		if (ret < 0 || buffer.overflow()) {
+			cerr << "Failed to serialize V4L2-like ControlInfoMap" << endl;
+			return TestFail;
+		}
+
+		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(infoData.data()),
+					  infoData.size());
+		ControlInfoMap v4l2InfoMapDes =
+			v4l2Deserializer.deserialize<ControlInfoMap>(buffer);
+		if (v4l2InfoMapDes.empty()) {
+			cerr << "Failed to deserialize V4L2-like ControlInfoMap" << endl;
+			return TestFail;
+		}
+
+		auto idIt = v4l2InfoMapDes.idmap().find(kV4L2TestControlId);
+		if (idIt == v4l2InfoMapDes.idmap().end()) {
+			cerr << "Deserialized V4L2-like id map misses test control" << endl;
+			return TestFail;
+		}
+
+		if (idIt->second->name() != kV4L2ControlName) {
+			cerr << "Deserialized V4L2-like control name doesn't match" << endl;
+			return TestFail;
+		}
+
+		/* 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;
+
+		ControlSerializer badNameLenDeserializer(ControlSerializer::Role::Worker);
+		buffer = ByteStreamBuffer(const_cast<const uint8_t *>(badNameLenData.data()),
+					  badNameLenData.size());
+		if (!badNameLenDeserializer.deserialize<ControlInfoMap>(buffer).empty()) {
+			cerr << "Oversized control name should be rejected" << endl;
+			return TestFail;
+		}
+
+		/* Reject malformed packets with non-null-terminated names. */
+		vector<uint8_t> badTermData = infoData;
+		badTermData.back() = 'X';
+
+		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;
+			return TestFail;
+		}
+
+		/* Reject too-long names at serialization time. */
+		vector<unique_ptr<ControlId>> longNameControlIds;
+		ControlIdMap longNameIdMap;
+		string longName(1025, 'n');
+
+		longNameControlIds.emplace_back(std::make_unique<ControlId>(
+			0x009a2002, longName, "v4l2", ControlTypeInteger32,
+			ControlId::Direction::In));
+		longNameIdMap.emplace(0x009a2002, longNameControlIds.back().get());
+
+		ControlInfoMap::Map longNameInfo;
+		longNameInfo.emplace(longNameControlIds.back().get(),
+				     ControlInfo(ControlValue(int32_t{ 0 }),
+						 ControlValue(int32_t{ 255 }),
+						 ControlValue(int32_t{ 16 })));
+		ControlInfoMap longNameInfoMap(std::move(longNameInfo), longNameIdMap);
+
+		ControlSerializer longNameSerializer(ControlSerializer::Role::Proxy);
+		size = longNameSerializer.binarySize(longNameInfoMap);
+		infoData.resize(size);
+		buffer = ByteStreamBuffer(infoData.data(), infoData.size());
+
+		ret = longNameSerializer.serialize(longNameInfoMap, buffer);
+		if (ret != -EINVAL) {
+			cerr << "Too-long control name should fail serialization" << endl;
+			return TestFail;
+		}
+
 		return TestPass;
 	}
 };