From 11e77b77804c47094d540e33092a66d171278b93 Mon Sep 17 00:00:00 2001 From: Matthew Kennedy Date: Sat, 14 Nov 2020 16:21:29 -0800 Subject: [PATCH] fix CRC errors (#1944) * fix crc mismatch * cleanup * use size_t, add null check * bounds check for good measure * rename to scratchBuffer * comment --- firmware/console/binary/tunerstudio.cpp | 22 +++++----- firmware/console/binary/tunerstudio_io.cpp | 48 ++++++++++++++-------- firmware/console/binary/tunerstudio_io.h | 5 +-- 3 files changed, 45 insertions(+), 30 deletions(-) diff --git a/firmware/console/binary/tunerstudio.cpp b/firmware/console/binary/tunerstudio.cpp index 2bd7c7cd84..eb56fe97fa 100644 --- a/firmware/console/binary/tunerstudio.cpp +++ b/firmware/console/binary/tunerstudio.cpp @@ -510,21 +510,21 @@ void runBinaryProtocolLoop(ts_channel_s *tsChannel) { uint16_t incomingPacketSize = firstByte << 8 | secondByte; - if (incomingPacketSize == 0 || incomingPacketSize > (sizeof(tsChannel->crcReadBuffer) - CRC_WRAPPING_SIZE)) { + if (incomingPacketSize == 0 || incomingPacketSize > (sizeof(tsChannel->scratchBuffer) - CRC_WRAPPING_SIZE)) { scheduleMsg(&tsLogger, "TunerStudio: invalid size: %d", incomingPacketSize); tunerStudioError("ERROR: CRC header size"); sendErrorCode(tsChannel, TS_RESPONSE_UNDERRUN); continue; } - received = sr5ReadData(tsChannel, (uint8_t* )tsChannel->crcReadBuffer, 1); + received = sr5ReadData(tsChannel, (uint8_t* )tsChannel->scratchBuffer, 1); if (received != 1) { tunerStudioError("ERROR: did not receive command"); sendErrorCode(tsChannel, TS_RESPONSE_UNDERRUN); continue; } - char command = tsChannel->crcReadBuffer[0]; + char command = tsChannel->scratchBuffer[0]; if (!isKnownCommand(command)) { scheduleMsg(&tsLogger, "unexpected command %x", command); sendErrorCode(tsChannel, TS_RESPONSE_UNRECOGNIZED_COMMAND); @@ -535,7 +535,7 @@ void runBinaryProtocolLoop(ts_channel_s *tsChannel) { logMsg("command %c\r\n", command); #endif - received = sr5ReadData(tsChannel, (uint8_t * ) (tsChannel->crcReadBuffer + 1), + received = sr5ReadData(tsChannel, (uint8_t * ) (tsChannel->scratchBuffer + 1), incomingPacketSize + CRC_VALUE_SIZE - 1); int expectedSize = incomingPacketSize + CRC_VALUE_SIZE - 1; if (received != expectedSize) { @@ -546,24 +546,24 @@ void runBinaryProtocolLoop(ts_channel_s *tsChannel) { continue; } - uint32_t expectedCrc = *(uint32_t*) (tsChannel->crcReadBuffer + incomingPacketSize); + uint32_t expectedCrc = *(uint32_t*) (tsChannel->scratchBuffer + incomingPacketSize); expectedCrc = SWAP_UINT32(expectedCrc); - uint32_t actualCrc = crc32(tsChannel->crcReadBuffer, incomingPacketSize); + uint32_t actualCrc = crc32(tsChannel->scratchBuffer, incomingPacketSize); if (actualCrc != expectedCrc) { - scheduleMsg(&tsLogger, "TunerStudio: CRC %x %x %x %x", tsChannel->crcReadBuffer[incomingPacketSize + 0], - tsChannel->crcReadBuffer[incomingPacketSize + 1], tsChannel->crcReadBuffer[incomingPacketSize + 2], - tsChannel->crcReadBuffer[incomingPacketSize + 3]); + scheduleMsg(&tsLogger, "TunerStudio: CRC %x %x %x %x", tsChannel->scratchBuffer[incomingPacketSize + 0], + tsChannel->scratchBuffer[incomingPacketSize + 1], tsChannel->scratchBuffer[incomingPacketSize + 2], + tsChannel->scratchBuffer[incomingPacketSize + 3]); - scheduleMsg(&tsLogger, "TunerStudio: command %c actual CRC %x/expected %x", tsChannel->crcReadBuffer[0], + scheduleMsg(&tsLogger, "TunerStudio: command %c actual CRC %x/expected %x", tsChannel->scratchBuffer[0], actualCrc, expectedCrc); tunerStudioError("ERROR: CRC issue"); sendErrorCode(tsChannel, TS_RESPONSE_CRC_FAILURE); continue; } - int success = tunerStudioHandleCrcCommand(tsChannel, tsChannel->crcReadBuffer, incomingPacketSize); + int success = tunerStudioHandleCrcCommand(tsChannel, tsChannel->scratchBuffer, incomingPacketSize); if (!success) print("got unexpected TunerStudio command %x:%c\r\n", command, command); diff --git a/firmware/console/binary/tunerstudio_io.cpp b/firmware/console/binary/tunerstudio_io.cpp index aba0e5b8ed..c79eef5587 100644 --- a/firmware/console/binary/tunerstudio_io.cpp +++ b/firmware/console/binary/tunerstudio_io.cpp @@ -228,10 +228,7 @@ int sr5ReadData(ts_channel_s *tsChannel, uint8_t * buffer, int size) { /** * Adds size to the beginning of a packet and a crc32 at the end. Then send the packet. */ -void sr5WriteCrcPacket(ts_channel_s *tsChannel, const uint8_t responseCode, const void *buf, const uint16_t size) { - uint8_t *writeBuffer = tsChannel->writeBuffer; - uint8_t *crcBuffer = &tsChannel->writeBuffer[3]; - +void sr5WriteCrcPacket(ts_channel_s *tsChannel, const uint8_t responseCode, const void *buf, size_t size) { #if defined(TS_CAN_DEVICE) && defined(TS_CAN_DEVICE_SHORT_PACKETS_IN_ONE_FRAME) // a special case for short packets: we can sent them in 1 frame, without CRC & size, // because the CAN protocol is already protected by its own checksum. @@ -245,20 +242,37 @@ void sr5WriteCrcPacket(ts_channel_s *tsChannel, const uint8_t responseCode, cons } #endif /* TS_CAN_DEVICE */ - *(uint16_t *) writeBuffer = SWAP_UINT16(size + 1); // packet size including command - *(uint8_t *) (writeBuffer + 2) = responseCode; + auto scratchBuffer = tsChannel->scratchBuffer; - // CRC on whole packet - uint32_t crc = crc32((void *) (writeBuffer + 2), 1); // command part of CRC - crc = crc32inc((void *) buf, crc, (uint32_t) (size)); // combined with packet CRC - - *(uint32_t *) (crcBuffer) = SWAP_UINT32(crc); - - sr5WriteData(tsChannel, writeBuffer, 3); // header - if (size > 0) { - sr5WriteData(tsChannel, (const uint8_t*)buf, size); // body + // don't transmit a null buffer... + if (!buf) { + size = 0; } - sr5WriteData(tsChannel, crcBuffer, 4); // CRC footer + + // don't transmit too large a buffer + efiAssertVoid(OBD_PCM_Processor_Fault, size <= BLOCKING_FACTOR + 7, "sr5WriteCrcPacket tried to transmit too large a packet") + + // If transmitting data, copy it in to place in the scratch buffer + // We want to prevent the data changing itself (higher priority threads could write + // tsOutputChannels) during the CRC computation. Instead compute the CRC on our + // local buffer that nobody else will write. + if (size) { + memcpy(scratchBuffer + 3, buf, size); + } + + // Index 0/1 = packet size (big endian) + *(uint16_t*)scratchBuffer = SWAP_UINT16(size + 1); + // Index 2 = response code + scratchBuffer[2] = responseCode; + + // CRC is computed on the responseCode and payload but not length + uint32_t crc = crc32(&scratchBuffer[2], size + 1); // command part of CRC + + // Place the CRC at the end + *reinterpret_cast(&scratchBuffer[size + 3]) = SWAP_UINT32(crc); + + // Write to the underlying stream + sr5WriteData(tsChannel, reinterpret_cast(scratchBuffer), size + 7); sr5FlushData(tsChannel); } @@ -287,6 +301,8 @@ void sr5FlushData(ts_channel_s *tsChannel) { #if defined(TS_CAN_DEVICE) UNUSED(tsChannel); canFlushTxStream(&TS_CAN_DEVICE); +#else + UNUSED(tsChannel); #endif /* TS_CAN_DEVICE */ } diff --git a/firmware/console/binary/tunerstudio_io.h b/firmware/console/binary/tunerstudio_io.h index 63f8d0f74f..5d99086bec 100644 --- a/firmware/console/binary/tunerstudio_io.h +++ b/firmware/console/binary/tunerstudio_io.h @@ -26,11 +26,10 @@ struct ts_channel_s { #if ! EFI_UNIT_TEST BaseChannel * channel = nullptr; #endif - uint8_t writeBuffer[7]; // size(2 bytes) + response(1 byte) + crc32 (4 bytes) /** * See 'blockingFactor' in rusefi.ini */ - char crcReadBuffer[BLOCKING_FACTOR + 30]; + char scratchBuffer[BLOCKING_FACTOR + 30]; #if TS_UART_DMA_MODE || PRIMARY_UART_DMA_MODE || TS_UART_MODE UARTDriver *uartp = nullptr; @@ -51,7 +50,7 @@ bool stopTsPort(ts_channel_s *tsChannel); #define SR5_READ_TIMEOUT TIME_MS2I(1000) void sr5WriteData(ts_channel_s *tsChannel, const uint8_t * buffer, int size); -void sr5WriteCrcPacket(ts_channel_s *tsChannel, const uint8_t responseCode, const void *buf, const uint16_t size); +void sr5WriteCrcPacket(ts_channel_s *tsChannel, const uint8_t responseCode, const void *buf, size_t size); void sr5SendResponse(ts_channel_s *tsChannel, ts_response_format_e mode, const uint8_t * buffer, int size); void sendOkResponse(ts_channel_s *tsChannel, ts_response_format_e mode); int sr5ReadData(ts_channel_s *tsChannel, uint8_t * buffer, int size);