From 921d3dc2b2c0afb0cc3e7bc7b623e4fd613ee27c Mon Sep 17 00:00:00 2001 From: Jun Lin Date: Wed, 11 Dec 2024 17:54:18 +0800 Subject: [PATCH 1/2] driver: timer: npcx: fix possible vulnerabilities This commit fixes some potential leakage in the timer driver. Signed-off-by: Jun Lin (cherry picked from commit bdf0500497624f4f6c7b33e7247b3d3c71eeaf4f) --- drivers/timer/npcx_itim_timer.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/drivers/timer/npcx_itim_timer.c b/drivers/timer/npcx_itim_timer.c index fba2bc2b3c113..ce4a60b2b7773 100644 --- a/drivers/timer/npcx_itim_timer.c +++ b/drivers/timer/npcx_itim_timer.c @@ -140,8 +140,6 @@ static inline void npcx_itim_evt_disable(void) /* ITIM local functions */ static int npcx_itim_start_evt_tmr_by_tick(int32_t ticks) { - k_spinlock_key_t key = k_spin_lock(&lock); - /* * Get desired cycles of event timer from the requested ticks which * round up to next tick boundary. @@ -152,7 +150,7 @@ static int npcx_itim_start_evt_tmr_by_tick(int32_t ticks) } else { uint64_t next_cycs; uint64_t curr = npcx_itim_get_sys_cyc64(); - uint32_t dcycles; + uint64_t dcycles; if (ticks <= 0) { ticks = 1; @@ -162,11 +160,13 @@ static int npcx_itim_start_evt_tmr_by_tick(int32_t ticks) if (unlikely(next_cycs <= curr)) { cyc_evt_timeout = 1; } else { + uint32_t dticks; + dcycles = next_cycs - curr; - cyc_evt_timeout = - CLAMP((dcycles / SYS_CYC_PER_EVT_CYC), 1, NPCX_ITIM32_MAX_CNT); + dticks = DIV_ROUND_UP(dcycles * EVT_CYCLES_PER_SEC, + sys_clock_hw_cycles_per_sec()); + cyc_evt_timeout = CLAMP(dticks, 1, NPCX_ITIM32_MAX_CNT); } - } LOG_DBG("ticks %x, cyc_evt_timeout %x", ticks, cyc_evt_timeout); @@ -178,7 +178,6 @@ static int npcx_itim_start_evt_tmr_by_tick(int32_t ticks) /* Upload counter of event timer */ evt_tmr->ITCNT32 = MAX(cyc_evt_timeout - 1, 1); - k_spin_unlock(&lock, key); /* Enable event timer and start ticking */ return npcx_itim_evt_enable(); } @@ -234,13 +233,13 @@ static uint32_t npcx_itim_evt_elapsed_cyc32(void) { uint32_t cnt1 = npcx_itim_get_evt_cyc32(); uint8_t sys_cts = evt_tmr->ITCTS32; - uint16_t cnt2 = npcx_itim_get_evt_cyc32(); + uint32_t cnt2 = npcx_itim_get_evt_cyc32(); /* Event has been triggered but timer ISR doesn't handle it */ if (IS_BIT_SET(sys_cts, NPCX_ITCTSXX_TO_STS) || (cnt2 > cnt1)) { cnt2 = cyc_evt_timeout; } else { - cnt2 = cyc_evt_timeout - cnt2; + cnt2 = cyc_evt_timeout - cnt2 - 1; } /* Return elapsed cycles of 32-bit counter of event timer */ @@ -260,7 +259,10 @@ void sys_clock_set_timeout(int32_t ticks, bool idle) LOG_DBG("timeout is %d", ticks); /* Start a event timer in ticks */ + + k_spinlock_key_t key = k_spin_lock(&lock); npcx_itim_start_evt_tmr_by_tick(ticks); + k_spin_unlock(&lock, key); } uint32_t sys_clock_elapsed(void) @@ -272,7 +274,7 @@ uint32_t sys_clock_elapsed(void) k_spinlock_key_t key = k_spin_lock(&lock); uint64_t delta_cycle = npcx_itim_get_sys_cyc64() - cyc_sys_announced; - uint32_t delta_ticks = (uint32_t)delta_cycle / SYS_CYCLES_PER_TICK; + uint32_t delta_ticks = (uint32_t)(delta_cycle / SYS_CYCLES_PER_TICK); last_elapsed = delta_ticks; k_spin_unlock(&lock, key); From a94ab5ae0b2dd9ad8db12085ef47f324747ce905 Mon Sep 17 00:00:00 2001 From: Jun Lin Date: Wed, 11 Dec 2024 17:51:12 +0800 Subject: [PATCH 2/2] driver: timer: npcx: bypass timer counter reading issue Originally, when the timer's source clock is 32.768 kHz, the timer driver uses two consecutive reads to ensure the timer reading is correct. However, it is not robust enough due to an asynchronous timing issue in the chip. The workaround is to add at least two NOPs between the LDR and CMP instructions. This commit implements the workaround in the assembly code to ensure it is not affected by the compiler toolchain or optimization flags. Signed-off-by: Jun Lin (cherry picked from commit 2bda7b87ee13cd57a2d05935b0a9ffa10fbffabe) --- drivers/timer/npcx_itim_timer.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/drivers/timer/npcx_itim_timer.c b/drivers/timer/npcx_itim_timer.c index ce4a60b2b7773..5960df666e404 100644 --- a/drivers/timer/npcx_itim_timer.c +++ b/drivers/timer/npcx_itim_timer.c @@ -217,14 +217,18 @@ static inline uint32_t npcx_itim_get_evt_cyc32(void) { uint32_t cnt1, cnt2; - cnt1 = evt_tmr->ITCNT32; - /* - * Wait for two consecutive equal values are read since the source clock - * of event timer is 32KHz. - */ - while ((cnt2 = evt_tmr->ITCNT32) != cnt1) - cnt1 = cnt2; - + __asm__ volatile( + "ldr %[c2], [%[tmr], %[itcnt32_off]]\n\t" + ".read_itim_cnt_loop_%=:\n\t" + "mov %[c1], %[c2]\n\t" + "ldr %[c2], [%[tmr], %[itcnt32_off]]\n\t" + "nop\n\t" + "nop\n\t" + "cmp %[c1], %[c2]\n\t" + "bne .read_itim_cnt_loop_%=\n\t" + : [c1] "=&r"(cnt1), [c2] "=&r"(cnt2) + : [tmr] "r"(evt_tmr), [itcnt32_off] "i"(offsetof(struct itim32_reg, ITCNT32)) + : "memory"); /* Return current value of 32-bit counter of event timer */ return cnt2; }