| Message ID | 20220628054539.33187-1-schspa@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: dw_wdt: Fix buffer overflow when get timeout | expand |
On 6/27/22 22:45, Schspa Shi wrote: > The top_val can be obtained from device-tree, if it is not configured > correctly, there will be buffer overflow. > > Signed-off-by: Schspa Shi <schspa@gmail.com> > --- > drivers/watchdog/dw_wdt.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c > index cd578843277e..1f8605c0d712 100644 > --- a/drivers/watchdog/dw_wdt.c > +++ b/drivers/watchdog/dw_wdt.c > @@ -155,6 +155,9 @@ static unsigned int dw_wdt_get_min_timeout(struct dw_wdt *dw_wdt) > break; > } > > + if (WARN_ON_ONCE(idx == DW_WDT_NUM_TOPS)) > + idx = DW_WDT_NUM_TOPS - 1; > + dw_wdt_get_min_timeout() returns the lowest non-0 configurable timeout. The last entry in the timeout array must not be 0, meaning there must be at least one entry in the array where the timeout is not 0. Therefore this situation can not happen. > return dw_wdt->timeouts[idx].sec; > } > > @@ -178,6 +181,9 @@ static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt) > break; > } > > + if (WARN_ON_ONCE(idx == DW_WDT_NUM_TOPS)) > + idx = DW_WDT_NUM_TOPS - 1; > + idx is derived from a top_val value written into WDOG_TIMEOUT_RANGE_REG_OFFSET, and the value written is derived from an entry in the timeouts array. This array contains an entry for each possible top_val. While the array is not sorted by top_val, dw_wdt_handle_tops() still guarantees that an entry exists. I do not see how bad devicetree data can circumvent that. If it does, please provide an example and explain. Thanks, Guenter
Guenter Roeck <linux@roeck-us.net> writes: > On 6/27/22 22:45, Schspa Shi wrote: >> The top_val can be obtained from device-tree, if it is not configured >> correctly, there will be buffer overflow. >> Signed-off-by: Schspa Shi <schspa@gmail.com> >> --- >> drivers/watchdog/dw_wdt.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c >> index cd578843277e..1f8605c0d712 100644 >> --- a/drivers/watchdog/dw_wdt.c >> +++ b/drivers/watchdog/dw_wdt.c >> @@ -155,6 +155,9 @@ static unsigned int dw_wdt_get_min_timeout(struct dw_wdt *dw_wdt) >> break; >> } >> + if (WARN_ON_ONCE(idx == DW_WDT_NUM_TOPS)) >> + idx = DW_WDT_NUM_TOPS - 1; >> + > dw_wdt_get_min_timeout() returns the lowest non-0 configurable timeout. > The last entry in the timeout array must not be 0, meaning there must > be at least one entry in the array where the timeout is not 0. Therefore > this situation can not happen. > Yes, I have seen this, you are correct, sorry for the bad patch. >> return dw_wdt->timeouts[idx].sec; >> } >> @@ -178,6 +181,9 @@ static unsigned int dw_wdt_get_timeout(struct dw_wdt >> *dw_wdt) >> break; >> } >> + if (WARN_ON_ONCE(idx == DW_WDT_NUM_TOPS)) >> + idx = DW_WDT_NUM_TOPS - 1; >> + > > idx is derived from a top_val value written into WDOG_TIMEOUT_RANGE_REG_OFFSET, > and the value written is derived from an entry in the timeouts array. > This array contains an entry for each possible top_val. While the array is not > sorted by top_val, dw_wdt_handle_tops() still guarantees that an entry exists. > > I do not see how bad devicetree data can circumvent that. If it does, please > provide an example and explain. > My bad, there is no problem. > Thanks, > Guenter
diff --git a/drivers/watchdog/dw_wdt.c b/drivers/watchdog/dw_wdt.c index cd578843277e..1f8605c0d712 100644 --- a/drivers/watchdog/dw_wdt.c +++ b/drivers/watchdog/dw_wdt.c @@ -155,6 +155,9 @@ static unsigned int dw_wdt_get_min_timeout(struct dw_wdt *dw_wdt) break; } + if (WARN_ON_ONCE(idx == DW_WDT_NUM_TOPS)) + idx = DW_WDT_NUM_TOPS - 1; + return dw_wdt->timeouts[idx].sec; } @@ -178,6 +181,9 @@ static unsigned int dw_wdt_get_timeout(struct dw_wdt *dw_wdt) break; } + if (WARN_ON_ONCE(idx == DW_WDT_NUM_TOPS)) + idx = DW_WDT_NUM_TOPS - 1; + /* * In IRQ mode due to the two stages counter, the actual timeout is * twice greater than the TOP setting.
The top_val can be obtained from device-tree, if it is not configured correctly, there will be buffer overflow. Signed-off-by: Schspa Shi <schspa@gmail.com> --- drivers/watchdog/dw_wdt.c | 6 ++++++ 1 file changed, 6 insertions(+)