Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <c230ccdc-b20d-32a6-c3cb-715698d06945@roeck-us.net>
Date:   Tue, 28 Jun 2022 06:55:29 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Content-Language: en-US
To:     Schspa Shi <schspa@gmail.com>, wim@linux-watchdog.org
Cc:     linux-watchdog@vger.kernel.org, linux-kernel@vger.kernel.org,
        zhaohui.shi@horizon.ai
References: <20220628054539.33187-1-schspa@gmail.com>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH] watchdog: dw_wdt: Fix buffer overflow when get timeout
In-Reply-To: <20220628054539.33187-1-schspa@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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