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: <20240605135106.16065-1-phil.chang@mediatek.com>
Date: Wed, 5 Jun 2024 21:51:06 +0800
From: Phil Chang <phil.chang@...iatek.com>
To: <anna-maria@...utronix.de>
CC: <alix.wu@...iatek.com>, <angelogioacchino.delregno@...labora.com>,
	<frederic@...nel.org>, <jy.ho@...iatek.com>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<matthias.bgg@...il.com>, <phil.chang@...iatek.com>, <tglx@...utronix.de>
Subject: Re: [PATCH] hrtimer: check hrtimer with a NULL function

>> 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.
>
The timer->function is provided  by caller, which is invaild if fuction is NULL, 
and currently, the hrtime code does not perform any checks to validate this. 
Passing a NULL function can lead to a system panic, with a backtrace likes:
```
   __hrtimer_run_queues+0x1d8/0x3b8
   hrtimer_interrupt+0xdc/0x3a0
   arch_timer_handler_phys+0x54/0x94
   handle_percpu_devid_irq+0xb8/0x308
   handle_domain_irq+0x78/0xec
   gic_handle_irq+0x50/0x10c
   call_on_irq_stack+0x38/0x54
   do_interrupt_handler+0x40/0x98
```
This backtrace does not clearly indicate the source of the invalid usage of hrtimer.

>> 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.
>
Agree, how about BUG_ON here ?
>> +
>>  	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

Thank You