From 562062b30308aa15f37da556ace9d8a7daab21e3 Mon Sep 17 00:00:00 2001 From: Jacob Dahl <37091262+dakejahl@users.noreply.github.com> Date: Fri, 24 Jan 2020 03:01:26 -0700 Subject: [PATCH] batt_smbus: fix potential buffer overrun bug (#13904) --- src/drivers/batt_smbus/batt_smbus.cpp | 30 +++++++++++++-------------- src/drivers/batt_smbus/batt_smbus.h | 4 ++-- src/lib/drivers/smbus/SMBus.hpp | 4 ++-- 3 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/drivers/batt_smbus/batt_smbus.cpp b/src/drivers/batt_smbus/batt_smbus.cpp index c2484d6b9c..2a7bb97213 100644 --- a/src/drivers/batt_smbus/batt_smbus.cpp +++ b/src/drivers/batt_smbus/batt_smbus.cpp @@ -339,23 +339,17 @@ void BATT_SMBUS::set_undervoltage_protection(float average_current) } //@NOTE: Currently unused, could be helpful for debugging a parameter set though. -int BATT_SMBUS::dataflash_read(uint16_t &address, void *data) +int BATT_SMBUS::dataflash_read(uint16_t &address, void *data, const unsigned length) { uint8_t code = BATT_SMBUS_MANUFACTURER_BLOCK_ACCESS; - // address is 2 bytes int result = _interface->block_write(code, &address, 2, true); if (result != PX4_OK) { return result; } - // @NOTE: The data buffer MUST be 32 bytes. - result = _interface->block_read(code, data, DATA_BUFFER_SIZE + 2, true); - - // When reading a BATT_SMBUS_MANUFACTURER_BLOCK_ACCESS the first 2 bytes will be the command code - // We will remove these since we do not care about the command code. - //memcpy(data, &((uint8_t *)data)[2], DATA_BUFFER_SIZE); + result = _interface->block_read(code, data, length, true); return result; } @@ -364,10 +358,15 @@ int BATT_SMBUS::dataflash_write(uint16_t &address, void *data, const unsigned le { uint8_t code = BATT_SMBUS_MANUFACTURER_BLOCK_ACCESS; - uint8_t tx_buf[DATA_BUFFER_SIZE + 2] = {}; + uint8_t tx_buf[MAC_DATA_BUFFER_SIZE + 2] = {}; tx_buf[0] = ((uint8_t *)&address)[0]; tx_buf[1] = ((uint8_t *)&address)[1]; + + if (length > MAC_DATA_BUFFER_SIZE) { + return PX4_ERROR; + } + memcpy(&tx_buf[2], data, length); // code (1), byte_count (1), addr(2), data(32) + pec @@ -489,9 +488,8 @@ int BATT_SMBUS::manufacturer_read(const uint16_t cmd_code, void *data, const uns return result; } - // returns the 2 bytes of addr + data[] - result = _interface->block_read(code, data, length + 2, true); - memcpy(data, &((uint8_t *)data)[2], length); + result = _interface->block_read(code, data, length, true); + memmove(data, &((uint8_t *)data)[2], length - 2); // remove the address bytes return result; } @@ -504,7 +502,7 @@ int BATT_SMBUS::manufacturer_write(const uint16_t cmd_code, void *data, const un address[0] = ((uint8_t *)&cmd_code)[0]; address[1] = ((uint8_t *)&cmd_code)[1]; - uint8_t tx_buf[DATA_BUFFER_SIZE + 2] = {}; + uint8_t tx_buf[MAC_DATA_BUFFER_SIZE + 2] = {}; memcpy(tx_buf, address, 2); if (data != nullptr) { @@ -545,10 +543,10 @@ int BATT_SMBUS::lifetime_data_flush() int BATT_SMBUS::lifetime_read_block_one() { + int buffer_size = 32 + 2; // 32 bytes of data and 2 bytes of address + uint8_t lifetime_block_one[buffer_size] = {}; - uint8_t lifetime_block_one[32] = {}; - - if (PX4_OK != manufacturer_read(BATT_SMBUS_LIFETIME_BLOCK_ONE, lifetime_block_one, 32)) { + if (PX4_OK != manufacturer_read(BATT_SMBUS_LIFETIME_BLOCK_ONE, lifetime_block_one, buffer_size)) { PX4_INFO("Failed to read lifetime block 1."); return PX4_ERROR; } diff --git a/src/drivers/batt_smbus/batt_smbus.h b/src/drivers/batt_smbus/batt_smbus.h index 89b8978d30..5bbe04d327 100644 --- a/src/drivers/batt_smbus/batt_smbus.h +++ b/src/drivers/batt_smbus/batt_smbus.h @@ -54,7 +54,7 @@ #include "board_config.h" -#define DATA_BUFFER_SIZE 32 +#define MAC_DATA_BUFFER_SIZE 32 #define BATT_CELL_VOLTAGE_THRESHOLD_RTL 0.5f ///< Threshold in volts to RTL if cells are imbalanced #define BATT_CELL_VOLTAGE_THRESHOLD_FAILED 1.5f ///< Threshold in volts to Land if cells are imbalanced @@ -147,7 +147,7 @@ public: * @param data The returned data. * @return Returns PX4_OK on success, PX4_ERROR on failure. */ - int dataflash_read(uint16_t &address, void *data); + int dataflash_read(uint16_t &address, void *data, const unsigned length); /** * @brief Writes data to flash. diff --git a/src/lib/drivers/smbus/SMBus.hpp b/src/lib/drivers/smbus/SMBus.hpp index 2a19b79afd..2a3f70adc3 100644 --- a/src/lib/drivers/smbus/SMBus.hpp +++ b/src/lib/drivers/smbus/SMBus.hpp @@ -62,8 +62,8 @@ public: /** * @brief Sends a block read command. * @param cmd_code The command code. - * @param data The returned data. - * @param length The number of bytes being read + * @param data The returned data. The returned data will always contain 2 bytes of address information followed by data[length]. + * @param length The number of bytes being read. * @return Returns PX4_OK on success, -errno on failure. */ int block_read(const uint8_t cmd_code, void *data, const uint8_t length, bool use_pec);