linux-kernel - Re: [PATCH] hrtimer: check hrtimer with a NULL function

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: <87plsvvgpn.fsf@somnus>
Date: Wed, 05 Jun 2024 12:15:00 +0200
From: Anna-Maria Behnsen <anna-maria@...utronix.de>
To: Phil Chang <phil.chang@...iatek.com>, phil.chang@...iatek.com
Cc: frederic@...nel.org, tglx@...utronix.de, matthias.bgg@...il.com,
 angelogioacchino.delregno@...labora.com, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, jy.ho@...iatek.com,
 alix.wu@...iatk.com
Subject: Re: [PATCH] hrtimer: check hrtimer with a NULL function

Hi,

Phil Chang <phil.chang@...iatek.com> writes:

> simillar with timers, check for timer->function == NULL.
> If the pointer is NULL, discard the request silently.

Can you please explain, why this change is required?

The statement "similar to timers" is not a valid explaination as timer
list timers and hrtimers are two different things. The function pointer
for timer list timers is explicitly set to NULL in shutdown path to
prevent unwanted rearming of the timer. For hrtimers there is no
shutdown function implemented and function is never set to NULL by
hrtimer code.

> Signed-off-by: Phil Chang <phil.chang@...iatek.com>
> ---
>  kernel/time/hrtimer.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 492c14aac642..72d6e7bc9cd9 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -1297,9 +1297,13 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
>  
>  	base = lock_hrtimer_base(timer, &flags);
>  
> +	if (!timer->function)
> +		goto out;

When this happens, user of hrtimers do not follow the semantics of
hrtimers which means this is a bug.

> +
>  	if (__hrtimer_start_range_ns(timer, tim, delta_ns, mode, base))
>  		hrtimer_reprogram(timer, true);
>  
> +out:
>  	unlock_hrtimer_base(timer, &flags);
>  }
>  EXPORT_SYMBOL_GPL(hrtimer_start_range_ns);
> @@ -1667,6 +1671,11 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
>  	__remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
>  	fn = timer->function;
>  
> +	if (WARN_ON_ONCE(!fn)) {
> +		/* Should never happen. */

...same as above...

> +		goto out;
> +	}
> +
>  	/*
>  	 * Clear the 'is relative' flag for the TIME_LOW_RES case. If the
>  	 * timer is restarted with a period then it becomes an absolute
> @@ -1710,6 +1719,7 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base,
>  	 * hrtimer_active() cannot observe base->running.timer == NULL &&
>  	 * timer->state == INACTIVE.
>  	 */
> +out:
>  	raw_write_seqcount_barrier(&base->seq);
>  
>  	WARN_ON_ONCE(base->running != timer);


Thanks,

	Anna-Maria