Harden four MSP request paths

[Raffi1202]

Problem

@jFriedli reported four MSP paths on maintenance-10.x that mishandle unexpected input: a zero-length MSP_SET_PASSTHROUGH enters ESC 4-way mode and disables the motor outputs even while armed (#11777); an MSP_DATAFLASH_READ with a start address beyond the FlashFS volume underflows the length and reaches the flash driver with an oversized read (#11778); a 2-byte MSPv1 start frame over telemetry is accepted although the parser reads a 3-byte header (#11779); and a short final MSP-over-SmartPort response frame carries uninitialised stack bytes (#11780). All four were shown with host-side ASan/MSan harnesses, not on hardware. Fixes #11777, fixes #11778, fixes #11779, fixes #11780.

Cause

On maintenance-10.x:

• src/main/fc/fc_msp.c:256-260 calls esc4wayInit() with no ARMING_FLAG(ARMED) check; esc4wayInit() starts with pwmDisableMotors() (src/main/io/serial_4way.c:138). The dispatcher at fc_msp.c:5159-5161 always returns ACK.
• src/main/fc/fc_msp.c:380-383 computes flashfsSize - address before checking address < flashfsSize; the unsigned subtraction wraps.
• src/main/telemetry/msp_shared.c:127-149 enforces only MIN_LENGTH_CHUNK (2), then reads frameStart[2] and initialises the request sbuf at frameStart + 3. MIN_LENGTH_REQUEST_V1 (line 39) is never applied.
• src/main/telemetry/smartport.c:419-421 leaves payload uninitialised and copies only dataSize bytes; smartPortWriteFrameSerial() (line 336) sends all sizeof(smartPortPayload_t) bytes.

Change

mspFcSetPassthroughCommand() now returns mspResult_e; the 4-way branch returns MSP_RESULT_ERROR while armed and the dispatcher passes that result through. serializeDataflashReadReply() sets readLen = 0 when address >= flashfsSize and skips flashfsReadAbs() for an empty read. handleMspFrame() requires MIN_LENGTH_REQUEST_V1 for an MSPv1 start frame and clears mspStarted on every start frame and on the short-chunk and version-mismatch exits, so a rejected start frame cannot leave an earlier partial request live. smartPortSendMspResponse() zeroes the payload before filling it. flashfsReadAbs() keeps its own subtraction and is not touched.

Test

Not run on hardware or SITL. Cause verified by reading the locations above; each of the reporter's harnesses maps to one of them. No firmware build has run: the upstream "Build firmware" run is blocked awaiting approval with no jobs executed (https://github.com/iNavFlight/inav/actions/runs/34620914526) and the fork has no run for this branch. No test in src/test/unit compiles fc_msp.c, msp_shared.c or smartport.c. Qodo's review flagged one issue, stale request state after a rejected short start frame (#11899 (comment)), fixed in ef4bffa. Compiled for all targets and the four SITL builds on the fork, green: https://github.com/Raffi1202/inav/actions/runs/34770665897

Flash / RAM

Builds clean on all targets. No size comparison yet: the fork build has no baseline for this branch, and the upstream size report runs once CI is released for this PR.

Docs

docs/development/msp/msp_messages.json: MSP_SET_PASSTHROUGH now documents that ESC 4-way is refused while armed, the way MSP_REBOOT and MSP_ACC_CALIBRATION already do. README.md regenerated with gen_msp_md.py.

[qodo-code-review]

ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Harden four MSP request and telemetry paths

🐞 Bug fix 🕐 20-40 Minutes

AI Description

• Rejects armed ESC passthrough and safely bounds dataflash reads.
• Validates complete MSPv1 headers before parsing telemetry requests.
• Zero-fills SmartPort MSP fragments to prevent stack-byte disclosure.

Diagram

graph TD
    A["MSP Client"] --> B["Telemetry Parser"] --> C["Command Router"] --> H["SmartPort Serializer"]
    C --> D["Arming Guard"] --> E["ESC 4-Way"]
    C --> F["Read Bounds"] --> G["FlashFS"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Centralize FlashFS bounds checking

• ➕ Protects every caller of flashfsReadAbs from address-subtraction underflow.
• ➕ Keeps the storage safety invariant inside the low-level API.
• ➖ Broadens the behavioral scope beyond the reported MSP path.
• ➖ Requires auditing other callers and defining consistent out-of-range semantics.

2. Guard ESC initialization centrally

• ➕ Prevents any caller from disabling motor outputs while armed.
• ➕ Makes the safety invariant independent of the MSP command path.
• ➖ Couples the low-level ESC interface to flight-controller arming state.
• ➖ May affect legitimate internal callers or build configurations.

Recommendation: The localized request-path guards are appropriate for a narrowly scoped maintenance fix and preserve valid-request behavior. Follow up by hardening flashfsReadAbs itself after auditing callers; consider centralizing the armed-state invariant only if esc4wayInit is reachable through additional paths.

Files changed (3) +22 / -7

Bug fix (3) +22 / -7

fc_msp.cReject unsafe passthrough and out-of-range flash reads +18/-7

Reject unsafe passthrough and out-of-range flash reads

• Changes the passthrough helper to return an MSP result and rejects ESC 4-way initialization while armed. Bounds dataflash addresses before subtraction, skips empty reads, and propagates passthrough errors to the command dispatcher.

src/main/fc/fc_msp.c

msp_shared.cRequire complete MSPv1 start-frame headers +3/-0

Require complete MSPv1 start-frame headers

• Rejects MSPv1 start frames shorter than the three-byte minimum before reading the command byte or deriving the payload position.

src/main/telemetry/msp_shared.c

smartport.cZero-initialize SmartPort MSP response payloads +1/-0

Zero-initialize SmartPort MSP response payloads

• Clears the packed SmartPort payload before copying response data so short final fragments cannot serialize uninitialized stack bytes.

src/main/telemetry/smartport.c

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Short frames can execute stale commands ✓ Resolved 🐞 Bug ≡ Correctness

Description

The new MSPv1 length guard in handleMspFrame() returns without clearing mspStarted, so an
incomplete request and its sequence state remain active. On CRSF, a two-byte start frame received
between chunks lets the next matching continuation finish and execute the earlier command instead of
discarding it.

Code

src/main/telemetry/msp_shared.c[R146-148]

+            if (payloadLength < MIN_LENGTH_REQUEST_V1) {
+                return false;   // prevent analyzing garbage data
+            }

Evidence

mspStarted and lastSeq persist across calls, while the new guard returns before replacing or
clearing the current request. The continuation path subsequently accepts the expected sequence,
appends bytes to that retained request, and calls processMspPacket() once it is complete; CRSF
supplies variable-length chunks and permits the two-byte payload needed to reach this guard.

src/main/telemetry/msp_shared.c[116-152]
src/main/telemetry/msp_shared.c[174-206]
src/main/rx/crsf.c[179-188]
src/main/telemetry/crsf.c[101-143]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Rejecting a short MSPv1 start frame leaves the previous partial request active, allowing later continuation chunks to complete and execute that stale command.
## Fix Focus Areas
- src/main/telemetry/msp_shared.c[116-206]
## Recommended Fix
Set `mspStarted` to zero before returning for a short MSPv1 start frame, ensuring the next frame cannot continue the previous request. Apply the same reset consistently to malformed start-frame rejection paths.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[github-actions]

RAM / Flash usage vs. base commit 76ee415 — commit 53fa1ae

Target	Flash Δ	RAM Δ
MATEKF405 ⚠️	+23124 B (+3.31%)	-10396 B (-6.96%)
MATEKF722 ⚠️	+10396 B (+2.21%)	-11256 B (-8.98%)
MATEKF765 ⚠️	+16252 B (+2.20%)	-9260 B (-5.60%)
MATEKH743 ⚠️	+22776 B (+2.94%)	-8412 B (-4.98%)

See RAM/flash optimization guide for techniques to reduce usage.

[github-actions]

Test firmware build ready — commit 53fa1ae

Download firmware for PR #11899

250 targets built. Find your board's .hex file by name on that page (e.g. MATEKF405SE.hex). Files are individually downloadable — no GitHub login required.

Development build for testing only. Use Full Chip Erase when flashing.