Last active
July 7, 2026 05:36
-
-
Save quart27219/6bfcc615f89fb493d02aad480704593b to your computer and use it in GitHub Desktop.
Unchecked MAVLink SERIAL_CONTROL count causes out-of-bounds read in ArduPilot
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
| ### Summary | |
| `GCS_MAVLINK::handle_serial_control()` uses the wire-controlled | |
| `SERIAL_CONTROL.count` field as the write length for the fixed-size decoded | |
| `SERIAL_CONTROL.data[70]` buffer without first checking that | |
| `count <= sizeof(data)`. | |
| A decoder-accepted MAVLink `SERIAL_CONTROL` message can therefore cause: | |
| ```cpp | |
| stream->write(packet.data, packet.count); | |
| ``` | |
| to read past the end of the decoded `packet.data` array. | |
| ### Root Cause | |
| mavlink_serial_control_t contains a fixed-width data buffer: | |
| `packet.data` | |
| with a maximum serialized data length of 70 bytes. | |
| However, packet.count is a separate field controlled by the received MAVLink | |
| message. After decoding, packet.count can be larger than | |
| sizeof(packet.data). | |
| The handler currently trusts packet.count and passes it directly as the number of bytes to write: | |
| `stream->write(packet.data, packet.count);` | |
| If `packet.count > sizeof(packet.data)`, the stream layer is asked to read | |
| past the end of the decoded packet.data buffer. | |
| ### Related Logs | |
| The following Valgrind output is supporting runtime evidence only. It shows | |
| that bytes originating from stack storage in the MAVLink receive path can reach | |
| the SITL UART outbound send path. The root cause is the missing post-decode | |
| bounds check on `packet.count`. | |
| ``` | |
| ==3486== Conditional jump or move depends on uninitialised value(s) | |
| ==3486== at 0x3F34C1: mavlink_packetise(ByteBuffer&, unsigned short) | |
| ==3486== by 0x35DE51: HALSITL::UARTDriver::_timer_tick() | |
| ==3486== Uninitialised value was created by a stack allocation | |
| ==3486== at 0x277871: GCS_MAVLINK::raw_packetReceived( | |
| ==3486== unsigned char, | |
| ==3486== __mavlink_status const&, | |
| ==3486== __mavlink_message const& | |
| ==3486== ) | |
| ==3486== Syscall param socketcall.sendto(msg) points to uninitialised byte(s) | |
| ==3486== at 0x4D1CF2A: __libc_send (send.c:28) | |
| ==3486== by 0x4D1CF2A: send (send.c:23) | |
| ==3486== by 0x35DEC9: HALSITL::UARTDriver::_timer_tick() | |
| ==3486== Address 0x1fff0005e6 is on thread 1's stack | |
| ==3486== in frame #1, created by HALSITL::UARTDriver::_timer_tick() | |
| ==3486== Uninitialised value was created by a stack allocation | |
| ==3486== at 0x277871: GCS_MAVLINK::raw_packetReceived(...) | |
| ``` | |
| ### Observed behavior | |
| When the message is handled, handle_serial_control() reaches: | |
| `stream->write(packet.data, packet.count);` | |
| with packet.count == 255 while the decoded `packet.data` field is a fixed 70-byte array. | |
| Runtime memory checking can report that bytes originating from stack storage | |
| are later used in the outbound send path. This is consistent with | |
| `packet.count` causing the stream layer to read beyond the fixed | |
| `packet.data` array. | |
| ### Expected behavior | |
| handle_serial_control() should never use packet.count as a buffer length | |
| unless it has first been bounded against sizeof(packet.data). | |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment