Reject DataFlash FMT records whose length and format disagree - #15086
Open
nicholasaleks wants to merge 1 commit into
Open
nicholasaleks wants to merge 1 commit into
nicholasaleks wants to merge 1 commit into
Conversation
Every FMT record in a .bin log carries both a length and a format string, and the parser trusts each of them separately without ever comparing the two. length is a uint8_t read from the log. Both parser passes compute "fmt.length - 3" to get a payload size, and integer promotion makes that a negative int for length < 3. The bounds check in iterateMessages(), "pos + payloadSize > size", is meant to catch an over-long payload; a negative value makes the left side smaller, so it never fires. The cursor then moves backward by that amount. Both loops consume the 3-byte header with pos += 3 first, so length == 0 is exactly net zero movement and the same record is re-parsed forever. That hangs parseFmtMessages() during the initial format scan, before a single message is iterated: opening such a log pins a core at 100% with no error reported. Independently, parseMessage() walks the format string and accumulates an offset from formatCharSize() alone, up to 64 bytes for 'a' or 'Z'. It is not given the payload length and has nothing to check against, so a format declaring more bytes than the record holds reads past the end of it. The buffer is a memory-mapped file, so that read either lands in the zero-filled tail of the last page or faults. Both follow from the same thing: the two sizes are never reconciled. They are first known together at registration, so check them there. A format is registered only when length leaves room for a header and the fields the format string declares fit inside the remaining payload. That keeps "length - 3" non-negative everywhere it is used and bounds the offset parseMessage() accumulates, without changing its signature or its call sites. Adds a test covering length 0, 1 and 2, a format wider than the declared payload, and a consistent record that must still be accepted.
Contributor
|
See the Build Results workflow run for details. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Related to: https://github.com/nicholasaleks/infected-drones/tree/main/QGC-04_dataflash_bin_parser_oob
Every FMT record in a .bin log carries both a length and a format string, and the parser trusts each of them separately without ever comparing the two.
length is a uint8_t read from the log. Both parser passes compute "fmt.length - 3" to get a payload size, and integer promotion makes that a negative int for length < 3. The bounds check in iterateMessages(), "pos + payloadSize > size", is meant to catch an over-long payload; a negative value makes the left side smaller, so it never fires. The cursor then moves backward by that amount. Both loops consume the 3-byte header with pos += 3 first, so length == 0 is exactly net zero movement and the same record is re-parsed forever. That hangs parseFmtMessages() during the initial format scan, before a single message is iterated: opening such a log pins a core at 100% with no error reported.
Independently, parseMessage() walks the format string and accumulates an offset from formatCharSize() alone, up to 64 bytes for 'a' or 'Z'. It is not given the payload length and has nothing to check against, so a format declaring more bytes than the record holds reads past the end of it. The buffer is a memory-mapped file, so that read either lands in the zero-filled tail of the last page or faults.
Both follow from the same thing: the two sizes are never reconciled. They are first known together at registration, so check them there. A format is registered only when length leaves room for a header and the fields the format string declares fit inside the remaining payload. That keeps "length - 3" non-negative everywhere it is used and bounds the offset parseMessage() accumulates, without changing its signature or its call sites.
Adds a test covering length 0, 1 and 2, a format wider than the declared payload, and a consistent record that must still be accepted.
Type of Change
Testing
Platforms Tested
Flight Stacks Tested
Screenshots
Video in https://github.com/nicholasaleks/infected-drones/tree/main/QGC-04_dataflash_bin_parser_oob
Checklist
Related Issues
https://github.com/nicholasaleks/infected-drones/tree/main/QGC-04_dataflash_bin_parser_oob
By submitting this pull request, I confirm that my contribution is made under the terms of the project's dual license (Apache 2.0 and GPL v3).