linux-kernel - Re: [PATCH] ipmi/watchdog: Stop watchdog timer when the current action is 'none'

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20210513162142.GA2921206@minyard.net>
Date:   Thu, 13 May 2021 11:21:42 -0500
From:   Corey Minyard <minyard@....org>
To:     Petr Pavlu <petr.pavlu@...e.com>
Cc:     openipmi-developer@...ts.sourceforge.net,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] ipmi/watchdog: Stop watchdog timer when the current
 action is 'none'

On Thu, May 13, 2021 at 02:26:36PM +0200, Petr Pavlu wrote:
> When an IPMI watchdog timer is being stopped in ipmi_close() or
> ipmi_ioctl(WDIOS_DISABLECARD), the current watchdog action is updated to
> WDOG_TIMEOUT_NONE and _ipmi_set_timeout(IPMI_SET_TIMEOUT_NO_HB) is called
> to install this action. The latter function ends up invoking
> __ipmi_set_timeout() which makes the actual 'Set Watchdog Timer' IPMI
> request.
> 
> For IPMI 1.0, this operation results in fully stopping the watchdog timer.
> For IPMI >= 1.5, function __ipmi_set_timeout() always specifies the "don't
> stop" flag in the prepared 'Set Watchdog Timer' IPMI request. This causes
> that the watchdog timer has its action correctly updated to 'none' but the
> timer continues to run. A problem is that IPMI firmware can then still log
> an expiration event when the configured timeout is reached, which is
> unexpected because the watchdog timer was requested to be stopped.
> 
> The patch fixes this problem by not setting the "don't stop" flag in
> __ipmi_set_timeout() when the current action is WDOG_TIMEOUT_NONE which
> results in stopping the watchdog timer. This makes the behaviour for
> IPMI >= 1.5 consistent with IPMI 1.0. It also matches the logic in
> __ipmi_heartbeat() which does not allow to reset the watchdog if the
> current action is WDOG_TIMEOUT_NONE as that would start the timer.

Yes, I believe this is correct, though it took a bit to be sure :).
Applied for linux-next.  I'm also requesting backport to stable kernels.

-corey

> 
> Signed-off-by: Petr Pavlu <petr.pavlu@...e.com>
> ---
>  drivers/char/ipmi/ipmi_watchdog.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c
> index 32c334e34d55..e4ff3b50de7f 100644
> --- a/drivers/char/ipmi/ipmi_watchdog.c
> +++ b/drivers/char/ipmi/ipmi_watchdog.c
> @@ -371,16 +371,18 @@ static int __ipmi_set_timeout(struct ipmi_smi_msg  *smi_msg,
>  	data[0] = 0;
>  	WDOG_SET_TIMER_USE(data[0], WDOG_TIMER_USE_SMS_OS);
>  
> -	if ((ipmi_version_major > 1)
> -	    || ((ipmi_version_major == 1) && (ipmi_version_minor >= 5))) {
> -		/* This is an IPMI 1.5-only feature. */
> -		data[0] |= WDOG_DONT_STOP_ON_SET;
> -	} else if (ipmi_watchdog_state != WDOG_TIMEOUT_NONE) {
> -		/*
> -		 * In ipmi 1.0, setting the timer stops the watchdog, we
> -		 * need to start it back up again.
> -		 */
> -		hbnow = 1;
> +	if (ipmi_watchdog_state != WDOG_TIMEOUT_NONE) {
> +		if ((ipmi_version_major > 1) ||
> +		    ((ipmi_version_major == 1) && (ipmi_version_minor >= 5))) {
> +			/* This is an IPMI 1.5-only feature. */
> +			data[0] |= WDOG_DONT_STOP_ON_SET;
> +		} else {
> +			/*
> +			 * In ipmi 1.0, setting the timer stops the watchdog, we
> +			 * need to start it back up again.
> +			 */
> +			hbnow = 1;
> +		}
>  	}
>  
>  	data[1] = 0;
> -- 
> 2.26.2
>