fix(IWatchdog): guard reload/window register computation against underflow#3021
Open
94xhn wants to merge 1 commit into
Open
fix(IWatchdog): guard reload/window register computation against underflow#302194xhn wants to merge 1 commit into
94xhn wants to merge 1 commit into
Conversation
…rflow IWatchdogClass::set() computes the IWDG reload register with reload = (uint32_t)(t_sec / div) - 1; (and the equivalent for the optional 'window' register), with no check that (uint32_t)(t_sec / div) is non-zero before subtracting 1. IWDG_TIMEOUT_MIN, the documented and enforced lower bound for the 'timeout' argument, is defined as (4*1000000)/LSI_VALUE using integer division. On most series (LSI_VALUE 32000 or 40000) this divides evenly, so calling begin(IWDG_TIMEOUT_MIN) is always safe. On STM32L0xx/STM32L1xx (LSI_VALUE = 37000), 4000000/37000 is not an integer (108.108...), so the macro truncates down to 108 -- a value IS_IWDG_TIMEOUT() accepts, but that is below the true precision needed: t_sec works out to 3.996 there, so (uint32_t)(t_sec / 4) floors to 0, and "0 - 1" underflows reload to 0xFFFFFFFF. LL_IWDG_SetReloadCounter() then masks that into the 12-bit RLR register as 0x0FFF (4095), the maximum possible value, instead of 0. Net effect: on STM32L0/L1, IWatchdog.begin(IWDG_TIMEOUT_MIN) -- the shortest legal, documented watchdog timeout -- silently programs a timeout of about 443 ms instead of about 108 us (roughly 4100x longer), with no error indication. The same underflow shape is reachable through the optional 'window' parameter for the same reason. Guard both computations with an explicit "ticks > 0" check before subtracting, returning 0 (the safe minimum reload) instead of underflowing, without changing the public API or behavior on any series where the computation was already safe. Signed-off-by: 94xhn <87560781+94xhn@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request template
Summary
IWatchdogClass::set()computes the IWDG reload register with an unguarded(uint32_t)(t_sec / div) - 1. On STM32L0xx/STM32L1xx (whereLSI_VALUE = 37000), callingIWatchdog.begin(IWDG_TIMEOUT_MIN)— the documented, validated minimum timeout — underflows this to0xFFFFFFFF, which gets masked into the watchdog's reload register as its maximum value instead of its minimum, silently turning the shortest legal watchdog timeout into one about 4100x longer.This PR fixes/implements the following bugs/features
IWatchdogClass::set()'s reload-register computation fortimeout(and the equivalent one for the optionalwindowargument) can underflow auint32_twhen the computed tick count floors to0, which is reachable through the library's own documentedIWDG_TIMEOUT_MINconstant on STM32L0xx/STM32L1xx.Motivation
In
libraries/IWatchdog/src/IWatchdog.h:IWDG_TIMEOUT_MINis computed with integer division.LSI_VALUE(the LSI oscillator frequency in Hz, per-series) is32000,37000or40000across this core's supported families (checked everyLSI_VALUEdefinition undersystem/Drivers/**/Inc/*.h).4000000divides evenly by32000and40000, but not by37000(STM32L0xx/STM32L1xx):4000000 / 37000 = 108.108..., truncated by integer division to108.In
libraries/IWatchdog/src/IWatchdog.cpp,IWatchdogClass::set():Calling
IWatchdog.begin(IWDG_TIMEOUT_MIN)on an L0/L1 part passesIS_IWDG_TIMEOUT(108)(true, since108 >= IWDG_TIMEOUT_MIN(108)), butt_sec = 108 * 37000 / 1e6 = 3.996— just below the4.0the formula actually needs atdiv = 4(the smallest, always-tried-first divider).(uint32_t)(3.996 / 4)floors to0, soreload = 0 - 1wraps auint32_tto0xFFFFFFFF.LL_IWDG_SetReloadCounter()then doesWRITE_REG(IWDGx->RLR, IWDG_RLR_RL & Counter)— masking to the 12-bit register — which writes0x0FFF(4095, the maximum reload value) instead of the intended0(minimum).Net effect: on STM32L0xx/STM32L1xx, asking for the shortest legal, documented watchdog timeout (
IWDG_TIMEOUT_MIN= 108 µs) silently programs an actual timeout of about 443 ms instead of about 108 µs — roughly 4100x longer — with no error return, no assertion, nothing. For a hardware watchdog, this is exactly the failure mode a caller can least afford: they asked for fast fault detection and silently got (almost) the slowest setting available. The identical underflow shape is reachable through the optionalwindowparameter for the same reason (same formula, few lines below).Fix
Compute the tick count into a temporary first and guard against it being
0before subtracting1, returning0(the safe minimum reload) instead of underflowing — for both thetimeout→reloadconversion and thewindow→reloadconversion. No public API/signature changes; behavior is unchanged for every series/timeout where the computation was already safe (i.e. everywhere except this exact STM32L0/L1 boundary case).Validation
Ensure CI build is passed.
This is pure integer/float arithmetic on plain scalars (no MCU register access besides the final masked register write, which I looked up directly in
stm32l0xx_ll_iwdg.h), so I extracted the formula verbatim into a standalone host-side C program and ran it on a desktop toolchain (independent of any STM32 board), sweeping the threeLSI_VALUEs actually used by this core:This confirms the underflow is real and specific to
LSI_VALUE = 37000(STM32L0xx/STM32L1xx) at the documented minimum timeout, and that the fix produces the correct/expected reload (and thus timeout) there while leaving every other series byte-for-byte unaffected.I do not own STM32L0/L1 hardware to run this on-target; the change only adds a zero-guard around a pure integer computation that is otherwise structurally identical to the surrounding, already-shipped code, so on-target risk is limited to the two branches touched.
Code formatting
Closing issues
N/A – found while reading the timer/tick-conversion code in this core after #3019 and #3020, not tied to an existing issue.
Disclosure: I used an AI coding assistant (Claude) to help read through the codebase, extract the reproduction, and draft this PR. All findings were verified by me by compiling and running the standalone reproduction shown above.