Skip to content

Commit

Permalink
drivers: counter: Fix possible out-of-bounds access in MCP7940N driver
Browse files Browse the repository at this point in the history 
This PR fixes the issue of possible out-of-bounds
access if write_data_block function was misused.

Size argument of the function was only validated
against the maximum allowed value, not the real
size of the data struct to be read, what could
cause accessing data past the boundary of the struct.

Should fix #81930.

Signed-off-by: Marcin Lyda <[email protected]>
  • Loading branch information
@Lefucjusz
Lefucjusz committed Jan 24, 2025
1 parent 4ed85ee commit 7c576ac
Showing 1 changed file with 48 additions and 68 deletions.
116 changes: 48 additions & 68 deletions drivers/counter/rtc_mcp7940n.c
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*
* Copyright (c) 2019-2020 Peter Bigot Consulting, LLC
* Copyright (c) 2021 Laird Connectivity
* Copyright (c) 2025 Marcin Lyda <[email protected]>
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -26,20 +27,17 @@
LOG_MODULE_REGISTER(MCP7940N, CONFIG_COUNTER_LOG_LEVEL);

/* Alarm channels */
#define ALARM0_ID 0
#define ALARM1_ID 1
#define ALARM0_ID 0
#define ALARM1_ID 1

/* Size of block when writing whole struct */
#define RTC_TIME_REGISTERS_SIZE sizeof(struct mcp7940n_time_registers)
#define RTC_ALARM_REGISTERS_SIZE sizeof(struct mcp7940n_alarm_registers)

/* Largest block size */
#define MAX_WRITE_SIZE (RTC_TIME_REGISTERS_SIZE)
#define RTC_TIME_REGISTERS_SIZE sizeof(struct mcp7940n_time_registers)
#define RTC_ALARM_REGISTERS_SIZE sizeof(struct mcp7940n_alarm_registers)

/* tm struct uses years since 1900 but unix time uses years since
* 1970. MCP7940N default year is '1' so the offset is 69
*/
#define UNIX_YEAR_OFFSET 69
#define UNIX_YEAR_OFFSET 69

/* Macro used to decode BCD to UNIX time to avoid potential copy and paste
* errors.
Expand Down Expand Up @@ -79,7 +77,7 @@ static time_t decode_rtc(const struct device *dev)
{
struct mcp7940n_data *data = dev->data;
time_t time_unix = 0;
struct tm time = { 0 };
struct tm time = {0};

time.tm_sec = RTC_BCD_DECODE(data->registers.rtc_sec.sec);
time.tm_min = RTC_BCD_DECODE(data->registers.rtc_min.min);
Expand All @@ -89,8 +87,7 @@ static time_t decode_rtc(const struct device *dev)
/* tm struct starts months at 0, mcp7940n starts at 1 */
time.tm_mon = RTC_BCD_DECODE(data->registers.rtc_month.month) - 1;
/* tm struct uses years since 1900 but unix time uses years since 1970 */
time.tm_year = RTC_BCD_DECODE(data->registers.rtc_year.year) +
UNIX_YEAR_OFFSET;
time.tm_year = RTC_BCD_DECODE(data->registers.rtc_year.year) + UNIX_YEAR_OFFSET;

time_unix = timeutil_timegm(&time);

Expand Down Expand Up @@ -252,43 +249,41 @@ static int write_register(const struct device *dev, enum mcp7940n_register addr,
*
* @param dev the MCP7940N device pointer.
* @param addr first register address to write to, should be REG_RTC_SEC,
* REG_ALM0_SEC or REG_ALM0_SEC.
* @param size size of data struct that will be written.
* REG_ALM0_SEC or REG_ALM1_SEC.
*
* @retval return 0 on success, or a negative error code from an I2C
* transaction or invalid parameter.
*/
static int write_data_block(const struct device *dev, enum mcp7940n_register addr, uint8_t size)
static int write_data_block(const struct device *dev, enum mcp7940n_register addr)
{
struct mcp7940n_data *data = dev->data;
const struct mcp7940n_config *cfg = dev->config;
int rc = 0;
uint8_t time_data[MAX_WRITE_SIZE + 1];
uint8_t *write_block_start;

if (size > MAX_WRITE_SIZE) {
return -EINVAL;
}
int rc;
const uint8_t *write_block_start;
uint8_t write_block_size;

if (addr >= REG_INVAL) {
return -EINVAL;
}

if (addr == REG_RTC_SEC) {
switch (addr) {
case REG_RTC_SEC:
write_block_start = (uint8_t *)&data->registers;
} else if (addr == REG_ALM0_SEC) {
write_block_size = sizeof(data->registers);
break;
case REG_ALM0_SEC:
write_block_start = (uint8_t *)&data->alm0_registers;
} else if (addr == REG_ALM1_SEC) {
write_block_size = sizeof(data->alm0_registers);
break;
case REG_ALM1_SEC:
write_block_start = (uint8_t *)&data->alm1_registers;
} else {
write_block_size = sizeof(data->alm1_registers);
break;
default:
return -EINVAL;
}

/* Load register address into first byte then fill in data values */
time_data[0] = addr;
memcpy(&time_data[1], write_block_start, size);

rc = i2c_write_dt(&cfg->i2c, time_data, size + 1);
rc = i2c_burst_write_dt(&cfg->i2c, addr, write_block_start, write_block_size);

return rc;
}
Expand All @@ -308,13 +303,13 @@ static int write_data_block(const struct device *dev, enum mcp7940n_register add
static int set_day_of_week(const struct device *dev, time_t *unix_time)
{
struct mcp7940n_data *data = dev->data;
struct tm time_buffer = { 0 };
struct tm time_buffer = {0};
int rc = 0;

if (gmtime_r(unix_time, &time_buffer) != NULL) {
data->registers.rtc_weekday.weekday = time_buffer.tm_wday;
rc = write_register(dev, REG_RTC_WDAY,
*((uint8_t *)(&data->registers.rtc_weekday)));
*((uint8_t *)(&data->registers.rtc_weekday)));
} else {
rc = -EINVAL;
}
Expand Down Expand Up @@ -356,8 +351,7 @@ static void mcp7940n_handle_interrupt(const struct device *dev, uint8_t alarm_id
if (alm_regs->alm_weekday.alm_if) {
/* Clear interrupt */
alm_regs->alm_weekday.alm_if = 0;
write_register(dev, alarm_reg_address,
*((uint8_t *)(&alm_regs->alm_weekday)));
write_register(dev, alarm_reg_address, *((uint8_t *)(&alm_regs->alm_weekday)));

/* Fire callback */
if (data->counter_handler[alarm_id]) {
Expand All @@ -376,19 +370,16 @@ static void mcp7940n_handle_interrupt(const struct device *dev, uint8_t alarm_id

static void mcp7940n_work_handler(struct k_work *work)
{
struct mcp7940n_data *data =
CONTAINER_OF(work, struct mcp7940n_data, alarm_work);
struct mcp7940n_data *data = CONTAINER_OF(work, struct mcp7940n_data, alarm_work);

/* Check interrupt flags for both alarms */
mcp7940n_handle_interrupt(data->mcp7940n, ALARM0_ID);
mcp7940n_handle_interrupt(data->mcp7940n, ALARM1_ID);
}

static void mcp7940n_init_cb(const struct device *dev,
struct gpio_callback *gpio_cb, uint32_t pins)
static void mcp7940n_init_cb(const struct device *dev, struct gpio_callback *gpio_cb, uint32_t pins)
{
struct mcp7940n_data *data =
CONTAINER_OF(gpio_cb, struct mcp7940n_data, int_callback);
struct mcp7940n_data *data = CONTAINER_OF(gpio_cb, struct mcp7940n_data, int_callback);

ARG_UNUSED(pins);

Expand All @@ -398,7 +389,7 @@ static void mcp7940n_init_cb(const struct device *dev,
int mcp7940n_rtc_set_time(const struct device *dev, time_t unix_time)
{
struct mcp7940n_data *data = dev->data;
struct tm time_buffer = { 0 };
struct tm time_buffer = {0};
int rc = 0;

if (unix_time > UINT32_MAX) {
Expand All @@ -421,7 +412,7 @@ int mcp7940n_rtc_set_time(const struct device *dev, time_t unix_time)
}

/* Write to device */
rc = write_data_block(dev, REG_RTC_SEC, RTC_TIME_REGISTERS_SIZE);
rc = write_data_block(dev, REG_RTC_SEC);

out:
k_sem_give(&data->lock);
Expand All @@ -438,8 +429,7 @@ static int mcp7940n_counter_start(const struct device *dev)

/* Set start oscillator configuration bit */
data->registers.rtc_sec.start_osc = 1;
rc = write_register(dev, REG_RTC_SEC,
*((uint8_t *)(&data->registers.rtc_sec)));
rc = write_register(dev, REG_RTC_SEC, *((uint8_t *)(&data->registers.rtc_sec)));

k_sem_give(&data->lock);

Expand All @@ -455,16 +445,14 @@ static int mcp7940n_counter_stop(const struct device *dev)

/* Clear start oscillator configuration bit */
data->registers.rtc_sec.start_osc = 0;
rc = write_register(dev, REG_RTC_SEC,
*((uint8_t *)(&data->registers.rtc_sec)));
rc = write_register(dev, REG_RTC_SEC, *((uint8_t *)(&data->registers.rtc_sec)));

k_sem_give(&data->lock);

return rc;
}

static int mcp7940n_counter_get_value(const struct device *dev,
uint32_t *ticks)
static int mcp7940n_counter_get_value(const struct device *dev, uint32_t *ticks)
{
struct mcp7940n_data *data = dev->data;
time_t unix_time;
Expand Down Expand Up @@ -492,7 +480,7 @@ static int mcp7940n_counter_set_alarm(const struct device *dev, uint8_t alarm_id
uint32_t seconds_until_alarm;
time_t current_time;
time_t alarm_time;
struct tm time_buffer = { 0 };
struct tm time_buffer = {0};
uint8_t alarm_base_address;
struct mcp7940n_alarm_registers *alm_regs;
int rc = 0;
Expand Down Expand Up @@ -536,14 +524,13 @@ static int mcp7940n_counter_set_alarm(const struct device *dev, uint8_t alarm_id

/* Write time to alarm registers */
encode_alarm(dev, &time_buffer, alarm_id);
rc = write_data_block(dev, alarm_base_address, RTC_ALARM_REGISTERS_SIZE);
rc = write_data_block(dev, alarm_base_address);
if (rc < 0) {
goto out;
}

/* Enable alarm */
rc = write_register(dev, REG_RTC_CONTROL,
*((uint8_t *)(&data->registers.rtc_control)));
rc = write_register(dev, REG_RTC_CONTROL, *((uint8_t *)(&data->registers.rtc_control)));
if (rc < 0) {
goto out;
}
Expand Down Expand Up @@ -576,8 +563,7 @@ static int mcp7940n_counter_cancel_alarm(const struct device *dev, uint8_t alarm
goto out;
}

rc = write_register(dev, REG_RTC_CONTROL,
*((uint8_t *)(&data->registers.rtc_control)));
rc = write_register(dev, REG_RTC_CONTROL, *((uint8_t *)(&data->registers.rtc_control)));

out:
k_sem_give(&data->lock);
Expand Down Expand Up @@ -608,8 +594,7 @@ static uint32_t mcp7940n_counter_get_pending_int(const struct device *dev)
k_sem_take(&data->lock, K_FOREVER);

/* Check interrupt flag for alarm 0 */
rc = read_register(dev, REG_ALM0_WDAY,
(uint8_t *)&data->alm0_registers.alm_weekday);
rc = read_register(dev, REG_ALM0_WDAY, (uint8_t *)&data->alm0_registers.alm_weekday);
if (rc < 0) {
goto out;
}
Expand All @@ -618,16 +603,15 @@ static uint32_t mcp7940n_counter_get_pending_int(const struct device *dev)
/* Clear interrupt */
data->alm0_registers.alm_weekday.alm_if = 0;
rc = write_register(dev, REG_ALM0_WDAY,
*((uint8_t *)(&data->alm0_registers.alm_weekday)));
*((uint8_t *)(&data->alm0_registers.alm_weekday)));
if (rc < 0) {
goto out;
}
interrupt_pending |= (1 << ALARM0_ID);
}

/* Check interrupt flag for alarm 1 */
rc = read_register(dev, REG_ALM1_WDAY,
(uint8_t *)&data->alm1_registers.alm_weekday);
rc = read_register(dev, REG_ALM1_WDAY, (uint8_t *)&data->alm1_registers.alm_weekday);
if (rc < 0) {
goto out;
}
Expand All @@ -636,7 +620,7 @@ static uint32_t mcp7940n_counter_get_pending_int(const struct device *dev)
/* Clear interrupt */
data->alm1_registers.alm_weekday.alm_if = 0;
rc = write_register(dev, REG_ALM1_WDAY,
*((uint8_t *)(&data->alm1_registers.alm_weekday)));
*((uint8_t *)(&data->alm1_registers.alm_weekday)));
if (rc < 0) {
goto out;
}
Expand Down Expand Up @@ -685,8 +669,7 @@ static int mcp7940n_init(const struct device *dev)

/* Set 24-hour time */
data->registers.rtc_hours.twelve_hr = false;
rc = write_register(dev, REG_RTC_HOUR,
*((uint8_t *)(&data->registers.rtc_hours)));
rc = write_register(dev, REG_RTC_HOUR, *((uint8_t *)(&data->registers.rtc_hours)));
if (rc < 0) {
goto out;
}
Expand All @@ -695,8 +678,7 @@ static int mcp7940n_init(const struct device *dev)
if (cfg->int_gpios.port != NULL) {

if (!gpio_is_ready_dt(&cfg->int_gpios)) {
LOG_ERR("Port device %s is not ready",
cfg->int_gpios.port->name);
LOG_ERR("Port device %s is not ready", cfg->int_gpios.port->name);
rc = -ENODEV;
goto out;
}
Expand All @@ -706,11 +688,9 @@ static int mcp7940n_init(const struct device *dev)

gpio_pin_configure_dt(&cfg->int_gpios, GPIO_INPUT);

gpio_pin_interrupt_configure_dt(&cfg->int_gpios,
GPIO_INT_EDGE_TO_ACTIVE);
gpio_pin_interrupt_configure_dt(&cfg->int_gpios, GPIO_INT_EDGE_TO_ACTIVE);

gpio_init_callback(&data->int_callback, mcp7940n_init_cb,
BIT(cfg->int_gpios.pin));
gpio_init_callback(&data->int_callback, mcp7940n_init_cb, BIT(cfg->int_gpios.pin));

(void)gpio_add_callback(cfg->int_gpios.port, &data->int_callback);

Expand Down

0 comments on commit 7c576ac

Please sign in to comment.