| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <00af4a7d-357f-4bef-8cf9-7d1f8790cc3f@roeck-us.net>
Date: Thu, 1 Aug 2024 08:19:00 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Harini T <harini.t@....com>, michal.simek@....com,
srinivas.neeli@....com, shubhrajyoti.datta@....com
Cc: srinivas.goud@....com, wim@...ux-watchdog.org,
linux-watchdog@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, git@....com
Subject: Re: [PATCH V2] watchdog: xilinx_wwdt: Calculate max_hw_heartbeat_ms
using clock frequency
On 7/14/24 22:35, Harini T wrote:
> In the current implementation, the value of max_hw_heartbeat_ms is set
> to the timeout period expressed in milliseconds and fails to verify if
> the close window percentage exceeds the maximum value that the hardware
> supports.
>
> 1. Calculate max_hw_heartbeat_ms based on input clock frequency.
> 2. Limit the close and open window percent to hardware supported value
> to avoid truncation.
> 3. If the user input timeout exceeds the maximum timeout supported,
> use only open window and the framework supports the higher timeouts.
>
> Fixes: 12984cea1b8c ("watchdog: xilinx_wwdt: Add Versal window watchdog support")
>
> Signed-off-by: Harini T <harini.t@....com>
> ---
>
> Changes in V2:
> - Modify the implementation to make the timeout independent of the
> maximum hardware timeout as the framework supports it.
> - Modify the implementation to obtain ticks from milliseconds instead of
> ticks from seconds to avoid the minor time difference between hardware
> and software.
> - Limit the close and open window percentage to hardware supported value
> to avoid truncation.
> - If the timeout exceeds the maximum timeout supported, use only open
> window and set the min_hw_heartbeat_ms to zero.
> - Modify the commit title and description.
> - Add Fixes tag in commit description.
>
> V1 link: https://lore.kernel.org/all/20240319111219.21094-1-harini.t@amd.com/
>
> ---
> drivers/watchdog/xilinx_wwdt.c | 70 +++++++++++++++++++++++++++++-----
> 1 file changed, 60 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/watchdog/xilinx_wwdt.c b/drivers/watchdog/xilinx_wwdt.c
> index d271e2e8d6e2..94a8a0275cd8 100644
> --- a/drivers/watchdog/xilinx_wwdt.c
> +++ b/drivers/watchdog/xilinx_wwdt.c
> @@ -36,6 +36,12 @@
>
> #define XWWDT_CLOSE_WINDOW_PERCENT 50
>
> +/* Maximum count value of each 32 bit window */
> +#define XWWDT_MAX_COUNT_WINDOW GENMASK(31, 0)
> +
> +/* Maximum count value of closed and open window combined */
> +#define XWWDT_MAX_COUNT_WINDOW_COMBINED GENMASK_ULL(32, 1)
> +
> static int wwdt_timeout;
> static int closed_window_percent;
>
> @@ -54,6 +60,8 @@ MODULE_PARM_DESC(closed_window_percent,
> * @xilinx_wwdt_wdd: watchdog device structure
> * @freq: source clock frequency of WWDT
> * @close_percent: Closed window percent
> + * @closed_timeout: Closed window timeout in ticks
> + * @open_timeout: Open window timeout in ticks
> */
> struct xwwdt_device {
> void __iomem *base;
> @@ -61,27 +69,22 @@ struct xwwdt_device {
> struct watchdog_device xilinx_wwdt_wdd;
> unsigned long freq;
> u32 close_percent;
> + u64 closed_timeout;
> + u64 open_timeout;
> };
>
> static int xilinx_wwdt_start(struct watchdog_device *wdd)
> {
> struct xwwdt_device *xdev = watchdog_get_drvdata(wdd);
> struct watchdog_device *xilinx_wwdt_wdd = &xdev->xilinx_wwdt_wdd;
> - u64 time_out, closed_timeout, open_timeout;
> u32 control_status_reg;
>
> - /* Calculate timeout count */
> - time_out = xdev->freq * wdd->timeout;
> - closed_timeout = div_u64(time_out * xdev->close_percent, 100);
> - open_timeout = time_out - closed_timeout;
> - wdd->min_hw_heartbeat_ms = xdev->close_percent * 10 * wdd->timeout;
> -
> spin_lock(&xdev->spinlock);
>
> iowrite32(XWWDT_MWR_MASK, xdev->base + XWWDT_MWR_OFFSET);
> iowrite32(~(u32)XWWDT_ESR_WEN_MASK, xdev->base + XWWDT_ESR_OFFSET);
> - iowrite32((u32)closed_timeout, xdev->base + XWWDT_FWR_OFFSET);
> - iowrite32((u32)open_timeout, xdev->base + XWWDT_SWR_OFFSET);
> + iowrite32((u32)xdev->closed_timeout, xdev->base + XWWDT_FWR_OFFSET);
> + iowrite32((u32)xdev->open_timeout, xdev->base + XWWDT_SWR_OFFSET);
>
> /* Enable the window watchdog timer */
> control_status_reg = ioread32(xdev->base + XWWDT_ESR_OFFSET);
> @@ -133,7 +136,12 @@ static int xwwdt_probe(struct platform_device *pdev)
> struct watchdog_device *xilinx_wwdt_wdd;
> struct device *dev = &pdev->dev;
> struct xwwdt_device *xdev;
> + u64 max_per_window_ms;
> + u64 min_per_window_ms;
> + u64 timeout_count;
> struct clk *clk;
> + u64 timeout_ms;
Why u64 ? If that is really needed it would result in overflows
throughout the watchdog subsystem, which assumes that the timeout
is not larger than UINT_MAX / 1000.
> + u64 ms_count;
> int ret;
>
> xdev = devm_kzalloc(dev, sizeof(*xdev), GFP_KERNEL);
> @@ -159,7 +167,8 @@ static int xwwdt_probe(struct platform_device *pdev)
>
> xilinx_wwdt_wdd->min_timeout = XWWDT_MIN_TIMEOUT;
> xilinx_wwdt_wdd->timeout = XWWDT_DEFAULT_TIMEOUT;
> - xilinx_wwdt_wdd->max_hw_heartbeat_ms = 1000 * xilinx_wwdt_wdd->timeout;
> + xilinx_wwdt_wdd->max_hw_heartbeat_ms =
> + div64_u64(XWWDT_MAX_COUNT_WINDOW_COMBINED, xdev->freq) * 1000;
I don't know if it matters in practice, but this calculation will overflow if
the clock rate is below 2,000 Hz.
>
> if (closed_window_percent == 0 || closed_window_percent >= 100)
> xdev->close_percent = XWWDT_CLOSE_WINDOW_PERCENT;
> @@ -167,6 +176,47 @@ static int xwwdt_probe(struct platform_device *pdev)
> xdev->close_percent = closed_window_percent;
>
> watchdog_init_timeout(xilinx_wwdt_wdd, wwdt_timeout, &pdev->dev);
> +
> + /* Calculate ticks for 1 milli-second */
> + ms_count = div64_u64(xdev->freq, 1000);
Why div64_u64 here ? freq is unsigned long, and dividing it by 1000 will
never require an explicit 64-bit operation.
Overall it seems to me that the generous use of 64-bit operations and
variables raises the risk of overflows. I am not going to verify each
calculation and check for overflows, though. I just hope that it won't
bite you in the future.
> + timeout_ms = xilinx_wwdt_wdd->timeout * 1000;
> + timeout_count = timeout_ms * ms_count;
> +
> + if (timeout_ms > xilinx_wwdt_wdd->max_hw_heartbeat_ms) {
> + /* To avoid ping restrictions until the minimum hardware heartbeat,
> + * we will solely rely on the open window and
> + * adjust the minimum hardware heartbeat to 0.
> + */
We still use standard multi-line comments in the watchdog subsystem.
> + xdev->closed_timeout = 0;
> + xdev->open_timeout = XWWDT_MAX_COUNT_WINDOW;
> + xilinx_wwdt_wdd->min_hw_heartbeat_ms = 0;
> + xilinx_wwdt_wdd->max_hw_heartbeat_ms = xilinx_wwdt_wdd->max_hw_heartbeat_ms / 2;
> + } else {
> + xdev->closed_timeout = div64_u64(timeout_count * xdev->close_percent, 100);
> + xilinx_wwdt_wdd->min_hw_heartbeat_ms =
> + div64_u64(timeout_ms * xdev->close_percent, 100);
> +
> + if (timeout_ms > xilinx_wwdt_wdd->max_hw_heartbeat_ms / 2) {
> + max_per_window_ms = xilinx_wwdt_wdd->max_hw_heartbeat_ms / 2;
> + min_per_window_ms = timeout_ms - max_per_window_ms;
> +
> + if (xilinx_wwdt_wdd->min_hw_heartbeat_ms > max_per_window_ms) {
> + dev_info(xilinx_wwdt_wdd->parent,
> + "Closed window cannot be set to %d%%. Using maximum supported value.\n",
> + xdev->close_percent);
> + xdev->closed_timeout = max_per_window_ms * ms_count;
> + xilinx_wwdt_wdd->min_hw_heartbeat_ms = max_per_window_ms;
> + } else if (xilinx_wwdt_wdd->min_hw_heartbeat_ms < min_per_window_ms) {
> + dev_info(xilinx_wwdt_wdd->parent,
> + "Closed window cannot be set to %d%%. Using minimum supported value.\n",
> + xdev->close_percent);
> + xdev->closed_timeout = min_per_window_ms * ms_count;
> + xilinx_wwdt_wdd->min_hw_heartbeat_ms = min_per_window_ms;
> + }
> + }
> + xdev->open_timeout = timeout_count - xdev->closed_timeout;
> + }
> +
> spin_lock_init(&xdev->spinlock);
> watchdog_set_drvdata(xilinx_wwdt_wdd, xdev);
> watchdog_set_nowayout(xilinx_wwdt_wdd, 1);
Powered by blists - more mailing lists