Fix race condition in px4io serial driver (#20005)

* px4io: prevent memory corruption on corrput io data

* px4io_serial: Prevent race between handling wait timeout case and interrupt posting semaphore

platforms/nuttx/src/px4/stm/stm32f4/px4io_serial/px4io_serial.cpp

@@ -225,9 +225,6 @@ ArchPX4IOSerial::ioctl(unsigned operation, unsigned &arg)
 int
 ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 {
-	// to be paranoid ensure all previous DMA transfers are cleared
-	_abort_dma();
-
 	_current_packet = _packet;
 
 	/* clear any lingering error status */
@@ -278,8 +275,6 @@ ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 		DMA_SCR_PBURST_SINGLE	|
 		DMA_SCR_MBURST_SINGLE);
 	stm32_dmastart(_tx_dma, nullptr, nullptr, false);
-	//rCR1 &= ~USART_CR1_TE;
-	//rCR1 |= USART_CR1_TE;
 	rCR3 |= USART_CR3_DMAT;
 
 	/* compute the deadline for a 10ms timeout */
@@ -294,6 +289,7 @@ ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 
 	/* wait for the transaction to complete - 64 bytes @ 1.5Mbps ~426µs */
 	int ret;
+	irqstate_t irqs = enter_critical_section();
 
 	for (;;) {
 		ret = sem_timedwait(&_completion_semaphore, &abstime);
@@ -301,42 +297,53 @@ ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 		if (ret == OK) {
 			/* check for DMA errors */
 			if (_rx_dma_status & DMA_STATUS_TEIF) {
-				// stream transfer error, ensure all DMA is also stopped before exiting early
+				/* One of 3 things has happened:
-				_abort_dma();
+				 *	1. a DMA Stream error
+				 *	2. Serial parity, framing or over run error
+				 *	3. packet is malformed
+				 * In all cases DMA is stopped by either HW or the ISR error service path.
+				 */
 				perf_count(_pc_dmaerrs);
 				ret = -EIO;
 				break;
-			}
 
-			/* check packet CRC - corrupt packet errors mean IO receive CRC error */
+			} else {
-			uint8_t crc = _current_packet->crc;
+				/* successful DMA completion but the crc can still fail */
-			_current_packet->crc = 0;
-
-			if ((crc != crc_packet(_current_packet)) || (PKT_CODE(*_current_packet) == PKT_CODE_CORRUPT)) {
-				_abort_dma();
-				perf_count(_pc_crcerrs);
-				ret = -EIO;
 				break;
 			}
 
-			/* successful txn (may still be reporting an error) */
+		} else {
-			break;
+			if (errno == ETIMEDOUT) {
+				/* something has broken - clear out any partial DMA state and reconfigure */
+				_abort_dma();
+				_rx_dma_status = _dma_status_inactive;
+				/* Wait fot at least a character time to make sure that there is no lingering
+				 * IDLE interrupt triggering right after we re-enable interrupts for the next
+				 * exchange
+				 */
+				usleep(100);
+				perf_count(_pc_timeouts);
+				perf_cancel(_pc_txns);		/* don't count this as a transaction */
+				break;
+			}
 		}
 
-		if (errno == ETIMEDOUT) {
+		/* Loop in case we are interrupted on sleep */
-			/* something has broken - clear out any partial DMA state and reconfigure */
-			_abort_dma();
-			perf_count(_pc_timeouts);
-			perf_cancel(_pc_txns);		/* don't count this as a transaction */
-			break;
-		}
-
-		/* we might? see this for EINTR */
-		syslog(LOG_ERR, "unexpected ret %d/%d\n", ret, errno);
 	}
 
-	/* reset DMA status */
 	_rx_dma_status = _dma_status_inactive;
+	leave_critical_section(irqs);
+
+	if (ret == OK) {
+		/* check packet CRC - corrupt packet errors mean IO receive CRC error */
+		uint8_t crc = _current_packet->crc;
+		_current_packet->crc = 0;
+
+		if ((crc != crc_packet(_current_packet)) || (PKT_CODE(*_current_packet) == PKT_CODE_CORRUPT)) {
+			perf_count(_pc_crcerrs);
+			ret = -EIO;
+		}
+	}
 
 	/* update counters */
 	perf_end(_pc_txns);
@@ -373,6 +380,8 @@ ArchPX4IOSerial::_do_rx_dma_callback(unsigned status)
 
 		/* disable UART DMA */
 		rCR3 &= ~(USART_CR3_DMAT | USART_CR3_DMAR);
+		stm32_dmastop(_tx_dma);
+		stm32_dmastop(_rx_dma);
 
 		/* complete now */
 		px4_sem_post(&_completion_semaphore);
@@ -435,7 +444,7 @@ ArchPX4IOSerial::_do_interrupt()
 				/* stop the receive DMA */
 				stm32_dmastop(_rx_dma);
 
-				/* complete the short reception */
+				/* error flag completion of short reception */
 				_do_rx_dma_callback(DMA_STATUS_TEIF);
 				return;
 			}

platforms/nuttx/src/px4/stm/stm32f7/px4io_serial/px4io_serial.cpp

@@ -237,9 +237,6 @@ ArchPX4IOSerial::ioctl(unsigned operation, unsigned &arg)
 int
 ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 {
-	// to be paranoid ensure all previous DMA transfers are cleared
-	_abort_dma();
-
 	_current_packet = _packet;
 
 	/* clear data that may be in the RDR and clear overrun error: */
@@ -298,8 +295,6 @@ ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 		DMA_SCR_MBURST_SINGLE);
 	rCR3 |= USART_CR3_DMAT;
 	stm32_dmastart(_tx_dma, nullptr, nullptr, false);
-	//rCR1 &= ~USART_CR1_TE;
-	//rCR1 |= USART_CR1_TE;
 
 	/* compute the deadline for a 10ms timeout */
 	struct timespec abstime;
@@ -313,6 +308,7 @@ ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 
 	/* wait for the transaction to complete - 64 bytes @ 1.5Mbps ~426µs */
 	int ret;
+	irqstate_t irqs = enter_critical_section();
 
 	for (;;) {
 		ret = sem_timedwait(&_completion_semaphore, &abstime);
@@ -320,42 +316,53 @@ ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 		if (ret == OK) {
 			/* check for DMA errors */
 			if (_rx_dma_status & DMA_STATUS_TEIF) {
-				// stream transfer error, ensure all DMA is also stopped before exiting early
+				/* One of 3 things has happened:
-				_abort_dma();
+				 *	1. a DMA Stream error
+				 *	2. Serial parity, framing or over run error
+				 *	3. packet is malformed
+				 * In all cases DMA is stopped by either HW or the ISR error service path.
+				 */
 				perf_count(_pc_dmaerrs);
 				ret = -EIO;
 				break;
-			}
 
-			/* check packet CRC - corrupt packet errors mean IO receive CRC error */
+			} else {
-			uint8_t crc = _current_packet->crc;
+				/* successful DMA completion but the crc can still fail */
-			_current_packet->crc = 0;
-
-			if ((crc != crc_packet(_current_packet)) || (PKT_CODE(*_current_packet) == PKT_CODE_CORRUPT)) {
-				_abort_dma();
-				perf_count(_pc_crcerrs);
-				ret = -EIO;
 				break;
 			}
 
-			/* successful txn (may still be reporting an error) */
+		} else {
-			break;
+			if (errno == ETIMEDOUT) {
+				/* something has broken - clear out any partial DMA state and reconfigure */
+				_abort_dma();
+				_rx_dma_status = _dma_status_inactive;
+				/* Wait fot at least a character time to make sure that there is no lingering
+				 * IDLE interrupt triggering right after we re-enable interrupts for the next
+				 * exchange
+				 */
+				usleep(100);
+				perf_count(_pc_timeouts);
+				perf_cancel(_pc_txns);		/* don't count this as a transaction */
+				break;
+			}
 		}
 
-		if (errno == ETIMEDOUT) {
+		/* Loop in case we are interrupted on sleep */
-			/* something has broken - clear out any partial DMA state and reconfigure */
-			_abort_dma();
-			perf_count(_pc_timeouts);
-			perf_cancel(_pc_txns);		/* don't count this as a transaction */
-			break;
-		}
-
-		/* we might? see this for EINTR */
-		syslog(LOG_ERR, "unexpected ret %d/%d\n", ret, errno);
 	}
 
-	/* reset DMA status */
 	_rx_dma_status = _dma_status_inactive;
+	leave_critical_section(irqs);
+
+	if (ret == OK) {
+		/* check packet CRC - corrupt packet errors mean IO receive CRC error */
+		uint8_t crc = _current_packet->crc;
+		_current_packet->crc = 0;
+
+		if ((crc != crc_packet(_current_packet)) || (PKT_CODE(*_current_packet) == PKT_CODE_CORRUPT)) {
+			perf_count(_pc_crcerrs);
+			ret = -EIO;
+		}
+	}
 
 	/* update counters */
 	perf_end(_pc_txns);
@@ -393,6 +400,8 @@ ArchPX4IOSerial::_do_rx_dma_callback(unsigned status)
 
 		/* disable UART DMA */
 		rCR3 &= ~(USART_CR3_DMAT | USART_CR3_DMAR);
+		stm32_dmastop(_tx_dma);
+		stm32_dmastop(_rx_dma);
 
 		/* complete now */
 		px4_sem_post(&_completion_semaphore);
@@ -463,7 +472,7 @@ ArchPX4IOSerial::_do_interrupt()
 				/* stop the receive DMA */
 				stm32_dmastop(_rx_dma);
 
-				/* complete the short reception */
+				/* error flag completion of short reception */
 				_do_rx_dma_callback(DMA_STATUS_TEIF);
 				return;
 			}
@@ -482,12 +491,13 @@ ArchPX4IOSerial::_do_interrupt()
 void
 ArchPX4IOSerial::_abort_dma()
 {
+	/* disable UART DMA */
+	rCR3 &= ~(USART_CR3_DMAT | USART_CR3_DMAR);
+
 	/* stop DMA */
 	stm32_dmastop(_tx_dma);
 	stm32_dmastop(_rx_dma);
 
-	/* disable UART DMA */
-	rCR3 &= ~(USART_CR3_DMAT | USART_CR3_DMAR);
 
 	/* clear data that may be in the RDR and clear overrun error: */
 	if (rISR & USART_ISR_RXNE) {

platforms/nuttx/src/px4/stm/stm32h7/px4io_serial/px4io_serial.cpp

@@ -275,9 +275,6 @@ ArchPX4IOSerial::ioctl(unsigned operation, unsigned &arg)
 int
 ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 {
-	// to be paranoid ensure all previous DMA transfers are cleared
-	_abort_dma();
-
 	_current_packet = _packet;
 
 	/* clear data that may be in the RDR and clear overrun error: */
@@ -345,8 +342,6 @@ ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 	stm32_dmasetup(_tx_dma, &txdmacfg);
 	rCR3 |= USART_CR3_DMAT;
 	stm32_dmastart(_tx_dma, nullptr, nullptr, false);
-	//rCR1 &= ~USART_CR1_TE;
-	//rCR1 |= USART_CR1_TE;
 
 	/* compute the deadline for a 10ms timeout */
 	struct timespec abstime;
@@ -360,6 +355,7 @@ ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 
 	/* wait for the transaction to complete - 64 bytes @ 1.5Mbps ~426µs */
 	int ret;
+	irqstate_t irqs = enter_critical_section();
 
 	for (;;) {
 		ret = sem_timedwait(&_completion_semaphore, &abstime);
@@ -367,42 +363,53 @@ ArchPX4IOSerial::_bus_exchange(IOPacket *_packet)
 		if (ret == OK) {
 			/* check for DMA errors */
 			if (_rx_dma_status & DMA_STATUS_TEIF) {
-				// stream transfer error, ensure all DMA is also stopped before exiting early
+				/* One of 3 things has happened:
-				_abort_dma();
+				 *	1. a DMA Stream error
+				 *	2. Serial parity, framing or over run error
+				 *	3. packet is malformed
+				 * In all cases DMA is stopped by either HW or the ISR error service path.
+				 */
 				perf_count(_pc_dmaerrs);
 				ret = -EIO;
 				break;
-			}
 
-			/* check packet CRC - corrupt packet errors mean IO receive CRC error */
+			} else {
-			uint8_t crc = _current_packet->crc;
+				/* successful DMA completion but the crc can still fail */
-			_current_packet->crc = 0;
-
-			if ((crc != crc_packet(_current_packet)) || (PKT_CODE(*_current_packet) == PKT_CODE_CORRUPT)) {
-				_abort_dma();
-				perf_count(_pc_crcerrs);
-				ret = -EIO;
 				break;
 			}
 
-			/* successful txn (may still be reporting an error) */
+		} else {
-			break;
+			if (errno == ETIMEDOUT) {
+				/* something has broken - clear out any partial DMA state and reconfigure */
+				_abort_dma();
+				_rx_dma_status = _dma_status_inactive;
+				/* Wait fot at least a character time to make sure that there is no lingering
+				 * IDLE interrupt triggering right after we re-enable interrupts for the next
+				 * exchange
+				 */
+				usleep(100);
+				perf_count(_pc_timeouts);
+				perf_cancel(_pc_txns);    /* don't count this as a transaction */
+				break;
+			}
 		}
 
-		if (errno == ETIMEDOUT) {
+		/* Loop in case we are interrupted on sleep */
-			/* something has broken - clear out any partial DMA state and reconfigure */
-			_abort_dma();
-			perf_count(_pc_timeouts);
-			perf_cancel(_pc_txns);    /* don't count this as a transaction */
-			break;
-		}
-
-		/* we might? see this for EINTR */
-		syslog(LOG_ERR, "unexpected ret %d/%d\n", ret, errno);
 	}
 
-	/* reset DMA status */
 	_rx_dma_status = _dma_status_inactive;
+	leave_critical_section(irqs);
+
+	if (ret == OK) {
+		/* check packet CRC - corrupt packet errors mean IO receive CRC error */
+		uint8_t crc = _current_packet->crc;
+		_current_packet->crc = 0;
+
+		if ((crc != crc_packet(_current_packet)) || (PKT_CODE(*_current_packet) == PKT_CODE_CORRUPT)) {
+			perf_count(_pc_crcerrs);
+			ret = -EIO;
+		}
+	}
 
 	/* update counters */
 	perf_end(_pc_txns);
@@ -440,6 +447,8 @@ ArchPX4IOSerial::_do_rx_dma_callback(unsigned status)
 
 		/* disable UART DMA */
 		rCR3 &= ~(USART_CR3_DMAT | USART_CR3_DMAR);
+		stm32_dmastop(_tx_dma);
+		stm32_dmastop(_rx_dma);
 
 		/* complete now */
 		px4_sem_post(&_completion_semaphore);
@@ -510,7 +519,7 @@ ArchPX4IOSerial::_do_interrupt()
 				/* stop the receive DMA */
 				stm32_dmastop(_rx_dma);
 
-				/* complete the short reception */
+				/* error flag completion of short reception */
 				_do_rx_dma_callback(DMA_STATUS_TEIF);
 				return;
 			}
@@ -529,13 +538,13 @@ ArchPX4IOSerial::_do_interrupt()
 void
 ArchPX4IOSerial::_abort_dma()
 {
+	/* disable UART DMA */
+	rCR3 &= ~(USART_CR3_DMAT | USART_CR3_DMAR);
+
 	/* stop DMA */
 	stm32_dmastop(_tx_dma);
 	stm32_dmastop(_rx_dma);
 
-	/* disable UART DMA */
-	rCR3 &= ~(USART_CR3_DMAT | USART_CR3_DMAR);
-
 	/* clear data that may be in the RDR and clear overrun error: */
 	if (rISR & USART_ISR_RXNE) {
 		(void)rRDR;

src/drivers/px4io/px4io.cpp

@@ -1216,7 +1216,7 @@ int PX4IO::io_get_status()
 
 		uint16_t raw_inputs = io_reg_get(PX4IO_PAGE_RAW_RC_INPUT, PX4IO_P_RAW_RC_COUNT);
 
-		for (unsigned i = 0; i < raw_inputs; i++) {
+		for (unsigned i = 0; (i < raw_inputs) && (i < _max_rc_input); i++) {
 			status.raw_inputs[i] = io_reg_get(PX4IO_PAGE_RAW_RC_INPUT, PX4IO_P_RAW_RC_BASE + i);
 		}