MAVLink debug-trap and safety-switch command guard issues

ArduCopter Copter 4.7
1

Reproduction environment

Vehicle: ArduCopter SITL
ArduPilot baseline: ArduPilot-4.7 branch snapshot / 4.7.0 beta-era build
Tested commit: 97775f82cec5bd50296c523be9baa0dacef9bb9e
Commit summary: Plane: version to 4.7.0-beta7
SITL command shape: build/sitl/bin/arducopter --wipe --speedup 8 --model quad --defaults Tools/autotest/default_params/copter.parm
MAVLink: MAVLink 2, ardupilotmega dialect

Safety/authority guard bypasses

1. MAV_CMD_DEBUG_TRAP: debug trap can terminate the autopilot process even when addressed to another component

Input tested

COMMAND_LONG command=42700 MAV_CMD_DEBUG_TRAP target_component=42 params=[32451,0,0,0,0,0,0].

Reproduction steps

  1. Start a fresh ArduCopter SITL instance.
  2. Send MAV_CMD_DEBUG_TRAP with the magic value 32451 to target_component=42.
  3. Observe command ACK behavior and process liveness.

Observed behavior

  • The client did not receive a normal command ACK.
  • The MAVLink connection was lost immediately after sending the command.
  • The client log records sitl_exit_code=-5.

Expected behavior

A debug trap should not be reachable in normal operation, and it should not affect the autopilot when addressed to a different component.

Impact

A MAVLink sender can terminate the running autopilot process in SITL by sending the magic debug-trap command.

Code evidence

  • libraries/GCS_MAVLink/GCS_Common.cpp:5119-5128: The handler checks only the magic value and then calls hal.util->trap().
5119: MAV_RESULT GCS_MAVLINK::handle_command_debug_trap(const mavlink_command_int_t &packet)
5120: {
5121:     // magic number must be supplied to trap; you must *really* mean it.
5122:     if (uint32_t(packet.param1) != 32451) {
5123:         return MAV_RESULT_DENIED;
5124:     }
5125:     if (hal.util->trap()) {
5126:         return MAV_RESULT_ACCEPTED;
5127:     }
5128:     return MAV_RESULT_UNSUPPORTED;
  • libraries/GCS_MAVLink/GCS_Common.cpp:5675-5676: MAV_CMD_DEBUG_TRAP dispatches to this handler.
5675:     case MAV_CMD_DEBUG_TRAP:
5676:         return handle_command_debug_trap(packet);

2. MAV_CMD_DO_SET_MODE: legacy safety base-mode path accepts remote safety-switch changes

Input tested

COMMAND_LONG command=176 MAV_CMD_DO_SET_MODE params=[128,0,0,0,0,0,0] and params=[128,1,0,0,0,0,0], where 128 is MAV_MODE_FLAG_DECODE_POSITION_SAFETY.

Reproduction steps

  1. Start a fresh ArduCopter SITL instance.
  2. Send MAV_CMD_DO_SET_MODE with base mode MAV_MODE_FLAG_DECODE_POSITION_SAFETY and custom mode 1 to force safety on.
  3. Send MAV_CMD_DO_MOTOR_TEST with direct PWM input as a behavior check.
  4. Send MAV_CMD_DO_SET_MODE with the same base mode and custom mode 0 to force safety off.
  5. Send the same motor-test command again.
  6. Send MAV_CMD_DO_SET_MODE with custom mode 1 again and repeat the motor-test command.

Observed behavior

  • The initial safety-on command returned MAV_RESULT_ACCEPTED, and the following motor-test command returned MAV_RESULT_FAILED.
  • After the safety-off command returned MAV_RESULT_ACCEPTED, the same motor-test command returned MAV_RESULT_ACCEPTED.
  • After forcing safety on again, the same motor-test command returned MAV_RESULT_FAILED again.
  • This confirms that the command path did not only acknowledge receipt; in SITL it changed the HAL safety state enough to alter a safety-gated behavior.

Expected behavior

Remote mode-setting should not expose hardware safety-switch operations without explicit authorization and guard checks.

Impact

A MAVLink mode command can exercise the safety-switch path remotely. In SITL, this changes the safety state used by motor-test gating. On hardware with safety-switch support, the same code path maps to force_safety_on() and force_safety_off(), which are primary motor-output safety operations.

Code evidence

  • libraries/GCS_MAVLink/GCS_Common.cpp:5092-5097: handle_command_do_set_mode() casts command parameters and calls _set_mode_common().
5092: MAV_RESULT GCS_MAVLINK::handle_command_do_set_mode(const mavlink_command_int_t &packet)
5093: {
5094:     const uint8_t _base_mode = (uint8_t)packet.param1;
5095:     const uint32_t _custom_mode = (uint32_t)packet.param2;
5096: 
5097:     return _set_mode_common(_base_mode, _custom_mode);
  • libraries/GCS_MAVLink/GCS_Common.cpp:2892-2922: _set_mode_common() accepts MAV_MODE_FLAG_DECODE_POSITION_SAFETY and calls force_safety_off() or force_safety_on().
2892: MAV_RESULT GCS_MAVLINK::_set_mode_common(const uint8_t _base_mode, const uint32_t _custom_mode)
2893: {
2894:     // only accept custom modes because there is no easy mapping from Mavlink flight modes to AC flight modes
2895: #if AP_VEHICLE_ENABLED
2896:     if ((_base_mode & MAV_MODE_FLAG_CUSTOM_MODE_ENABLED) != 0) {
2897:         if (!AP::vehicle()->set_mode(_custom_mode, ModeReason::GCS_COMMAND)) {
2898:             // often we should be returning DENIED rather than FAILED
2899:             // here.  Perhaps a "has_mode" callback on AP_::vehicle()
2900:             // would do?
2901:             return MAV_RESULT_FAILED;
2902:         }
2903:         return MAV_RESULT_ACCEPTED;
2904:     }
2905: #endif
2906: 
2907:     if (_base_mode == MAV_MODE_FLAG_DECODE_POSITION_SAFETY) {
2908:         // set the safety switch position. Must be in a command by itself
2909:         if (_custom_mode == 0) {
2910:             // turn safety off (pwm outputs flow to the motors)
2911:             hal.rcout->force_safety_off();
2912:             return MAV_RESULT_ACCEPTED;
2913:         }
2914:         if (_custom_mode == 1) {
2915:             // turn safety on (no pwm outputs to the motors)
2916:             if (hal.rcout->force_safety_on()) {
2917:                 return MAV_RESULT_ACCEPTED;
2918:             }
2919:             return MAV_RESULT_FAILED;
2920:         }
2921:         return MAV_RESULT_DENIED;
2922:     }
  • libraries/GCS_MAVLink/GCS_Common.cpp:5717-5718: MAV_CMD_DO_SET_MODE dispatches to this handler.
5717:     case MAV_CMD_DO_SET_MODE:
5718:         return handle_command_do_set_mode(packet);
  • libraries/AP_HAL_SITL/RCOutput.h:22-39: In SITL, force_safety_on() and force_safety_off() update the stored safety state.
22:     /*
23:       force the safety switch on, disabling PWM output from the IO board
24:      */
25:     bool force_safety_on(void) override {
26:         safety_state = AP_HAL::Util::SAFETY_DISARMED;
27:         return true;
28:     }
29:     /*
30:       force the safety switch off, enabling PWM output from the IO board
31:      */
32:     void force_safety_off(void) override {
33:         safety_state = AP_HAL::Util::SAFETY_ARMED;
34:     }
35: 
36:     /*
37:       get safety switch state, used by Util.cpp
38:     */
39:     AP_HAL::Util::safety_state _safety_switch_state(void) { return safety_state; }
  • libraries/AP_HAL_SITL/Util.cpp:89-97: The SITL utility safety-state query returns the RCOutput safety state.
89: enum AP_HAL::Util::safety_state HALSITL::Util::safety_switch_state(void)
90: {
91: #define HAL_USE_PWM 1
92: #if HAL_USE_PWM
93:     return ((RCOutput *)hal.rcout)->_safety_switch_state();
94: #else
95:     return SAFETY_NONE;
96: #endif
97: }
  • ArduCopter/GCS_MAVLink_Copter.cpp:734-747: MAV_CMD_DO_MOTOR_TEST dispatches to mavlink_motor_test_start().
734: MAV_RESULT GCS_MAVLINK_Copter::handle_MAV_CMD_DO_MOTOR_TEST(const mavlink_command_int_t &packet)
735: {
736:         // param1 : motor sequence number (a number from 1 to max number of motors on the vehicle)
737:         // param2 : throttle type (0=throttle percentage, 1=PWM, 2=pilot throttle channel pass-through. See MOTOR_TEST_THROTTLE_TYPE enum)
738:         // param3 : throttle (range depends upon param2)
739:         // param4 : timeout (in seconds)
740:         // param5 : num_motors (in sequence)
741:         // param6 : motor test order
742:         return copter.mavlink_motor_test_start(*this,
743:                                                (uint8_t)packet.param1,
744:                                                (uint8_t)packet.param2,
745:                                                packet.param3,
746:                                                packet.param4,
747:                                                (uint8_t)packet.x);
  • ArduCopter/motor_test.cpp:127-131: Motor test is rejected when hal.util->safety_switch_state() reports SAFETY_DISARMED.
127:     // check if safety switch has been pushed
128:     if (hal.util->safety_switch_state() == AP_HAL::Util::SAFETY_DISARMED) {
129:         gcs_chan.send_text(MAV_SEVERITY_CRITICAL,"%s: Safety switch", mode);
130:         return false;
131:     }
  • ArduCopter/motor_test.cpp:143-161: If the safety checks pass, motor test returns success and emits the normal start path.
143: // mavlink_motor_test_start - start motor test - spin a single motor at a specified pwm
144: //  returns MAV_RESULT_ACCEPTED on success, MAV_RESULT_FAILED on failure
145: MAV_RESULT Copter::mavlink_motor_test_start(const GCS_MAVLINK &gcs_chan, uint8_t motor_seq, uint8_t throttle_type, float throttle_value,
146:                                          float timeout_sec, uint8_t motor_count)
147: {
148:     if (motor_count == 0) {
149:         motor_count = 1;
150:     }
151:     // if test has not started try to start it
152:     if (!ap.motor_test) {
153:         /* perform checks that it is ok to start test
154:            The RC calibrated check can be skipped if direct pwm is
155:            supplied
156:         */
157:         if (!mavlink_motor_control_check(gcs_chan, throttle_type != 1, "Motor Test")) {
158:             return MAV_RESULT_FAILED;
159:         } else {
160:             // start test
161:             gcs().send_text(MAV_SEVERITY_INFO, "starting motor test");

Suggested fixes

  • Gate debug/trap commands behind an explicit debug build or runtime option and strict target checks.
  • Avoid exposing safety-switch operations through legacy mode fields unless an explicit authorization path is used.
  • Return failure when a safety-affecting command is unavailable, not permitted, or not addressed to the autopilot component.
2

I’m not going to waste my time on any more of this type of thing. You are welcome to make PRs to fix issues yourself, but I suggest you use a concise, well explained narrative rather than this lengthy, auto-generated format.

Continuing in this vein may result in restrictions being placed on your account, as it is not particularly helpful here.

2 Likes