From 44a18acd513a7f6cff94add5e85fc9bf2e75762e Mon Sep 17 00:00:00 2001 From: Thomas Debrunner Date: Thu, 18 Aug 2022 17:46:47 +0200 Subject: [PATCH] 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 --- .../stm/stm32f4/px4io_serial/px4io_serial.cpp | 69 +++++++++-------- .../stm/stm32f7/px4io_serial/px4io_serial.cpp | 74 ++++++++++-------- .../stm/stm32h7/px4io_serial/px4io_serial.cpp | 75 +++++++++++-------- src/drivers/px4io/px4io.cpp | 2 +- 4 files changed, 124 insertions(+), 96 deletions(-) diff --git a/platforms/nuttx/src/px4/stm/stm32f4/px4io_serial/px4io_serial.cpp b/platforms/nuttx/src/px4/stm/stm32f4/px4io_serial/px4io_serial.cpp index c86b67c73f..0ab41dd384 100644 --- a/platforms/nuttx/src/px4/stm/stm32f4/px4io_serial/px4io_serial.cpp +++ b/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 - _abort_dma(); + /* One of 3 things has happened: + * 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 */ - uint8_t crc = _current_packet->crc; - _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; + } else { + /* successful DMA completion but the crc can still fail */ break; } - /* successful txn (may still be reporting an error) */ - break; + } else { + 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) { - /* 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); + /* Loop in case we are interrupted on sleep */ } - /* 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; } diff --git a/platforms/nuttx/src/px4/stm/stm32f7/px4io_serial/px4io_serial.cpp b/platforms/nuttx/src/px4/stm/stm32f7/px4io_serial/px4io_serial.cpp index a640166e05..2899c8db3f 100644 --- a/platforms/nuttx/src/px4/stm/stm32f7/px4io_serial/px4io_serial.cpp +++ b/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 - _abort_dma(); + /* One of 3 things has happened: + * 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 */ - uint8_t crc = _current_packet->crc; - _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; + } else { + /* successful DMA completion but the crc can still fail */ break; } - /* successful txn (may still be reporting an error) */ - break; + } else { + 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) { - /* 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); + /* Loop in case we are interrupted on sleep */ } - /* 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) { diff --git a/platforms/nuttx/src/px4/stm/stm32h7/px4io_serial/px4io_serial.cpp b/platforms/nuttx/src/px4/stm/stm32h7/px4io_serial/px4io_serial.cpp index 1492a7fc19..3b97f834e0 100644 --- a/platforms/nuttx/src/px4/stm/stm32h7/px4io_serial/px4io_serial.cpp +++ b/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 - _abort_dma(); + /* One of 3 things has happened: + * 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 */ - uint8_t crc = _current_packet->crc; - _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; + } else { + /* successful DMA completion but the crc can still fail */ break; } - /* successful txn (may still be reporting an error) */ - break; + } else { + 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) { - /* 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); + /* Loop in case we are interrupted on sleep */ } - /* 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; diff --git a/src/drivers/px4io/px4io.cpp b/src/drivers/px4io/px4io.cpp index 016d1ce128..61f27f4e38 100644 --- a/src/drivers/px4io/px4io.cpp +++ b/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); }