AP_Mission: protect against FPE in mavlink_int_to_mission_cmd#32009
AP_Mission: protect against FPE in mavlink_int_to_mission_cmd#32009peterbarker merged 1 commit intoArduPilot:masterfrom
Conversation
|
I wonder if it would be better to return |
|
So you want to hide a bug in your GCS software by patching ArduPilot? |
|
@peterbarker Regardless of whether the bad data comes from a buggy GCS, a script, or a malicious source, the firmware should never hit a SIGFPE and crash. Rejecting the invalid value with MAV_MISSION_INVALID_PARAM1 prevents the denial-of-service condition while correctly informing the sender that the command was rejected. |
|
The firmware never will hit a SIGFPE or crash, that feature is enabled in SITL specifically to help you find stuff like this. I don't think we should accept this either. |
Belt and braces. It's very bad software development practice for any software to be dependent on the correctness of another piece of software, especially when there is a published API that anyone and their dog could write buggy software against. I like @tpwrules suggestion:
|
|
I've confirmed this doesn't crash a real vehicle, it implicitly constrains just fine already without this change. It definitely would be nice in an ideal world to reject such a command, but we simply do not have the flash space to do it for every permutation of problem commands. So we let SITL diagnose the problem and ensure a real vehicle still does something sensible. |
|
As long as
Don't forget BTW - that if it didn't someone could deliberately send a malformed message to crash a drone. We can't rely on the GCS. This is going to become more of a thing. |
|
There are dozens and probably hundreds of well-formed messages that can crash a drone. You can even nicely ask ArduPilot to cause a hardware exception (or force a reboot) using mavproxy. Or set the desired altitude to zero. Or change your servo configs to disable the motors. Deliberately malformed messages are not interesting. For the record, to mitigate such problems, we have mavlink signing, and many radio systems have proper encryption too. |
|
I've created ArduPilot/ardupilot_wiki#7359 to try to lay out our assumption in ArduPilot about attack surfaces. |
|
I think focusing on the exception is the wrong take. The key thing for the user is that if the value is out of range the vehicle will not behave as expected. They will not find out about that until they run the mission.18 and a half hours us not a completly out of the relms of a delay you might want on rover. |
Signed-off-by: luckys00 <mathlover338@gmail.com> AP_Mission: reject invalid param1 instead of constraining
79a7b6f to
bbb6a14
Compare
|
@IamPete1 forcibly made the point that the user could reasonably ask the vehicle to delay for more than 65535, so we really should be a little nicer here. Marked for merge, thanks! |
|
Thanks @peterbarker for the review and the final polish! Also, big thanks to @IamPete1 and @timtuxworth for the valuable insights and support on this. |
|
Hi @peterbarker and team, I've been following the recent work on AP_Mission and this FPE fix. I'm very interested in contributing further to ArduPilot, specifically targeting GSoC 2026. Given the complexity of mission commands, are there any related issues or 'good first issues' you'd recommend I look into to better prepare my proposal?" |
#31902
This PR fixes a SIGFPE (Floating Point Exception) crash that occurs when uploading a mission file with an invalid large value in param1.
The Fix: Constrains the packet.param1 float value to UINT16_MAX before casting it to uint16_t. This prevents the overflow that was causing the arithmetic exception.
Verification: Tested in SITL using the reproduction steps from the issue (uploading a mission with 1e+10 in param1). Verified that the crash no longer occurs.