inav icon indicating copy to clipboard operation
inav copied to clipboard

data racing in crsf.c

Open RomanLut opened this issue 2 years ago • 0 comments

in https://github.com/iNavFlight/inav/blob/master/src/main/rx/crsf.c

method STATIC_UNIT_TESTED void crsfDataReceive(uint16_t c, void *rxCallbackData) is called from ISR and fills crsfFrame.bytes constantly.

method STATIC_UNIT_TESTED uint8_t crsfFrameStatus(rxRuntimeConfig_t *rxRuntimeConfig) is called periodically from main cycle and reads crsfFrame:

if (crsfFrameDone) {
        crsfFrameDone = false;
        if (crsfFrame.frame.type == CRSF_FRAMETYPE_RC_CHANNELS_PACKED) {
            // CRC includes type and payload of each frame
            const uint8_t crc = crsfFrameCRC();
            if (crc != crsfFrame.frame.payload[CRSF_FRAME_RC_CHANNELS_PAYLOAD_SIZE]) {
                return RX_FRAME_PENDING;
            }
            crsfFrame.frame.frameLength = CRSF_FRAME_RC_CHANNELS_PAYLOAD_SIZE + CRSF_FRAME_LENGTH_TYPE_CRC;   ///- why??? 

            // unpack the RC channels
            const crsfPayloadRcChannelsPacked_t* rcChannels = (crsfPayloadRcChannelsPacked_t*)&crsfFrame.frame.payload;
            crsfChannelData[0] = rcChannels->chan0;
            ...

As first method is called from ISR, it may be executed in the middle of crsfFrameStatus after it checked for crc and is extracting channel values. I can overwite channel values with another packet type (link statistic or CRSF over MSP).

This can lead to any kind of weird behaviour f.e disarm midair.

A quick look at sbus.c reveals the same problem, though it is less sensitive to ovewriting.

Suggested solution Make a copy of a frame after decoding. Second mehod should work on copy and guard it properly with crsfFrameDone variable.

RomanLut avatar Oct 10 '23 11:10 RomanLut