fix CRC errors (#1944)

* fix crc mismatch

* cleanup

* use size_t, add null check

* bounds check for good measure

* rename to scratchBuffer

* comment
This commit is contained in:
Matthew Kennedy 2020-11-14 16:21:29 -08:00 committed by GitHub
parent 1a73258778
commit 11e77b7780
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 45 additions and 30 deletions

View File

@ -510,21 +510,21 @@ void runBinaryProtocolLoop(ts_channel_s *tsChannel) {
uint16_t incomingPacketSize = firstByte << 8 | secondByte; 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); scheduleMsg(&tsLogger, "TunerStudio: invalid size: %d", incomingPacketSize);
tunerStudioError("ERROR: CRC header size"); tunerStudioError("ERROR: CRC header size");
sendErrorCode(tsChannel, TS_RESPONSE_UNDERRUN); sendErrorCode(tsChannel, TS_RESPONSE_UNDERRUN);
continue; continue;
} }
received = sr5ReadData(tsChannel, (uint8_t* )tsChannel->crcReadBuffer, 1); received = sr5ReadData(tsChannel, (uint8_t* )tsChannel->scratchBuffer, 1);
if (received != 1) { if (received != 1) {
tunerStudioError("ERROR: did not receive command"); tunerStudioError("ERROR: did not receive command");
sendErrorCode(tsChannel, TS_RESPONSE_UNDERRUN); sendErrorCode(tsChannel, TS_RESPONSE_UNDERRUN);
continue; continue;
} }
char command = tsChannel->crcReadBuffer[0]; char command = tsChannel->scratchBuffer[0];
if (!isKnownCommand(command)) { if (!isKnownCommand(command)) {
scheduleMsg(&tsLogger, "unexpected command %x", command); scheduleMsg(&tsLogger, "unexpected command %x", command);
sendErrorCode(tsChannel, TS_RESPONSE_UNRECOGNIZED_COMMAND); sendErrorCode(tsChannel, TS_RESPONSE_UNRECOGNIZED_COMMAND);
@ -535,7 +535,7 @@ void runBinaryProtocolLoop(ts_channel_s *tsChannel) {
logMsg("command %c\r\n", command); logMsg("command %c\r\n", command);
#endif #endif
received = sr5ReadData(tsChannel, (uint8_t * ) (tsChannel->crcReadBuffer + 1), received = sr5ReadData(tsChannel, (uint8_t * ) (tsChannel->scratchBuffer + 1),
incomingPacketSize + CRC_VALUE_SIZE - 1); incomingPacketSize + CRC_VALUE_SIZE - 1);
int expectedSize = incomingPacketSize + CRC_VALUE_SIZE - 1; int expectedSize = incomingPacketSize + CRC_VALUE_SIZE - 1;
if (received != expectedSize) { if (received != expectedSize) {
@ -546,24 +546,24 @@ void runBinaryProtocolLoop(ts_channel_s *tsChannel) {
continue; continue;
} }
uint32_t expectedCrc = *(uint32_t*) (tsChannel->crcReadBuffer + incomingPacketSize); uint32_t expectedCrc = *(uint32_t*) (tsChannel->scratchBuffer + incomingPacketSize);
expectedCrc = SWAP_UINT32(expectedCrc); expectedCrc = SWAP_UINT32(expectedCrc);
uint32_t actualCrc = crc32(tsChannel->crcReadBuffer, incomingPacketSize); uint32_t actualCrc = crc32(tsChannel->scratchBuffer, incomingPacketSize);
if (actualCrc != expectedCrc) { if (actualCrc != expectedCrc) {
scheduleMsg(&tsLogger, "TunerStudio: CRC %x %x %x %x", tsChannel->crcReadBuffer[incomingPacketSize + 0], scheduleMsg(&tsLogger, "TunerStudio: CRC %x %x %x %x", tsChannel->scratchBuffer[incomingPacketSize + 0],
tsChannel->crcReadBuffer[incomingPacketSize + 1], tsChannel->crcReadBuffer[incomingPacketSize + 2], tsChannel->scratchBuffer[incomingPacketSize + 1], tsChannel->scratchBuffer[incomingPacketSize + 2],
tsChannel->crcReadBuffer[incomingPacketSize + 3]); 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); actualCrc, expectedCrc);
tunerStudioError("ERROR: CRC issue"); tunerStudioError("ERROR: CRC issue");
sendErrorCode(tsChannel, TS_RESPONSE_CRC_FAILURE); sendErrorCode(tsChannel, TS_RESPONSE_CRC_FAILURE);
continue; continue;
} }
int success = tunerStudioHandleCrcCommand(tsChannel, tsChannel->crcReadBuffer, incomingPacketSize); int success = tunerStudioHandleCrcCommand(tsChannel, tsChannel->scratchBuffer, incomingPacketSize);
if (!success) if (!success)
print("got unexpected TunerStudio command %x:%c\r\n", command, command); print("got unexpected TunerStudio command %x:%c\r\n", command, command);

View File

@ -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. * 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) { void sr5WriteCrcPacket(ts_channel_s *tsChannel, const uint8_t responseCode, const void *buf, size_t size) {
uint8_t *writeBuffer = tsChannel->writeBuffer;
uint8_t *crcBuffer = &tsChannel->writeBuffer[3];
#if defined(TS_CAN_DEVICE) && defined(TS_CAN_DEVICE_SHORT_PACKETS_IN_ONE_FRAME) #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, // 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. // 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 */ #endif /* TS_CAN_DEVICE */
*(uint16_t *) writeBuffer = SWAP_UINT16(size + 1); // packet size including command auto scratchBuffer = tsChannel->scratchBuffer;
*(uint8_t *) (writeBuffer + 2) = responseCode;
// CRC on whole packet // don't transmit a null buffer...
uint32_t crc = crc32((void *) (writeBuffer + 2), 1); // command part of CRC if (!buf) {
crc = crc32inc((void *) buf, crc, (uint32_t) (size)); // combined with packet CRC size = 0;
*(uint32_t *) (crcBuffer) = SWAP_UINT32(crc);
sr5WriteData(tsChannel, writeBuffer, 3); // header
if (size > 0) {
sr5WriteData(tsChannel, (const uint8_t*)buf, size); // body
} }
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<uint32_t*>(&scratchBuffer[size + 3]) = SWAP_UINT32(crc);
// Write to the underlying stream
sr5WriteData(tsChannel, reinterpret_cast<uint8_t*>(scratchBuffer), size + 7);
sr5FlushData(tsChannel); sr5FlushData(tsChannel);
} }
@ -287,6 +301,8 @@ void sr5FlushData(ts_channel_s *tsChannel) {
#if defined(TS_CAN_DEVICE) #if defined(TS_CAN_DEVICE)
UNUSED(tsChannel); UNUSED(tsChannel);
canFlushTxStream(&TS_CAN_DEVICE); canFlushTxStream(&TS_CAN_DEVICE);
#else
UNUSED(tsChannel);
#endif /* TS_CAN_DEVICE */ #endif /* TS_CAN_DEVICE */
} }

View File

@ -26,11 +26,10 @@ struct ts_channel_s {
#if ! EFI_UNIT_TEST #if ! EFI_UNIT_TEST
BaseChannel * channel = nullptr; BaseChannel * channel = nullptr;
#endif #endif
uint8_t writeBuffer[7]; // size(2 bytes) + response(1 byte) + crc32 (4 bytes)
/** /**
* See 'blockingFactor' in rusefi.ini * 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 #if TS_UART_DMA_MODE || PRIMARY_UART_DMA_MODE || TS_UART_MODE
UARTDriver *uartp = nullptr; UARTDriver *uartp = nullptr;
@ -51,7 +50,7 @@ bool stopTsPort(ts_channel_s *tsChannel);
#define SR5_READ_TIMEOUT TIME_MS2I(1000) #define SR5_READ_TIMEOUT TIME_MS2I(1000)
void sr5WriteData(ts_channel_s *tsChannel, const uint8_t * buffer, int size); 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 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); void sendOkResponse(ts_channel_s *tsChannel, ts_response_format_e mode);
int sr5ReadData(ts_channel_s *tsChannel, uint8_t * buffer, int size); int sr5ReadData(ts_channel_s *tsChannel, uint8_t * buffer, int size);