crc8_ccitt discards the MSB of every input byte on non-AVR targets

[TudorFanaru18]

modm::math::crc8_ccitt_update never examines bit 7 of its input. On non-AVR targets the shift happens before the test, and on a uint8_t that discards the bit being tested.

src/modm/math/utils/crc.hpp:

inline uint8_t
crc8_ccitt_update(uint8_t crc, uint8_t data)
{
#ifdef __AVR__
    return _crc8_ccitt_update(crc, data);
#else
    data ^= crc;
    for (uint8_t ii = 0; ii < 8; ii++)
    {
        data <<= 1;                       // <-- bit 7 is gone here
        if (data & 0x80) data ^= 0x07;    // <-- so this tests the pre-shift bit 6
    }
    return data;
#endif
}

The avr-libc assembly it replaces:

1:  lsl    %0        ; shift left, MSB -> CARRY
    brcc   2f        ; branch on CARRY, i.e. the *pre-shift* MSB
    eor    %0, %2

lsl preserves the departing bit in the carry flag, so brcc still tests bit 7 of the value before the shift. C has no carry flag, so data <<= 1 simply loses that bit and the following if (data & 0x80) reads the pre-shift bit 6 instead.

The AVR path is correct; only the #else branch is affected.

Impact

crc8_ccitt("123456789") returns 0x9B. CRC-8 with polynomial 0x07 and init 0xFF is 0xFB.

Because one input bit is ignored:

• crc8_ccitt_update(crc, d) == crc8_ccitt_update(crc, d ^ 0x80) for all 32768 (crc, d) pairs
• only 128 of 256 output values are reachable
• for an n-byte message, 2ⁿ distinct messages share a CRC

Suggested fix

Capture the MSB before shifting, which is what the carry flag does on AVR:

data ^= crc;
for (uint8_t ii = 0; ii < 8; ii++)
{
    const bool msb = (data & 0x80) != 0;
    data <<= 1;
    if (msb) data ^= 0x07;
}
return data;

Notes

• Tested against develop at 2390135.
• Checked crc16_ccitt and crc32 against their published check values (0x6F91 CRC-16/MCRF4XX and 0xCBF43926 CRC-32/ISO-HDLC); both are correct and MSB-sensitive. Only crc8_ccitt is affected.

[TudorFanaru18]

As a result, AMNB has also been shipping a weakened header check: a corrupted address or command byte passes validation today if the error is in bit 7.

amnb::Message uses this function for its header integrity check:

// src/modm/communication/amnb/message.hpp.in:153
crcHeader() const
{ return modm::math::crc8_ccitt(&header.address, headerLength()-1); }

After the fix, AMNB header CRCs will change. Nodes running different modm versions would reject each other's headers. Not sure if this warrants a changelog note or a protocol version bump.

test/modm/communication/amnb/message_test.cpp currently encodes the uncorrected values, so 5 assertions fail with the fix applied. Two of the hardcoded fixtures are:

{197, 6, 12, 0x44, ..}   stored 197   current 197   corrected  68
{197, 4, 15, 0x64, ..}   stored 197   current 197   corrected 202

[salkinium]

Interesting! What's the hardware setup that you're using AMNB on? How did you find this specific problem?

[TudorFanaru18]

I was writing a CRSF (ExpressLRS) receiver driver for an STM32H743, which uses CRC-8/DVB-S2. modm::math::crc8_ccitt hardcodes polynomial 0x07, so I wrote a small parameterized CRC-8 instead. I checked it against modm on the one parametrization they share: polynomial 0x07 init 0xFF, and they disagreed.
I only noticed the AMNB consequence after running the unittests, since they failed due to hardcoding the CRC values produced by the uncorrected function.