fix(mavlink): align signing with MAVLink spec and fix performance regression

Remove the non-standard MAV_SIGN_CFG parameter and align the signing
implementation with the MAVLink specification.

Key changes:
- Remove MAV_SIGN_CFG parameter that conflicted with GCS implementations
- Only enable signing when a valid key is present on the SD card
- Accept SETUP_SIGNING on any link, not just USB
- Reject SETUP_SIGNING while the vehicle is armed
- Allow disabling signing via signed all-zero key SETUP_SIGNING message
- Propagate key changes to all mavlink instances
- Zero CPU/bandwidth overhead when signing is not active

Fixes #26893

Signed-off-by: Ramon Roche <mrpollo@gmail.com>

src/modules/mavlink/mavlink_main.cpp

@@ -97,10 +97,14 @@ mavlink_message_t *mavlink_get_channel_buffer(uint8_t channel) { return mavlink_
 
 static bool accept_unsigned_callback(const mavlink_status_t *status, uint32_t message_id)
 {
-	Mavlink *m = Mavlink::get_instance_for_status(status);
+	// Use link_id to index directly: the callback fires on the instance's own
+	// receiver thread, so no lock needed (instance can't be destroyed while running).
+	if (status->signing) {
+		Mavlink *inst = mavlink_module_instances[status->signing->link_id];
 
-	if (m != nullptr) {
-		return m -> accept_unsigned(m->sign_mode(), m -> is_usb_uart(), message_id);
+		if (inst != nullptr) {
+			return inst->accept_unsigned(message_id);
+		}
 	}
 
 	return false;
@@ -325,20 +329,6 @@ Mavlink::get_instance_for_device(const char *device_name)
 	return nullptr;
 }
 
-Mavlink *
-Mavlink::get_instance_for_status(const mavlink_status_t *status)
-{
-	LockGuard lg{mavlink_module_mutex};
-
-	for (Mavlink *inst : mavlink_module_instances) {
-		if (status == mavlink_get_channel_status(inst->get_instance_id())) {
-			return inst;
-		}
-	}
-
-	return nullptr;
-}
-
 #ifdef MAVLINK_UDP
 Mavlink *
 Mavlink::get_instance_for_network_port(unsigned long port)
@@ -1054,10 +1044,50 @@ Mavlink::handle_message(const mavlink_message_t *msg)
 	 *  NOTE: this is called from the receiver thread
 	 */
 
-	if (is_usb_uart()) {
-		if (_sign_control.check_for_signing(msg)) {
+	// SETUP_SIGNING must never be forwarded to other links (MAVLink spec requirement).
+	if (msg->msgid == MAVLINK_MSG_ID_SETUP_SIGNING) {
+		// Reject signing changes while armed
+		vehicle_status_s vehicle_status{};
+
+		if (_vehicle_status_sub.copy(&vehicle_status)
+		    && vehicle_status.arming_state == vehicle_status_s::ARMING_STATE_ARMED) {
+			send_statustext_critical("MAVLink signing: rejected while armed");
 			return;
 		}
+
+		MavlinkSignControl::SetupSigningResult result = _sign_control.check_for_signing(msg);
+
+		switch (result) {
+		case MavlinkSignControl::KEY_ACCEPTED:
+			send_statustext_info("MAVLink signing key accepted");
+			break;
+
+		case MavlinkSignControl::SIGNING_DISABLED:
+			send_statustext_info("MAVLink signing disabled");
+			break;
+
+		case MavlinkSignControl::BLANK_KEY_REJECTED:
+			send_statustext_critical("MAVLink signing: blank key rejected");
+			break;
+
+		default:
+			break;
+		}
+
+		if (result == MavlinkSignControl::KEY_ACCEPTED || result == MavlinkSignControl::SIGNING_DISABLED) {
+			// Signal all other instances to reload key from file on their own thread
+			LockGuard lg{mavlink_module_mutex};
+
+			for (int i = 0; i < MAVLINK_COMM_NUM_BUFFERS; i++) {
+				Mavlink *inst = mavlink_module_instances[i];
+
+				if (inst != nullptr && inst != this) {
+					inst->set_signing_key_dirty();
+				}
+			}
+		}
+
+		return;
 	}
 
 	if (get_forwarding_on()) {
@@ -1976,8 +2006,6 @@ Mavlink::task_main(int argc, char *argv[])
 		}
 	}
 
-	_sign_control.start(_instance_id, get_status(), &accept_unsigned_callback);
-
 	int ch;
 	_baudrate = 57600;
 	_datarate = 0;
@@ -2317,6 +2345,8 @@ Mavlink::task_main(int argc, char *argv[])
 		return PX4_ERROR;
 	}
 
+	_sign_control.start(get_instance_id(), get_status(), &accept_unsigned_callback);
+
 	pthread_mutex_init(&_mavlink_shell_mutex, nullptr);
 	pthread_mutex_init(&_message_buffer_mutex, nullptr);
 	pthread_mutex_init(&_send_mutex, nullptr);

src/modules/mavlink/mavlink_main.h

@@ -136,7 +136,6 @@ public:
 
 	mavlink_message_t *get_buffer() { return &_mavlink_buffer; }
 	mavlink_status_t *get_status() { return &_mavlink_status; }
-	static Mavlink *get_instance_for_status(const mavlink_status_t *status);
 
 	void setProtocolVersion(uint8_t version);
 	uint8_t getProtocolVersion() const { return _protocol_version; };
@@ -470,7 +469,6 @@ public:
 	bool ftp_enabled() const { return _ftp_on; }
 
 	bool hash_check_enabled() const { return _param_mav_hash_chk_en.get(); }
-	int32_t sign_mode() const { return _param_mav_sign_cfg.get(); }
 	bool forward_heartbeats_enabled() const { return _param_mav_hb_forw_en.get(); }
 
 	bool failure_injection_enabled() const { return _param_sys_failure_injection_enabled.get(); }
@@ -494,7 +492,9 @@ public:
 
 	bool radio_status_critical() const { return _radio_status_critical; }
 
-	bool accept_unsigned(int32_t sign_mode, bool is_usb_uart, uint32_t message_id) { return _sign_control.accept_unsigned(sign_mode, is_usb_uart, message_id); }
+	bool accept_unsigned(uint32_t message_id) { return _sign_control.accept_unsigned(message_id); }
+	void set_signing_key_dirty() { _signing_key_dirty.store(true); }
+	void check_signing_key_dirty() { if (_signing_key_dirty.load()) { _signing_key_dirty.store(false); _sign_control.reload_key(); } }
 
 
 private:
@@ -507,6 +507,7 @@ private:
 
 	px4::atomic_bool	_task_should_exit{false};
 	px4::atomic_bool	_task_running{false};
+	px4::atomic_bool	_signing_key_dirty{false};
 
 	bool			_transmitting_enabled{true};
 	bool			_transmitting_enabled_commanded{false};
@@ -641,7 +642,6 @@ private:
 		(ParamBool<px4::params::MAV_USEHILGPS>) _param_mav_usehilgps,
 		(ParamBool<px4::params::MAV_FWDEXTSP>) _param_mav_fwdextsp,
 		(ParamBool<px4::params::MAV_HASH_CHK_EN>) _param_mav_hash_chk_en,
-		(ParamInt<px4::params::MAV_SIGN_CFG>) _param_mav_sign_cfg,
 		(ParamBool<px4::params::MAV_HB_FORW_EN>) _param_mav_hb_forw_en,
 		(ParamInt<px4::params::MAV_RADIO_TOUT>)      _param_mav_radio_timeout,
 		(ParamInt<px4::params::SYS_HITL>) _param_sys_hitl,

src/modules/mavlink/mavlink_params.yaml

@@ -18,15 +18,6 @@ parameters:
       min: 1
       max: 250
       reboot_required: true
-    MAV_SIGN_CFG:
-      description:
-        short: MAVLink protocol signing
-      type: enum
-      values:
-        0: Message signing disabled
-        1: Signing enabled except on USB
-        2: Signing always enabled
-      default: 0
     MAV_PROTO_VER:
       description:
         short: MAVLink protocol version

src/modules/mavlink/mavlink_receiver.cpp

@@ -3272,6 +3272,9 @@ MavlinkReceiver::run()
 			updateParams();
 		}
 
+		// Reload signing key if another instance updated it
+		_mavlink.check_signing_key_dirty();
+
 		int ret = poll(&fds[0], 1, timeout);
 
 		if (ret > 0) {

src/modules/mavlink/mavlink_sign_control.cpp

@@ -43,7 +43,10 @@
 
 static mavlink_signing_streams_t global_mavlink_signing_streams = {};
 
+// Messages accepted without signing per MAVLink spec recommendation.
+// HEARTBEAT is required for link discovery/interop but allows spoofed phantom vehicles on GCS.
 static const uint32_t unsigned_messages[] = {
+	MAVLINK_MSG_ID_HEARTBEAT,
 	MAVLINK_MSG_ID_RADIO_STATUS,
 	MAVLINK_MSG_ID_ADSB_VEHICLE,
 	MAVLINK_MSG_ID_COLLISION
@@ -57,11 +60,11 @@ MavlinkSignControl::~MavlinkSignControl()
 {
 }
 
-void MavlinkSignControl::start(int _instance_id, mavlink_status_t *_mavlink_status,
+void MavlinkSignControl::start(int instance_id, mavlink_status_t *mavlink_status,
 			       mavlink_accept_unsigned_t accept_unsigned_callback)
 {
-	_mavlink_signing.link_id = _instance_id;
-	_mavlink_signing.flags = MAVLINK_SIGNING_FLAG_SIGN_OUTGOING;
+	_mavlink_status = mavlink_status;
+	_mavlink_signing.link_id = instance_id;
 	_mavlink_signing.accept_unsigned_callback = accept_unsigned_callback;
 	_is_signing_initialized = false;
 
@@ -71,19 +74,18 @@ void MavlinkSignControl::start(int _instance_id, mavlink_status_t *_mavlink_stat
 		PX4_ERR("failed creating module storage dir: %s (%i)", MAVLINK_FOLDER_PATH, errno);
 
 	} else {
-		int _fd = ::open(MAVLINK_SECRET_FILE, O_CREAT | O_RDONLY, PX4_O_MODE_600);
+		int fd = ::open(MAVLINK_SECRET_FILE, O_RDONLY);
 
-		if (_fd == -1) {
+		if (fd == -1) {
 			if (errno != ENOENT) {
-				PX4_ERR("failed creating mavlink secret key file: %s (%i)", MAVLINK_SECRET_FILE, errno);
+				PX4_ERR("failed opening mavlink secret key file: %s (%i)", MAVLINK_SECRET_FILE, errno);
 			}
 
 		} else {
-			//if we dont have enough bytes we simply ignore it , because it may be not set yet
-			ssize_t bytes_read = ::read(_fd, _mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH);
+			ssize_t bytes_read = ::read(fd, _mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH);
 
 			if (bytes_read == MAVLINK_SECRET_KEY_LENGTH) {
-				bytes_read = ::read(_fd, &_mavlink_signing.timestamp, MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH);
+				bytes_read = ::read(fd, &_mavlink_signing.timestamp, MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH);
 
 				if (bytes_read == MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH) {
 					if (_mavlink_signing.timestamp != 0 || !is_array_all_zeros(_mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH)) {
@@ -92,99 +94,145 @@ void MavlinkSignControl::start(int _instance_id, mavlink_status_t *_mavlink_stat
 				}
 			}
 
-			close(_fd);
+			close(fd);
 		}
 	}
 
-	//lets reset it to nulls if it was not read properly
 	if (!_is_signing_initialized) {
-		for (size_t i = 0; i < MAVLINK_SECRET_KEY_LENGTH; ++i) {
-			_mavlink_signing.secret_key[i] = 0;
-		}
-
+		memset(_mavlink_signing.secret_key, 0, MAVLINK_SECRET_KEY_LENGTH);
 		_mavlink_signing.timestamp = 0;
 	}
 
-	// copy pointer of the signing to status struct
-	_mavlink_status -> signing = &_mavlink_signing;
-	_mavlink_status -> signing_streams = &global_mavlink_signing_streams;
+	_update_signing_state();
 }
 
-bool MavlinkSignControl::check_for_signing(const mavlink_message_t *msg)
+MavlinkSignControl::SetupSigningResult MavlinkSignControl::check_for_signing(const mavlink_message_t *msg)
 {
 	if (msg->msgid != MAVLINK_MSG_ID_SETUP_SIGNING) {
-		return false;
+		return NOT_SETUP_SIGNING;
 	}
 
 	mavlink_setup_signing_t setup_signing;
 	mavlink_msg_setup_signing_decode(msg, &setup_signing);
 
-	//setup signing provides new key , lets update it
-	//we update it only in case everything was stored properly
-	memcpy(_mavlink_signing.secret_key, setup_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH);
-	_mavlink_signing.timestamp = setup_signing.initial_timestamp;
+	bool new_key_blank = (setup_signing.initial_timestamp == 0
+			      && is_array_all_zeros(setup_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH));
 
-	if (setup_signing.initial_timestamp != 0 || !is_array_all_zeros(setup_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH)) {
-		_is_signing_initialized = true;
+	if (new_key_blank) {
+		// Disable signing: only allowed if signing is active and the message is signed
+		if (!_is_signing_initialized) {
+			// Already disabled, nothing to do
+			return SIGNING_DISABLED;
+		}
 
-	} else {
+		bool msg_is_signed = (msg->incompat_flags & MAVLINK_IFLAG_SIGNED);
+
+		if (!msg_is_signed) {
+			PX4_WARN("SETUP_SIGNING blank key rejected: message must be signed");
+			return BLANK_KEY_REJECTED;
+		}
+
+		memset(_mavlink_signing.secret_key, 0, MAVLINK_SECRET_KEY_LENGTH);
+		_mavlink_signing.timestamp = 0;
 		_is_signing_initialized = false;
+
+		_update_signing_state();
+		write_key_and_timestamp();
+
+		return SIGNING_DISABLED;
 	}
 
+	memcpy(_mavlink_signing.secret_key, setup_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH);
+	_mavlink_signing.timestamp = setup_signing.initial_timestamp;
+	_is_signing_initialized = true;
+
+	_update_signing_state();
 	write_key_and_timestamp();
 
-	return true;
+	return KEY_ACCEPTED;
+}
+
+void MavlinkSignControl::reload_key()
+{
+	if (_mavlink_status == nullptr) {
+		return;
+	}
+
+	_is_signing_initialized = false;
+
+	int fd = ::open(MAVLINK_SECRET_FILE, O_RDONLY);
+
+	if (fd != -1) {
+		ssize_t bytes_read = ::read(fd, _mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH);
+
+		if (bytes_read == MAVLINK_SECRET_KEY_LENGTH) {
+			bytes_read = ::read(fd, &_mavlink_signing.timestamp, MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH);
+
+			if (bytes_read == MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH) {
+				if (_mavlink_signing.timestamp != 0 || !is_array_all_zeros(_mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH)) {
+					_is_signing_initialized = true;
+				}
+			}
+		}
+
+		close(fd);
+	}
+
+	if (!_is_signing_initialized) {
+		memset(_mavlink_signing.secret_key, 0, MAVLINK_SECRET_KEY_LENGTH);
+		_mavlink_signing.timestamp = 0;
+	}
+
+	_update_signing_state();
+}
+
+void MavlinkSignControl::_update_signing_state()
+{
+	if (_is_signing_initialized) {
+		_mavlink_signing.flags = MAVLINK_SIGNING_FLAG_SIGN_OUTGOING;
+		_mavlink_status->signing = &_mavlink_signing;
+		_mavlink_status->signing_streams = &global_mavlink_signing_streams;
+
+	} else {
+		_mavlink_signing.flags = 0;
+		_mavlink_status->signing = nullptr;
+		_mavlink_status->signing_streams = nullptr;
+	}
 }
 
 void MavlinkSignControl::write_key_and_timestamp()
 {
-	int _fd = ::open(MAVLINK_SECRET_FILE, O_CREAT | O_WRONLY | O_TRUNC, PX4_O_MODE_600);
+	int fd = ::open(MAVLINK_SECRET_FILE, O_CREAT | O_WRONLY | O_TRUNC, PX4_O_MODE_600);
 
-	if (_fd == -1) {
+	if (fd == -1) {
 		if (errno != ENOENT) {
 			PX4_ERR("failed opening mavlink secret key file for writing: %s (%i)", MAVLINK_SECRET_FILE, errno);
 		}
 
 	} else {
-		ssize_t bytes_write = ::write(_fd, _mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH);
+		ssize_t bytes_write = ::write(fd, _mavlink_signing.secret_key, MAVLINK_SECRET_KEY_LENGTH);
 
 		if (bytes_write == MAVLINK_SECRET_KEY_LENGTH) {
-			bytes_write = ::write(_fd, &_mavlink_signing.timestamp, MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH);
+			bytes_write = ::write(fd, &_mavlink_signing.timestamp, MAVLINK_SECRET_KEY_TIMESTAMP_LENGTH);
 		}
 
-		close(_fd);
+		close(fd);
 	}
 }
 
-bool MavlinkSignControl::accept_unsigned(int32_t sign_mode, bool is_usb_uart, uint32_t message_id)
+bool MavlinkSignControl::accept_unsigned(uint32_t message_id)
 {
-	// if signing is not initilized properly or has all zeroes we will allow any message
 	if (!_is_signing_initialized) {
 		return true;
 	}
 
-	// Always accept a few select messages even if unsigned
 	for (unsigned i = 0; i < sizeof(unsigned_messages) / sizeof(unsigned_messages[0]); i++) {
 		if (unsigned_messages[i] == message_id) {
 			return true;
 		}
 	}
 
-	switch (sign_mode) {
-	// If signing is not required always return true
-	case MavlinkSignControl::PROTO_SIGN_OPTIONAL:
-		return true;
-
-	// Accept USB links if enabled
-	case MavlinkSignControl::PROTO_SIGN_NON_USB:
-		return is_usb_uart;
-
-	case MavlinkSignControl::PROTO_SIGN_ALWAYS:
-
-	// fallthrough
-	default:
-		return false;
-	}
+	return false;
 }
 
 bool MavlinkSignControl::is_array_all_zeros(uint8_t arr[], size_t size)

src/modules/mavlink/mavlink_sign_control.h

@@ -59,40 +59,56 @@ public:
 	MavlinkSignControl();
 	~MavlinkSignControl();
 
-	enum PROTO_SIGN {
-		PROTO_SIGN_OPTIONAL = 0,
-		PROTO_SIGN_NON_USB,
-		PROTO_SIGN_ALWAYS
+	/**
+	 * Initialize signing state and read key from file.
+	 * Only enables signing if a valid key exists on the SD card.
+	 */
+	void start(int instance_id, mavlink_status_t *mavlink_status,
+		   mavlink_accept_unsigned_t accept_unsigned_callback);
+
+	enum SetupSigningResult {
+		NOT_SETUP_SIGNING = 0,  ///< Message was not SETUP_SIGNING
+		KEY_ACCEPTED,           ///< New key provisioned successfully
+		SIGNING_DISABLED,       ///< Signing disabled via signed blank key
+		BLANK_KEY_REJECTED      ///< Blank key rejected (unsigned or signing not active)
 	};
 
 	/**
-	 * Initialize signing and read configuration from file
+	 * Checks whether the message is SETUP_SIGNING, and if yes, updates local key.
+	 * Enables or disables signing based on whether the new key is valid.
 	 */
-	void start(int _instance_id, mavlink_status_t *_mavlink_status, mavlink_accept_unsigned_t accept_unsigned_callback);
+	SetupSigningResult check_for_signing(const mavlink_message_t *msg);
 
 	/**
-	 * Checks whether the message is SETUP_SIGNING, and if yes , updates local key
+	 * Reload key from SD card file. Called on the instance's own receiver thread
+	 * when the signing key dirty flag is set by another instance.
 	 */
-	bool check_for_signing(const mavlink_message_t *msg);
+	void reload_key();
 
 	/**
-	 * stores the key and timestamp from memory to file
+	 * Stores the key and timestamp from memory to file
 	 */
 	void write_key_and_timestamp();
 
 	/**
-	 * Checks whether should accept unsigned message for specific sign mode
+	 * Checks whether an unsigned message should be accepted
 	 */
-	bool accept_unsigned(int32_t sign_mode, bool is_usb_uart, uint32_t message_id);
+	bool accept_unsigned(uint32_t message_id);
+
+	bool is_signing_active() const { return _is_signing_initialized; }
 
 	static bool is_array_all_zeros(uint8_t arr[], size_t size);
+
 private:
 	mavlink_signing_t _mavlink_signing {};
+	mavlink_status_t *_mavlink_status{nullptr};
+
+	bool _is_signing_initialized{false};
 
 	/**
-	 * Checks whether the key has been initialized
+	 * Wire or unwire the signing struct into the mavlink status based on key state.
 	 */
-	bool _is_signing_initialized;
+	void _update_signing_state();
 };