2022-06-28 05:59:54

by Schspa Shi

Subject: [PATCH] watchdog: dw_wdt: Fix buffer overflow when get timeout

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 <[email protected]>
---
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;
+
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.
--
2.29.0

2022-06-28 14:31:40

by Guenter Roeck

Subject: Re: [PATCH] watchdog: dw_wdt: Fix buffer overflow when get timeout

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 <[email protected]>
> ---
> 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

2022-06-28 14:35:28

by Schspa Shi

Subject: Re: [PATCH] watchdog: dw_wdt: Fix buffer overflow when get timeout

Guenter Roeck <[email protected]> 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 <[email protected]>
>> ---
>> 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


--
BRs
Schspa Shi