linux-kernel - Re: [PATCH] clockevents/drivers/cs5535: improve resilience to spurious interrupts

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1710200011111.2054@nanos>
Date:   Fri, 20 Oct 2017 00:21:37 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     David Kozub <zub@...ux.fjfi.cvut.cz>
cc:     Daniel Lezcano <daniel.lezcano@...aro.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clockevents/drivers/cs5535: improve resilience to spurious
 interrupts

On Thu, 19 Oct 2017, David Kozub wrote:

> This solves a BUG on ALIX 2c3 where mfgpt_tick is called before
> clockevents_config_and_register returns. This caused mfgpt_tick to call a
> null function pointer.
> 
> Thanks to Daniel Lezcano and Thomas Gleixner for helping me analyze this
> and suggesting a solution.

Thanks for sending this! Though I have two minor issues with this:

1) The changelog.

We try to structure changelogs by explaining the context, the problem and
its consequences and the fix. If the fix is non obvious it wants an
elaborate explanation. Let me give you an example:

   The interrupt handler mfgpt_tick() is not robust versus spurious
   interrupts which happen before the clock event device is registered and
   fully initialized.

   The reason is that the safe guard against spurious interrupts solely
   checks for the clockevents shutdown state, but lacks a check for
   detached state. If the interrupt hits while the device is in detached
   state it passes the safe guard and dereferences the event handler call
   back which is NULL.

   Add the missing state check.

See?

> Suggested-by: Thomas Gleixner <tglx@...utronix.de>
> Signed-off-by: David Kozub <zub@...ux.fjfi.cvut.cz>
> ---
>  drivers/clocksource/cs5535-clockevt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clocksource/cs5535-clockevt.c b/drivers/clocksource/cs5535-clockevt.c
> index a1df588343f2..56100506b933 100644
> --- a/drivers/clocksource/cs5535-clockevt.c
> +++ b/drivers/clocksource/cs5535-clockevt.c
> @@ -117,7 +117,8 @@ static irqreturn_t mfgpt_tick(int irq, void *dev_id)
>  	/* Turn off the clock (and clear the event) */
>  	disable_timer(cs5535_event_clock);
>  
> -	if (clockevent_state_shutdown(&cs5535_clockevent))
> +	if (clockevent_state_detached(&cs5535_clockevent) ||
> +			clockevent_state_shutdown(&cs5535_clockevent))

Please do not use random indentation for multiline conditionals

	if (clockevent_state_detached(&cs5535_clockevent) ||
	    clockevent_state_shutdown(&cs5535_clockevent))

is the style we use through out the code.

Thanks,

	tglx