[PATCH] hrtimer: check hrtimer with a NULL function

[PATCH] hrtimer: check hrtimer with a NULL function

[Phil Chang]

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

Signed-off-by: Phil Chang <phil.chang@mediatek.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;
+
 	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. */
+		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);
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] hrtimer: check hrtimer with a NULL function

[Anna-Maria Behnsen]

Hi,

Phil Chang <phil.chang@mediatek.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@mediatek.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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] hrtimer: check hrtimer with a NULL function

[Phil Chang]

>> 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@mediatek.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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] hrtimer: check hrtimer with a NULL function

[Anna-Maria Behnsen]

Phil Chang <phil.chang@mediatek.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.
>>
> 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.

To make it more clear to the inexperienced hrtimer user that it is
mandatory to initialize timer->function to be able to use hrtimers, but
to prevent the kernel to crash, please add a warning in
hrtimer_start_range_ns(). The check can be done without locking the
hrtimer base. When the function pointer is not set, return directly
without doing anything. So this above mentioned backtrace will no longer
appear, as the timer is not added to a queue.

Thanks,

	Anna-Maria

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2] hrtimer: check hrtimer with a NULL function

[Phil Chang]

Since hrtimers do not allow a NULL function to be passed,
to prevent a kernel crash, return before adding the timer to a queue.

Signed-off-by: Phil Chang <phil.chang@mediatek.com>
---
 kernel/time/hrtimer.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 492c14aac642..d32c1afe59b3 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1285,6 +1285,9 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	struct hrtimer_clock_base *base;
 	unsigned long flags;
 
+	if (WARN_ON(!timer->function))
+		return;
+
 	/*
 	 * Check whether the HRTIMER_MODE_SOFT bit and hrtimer.is_soft
 	 * match on CONFIG_PREEMPT_RT = n. With PREEMPT_RT check the hard
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] hrtimer: check hrtimer with a NULL function

[Anna-Maria Behnsen]

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

> Since hrtimers do not allow a NULL function to be passed,
> to prevent a kernel crash, return before adding the timer to a queue.

You could point out in the commit message, where the kernel would crash
when using a hrtimer with a NULL pointer as function pointer. And it
shouldn't be a problem to use more than a single sentence for a commit
message :)

> Signed-off-by: Phil Chang <phil.chang@mediatek.com>
> ---
>  kernel/time/hrtimer.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 492c14aac642..d32c1afe59b3 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -1285,6 +1285,9 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
>  	struct hrtimer_clock_base *base;
>  	unsigned long flags;
>  
> +	if (WARN_ON(!timer->function))
> +		return;
> +

Please use a WARN_ON_ONCE() (as documented in
Documentation/process/coding-style.rst)

>  	/*
>  	 * Check whether the HRTIMER_MODE_SOFT bit and hrtimer.is_soft
>  	 * match on CONFIG_PREEMPT_RT = n. With PREEMPT_RT check the hard

Thanks,

	Anna-Maria


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3] hrtimer: check hrtimer with a NULL function

[Phil Chang]

To prevent improper usage of hrtimers and avoid potential kernel crashes,
this commit introduces a validation check for hrtimers with a valid function callback,
discard the hrtimers that have a NULL callback.

The `run_hrtimer` executes callbacks for every hrtimer,
and these callbacks must not be NULL. A NULL callback can lead to a kernel crash.
This update ensures that all hrtimers have properly initialized callbacks
before execution.

Signed-off-by: Phil Chang <phil.chang@mediatek.com>
---
 kernel/time/hrtimer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 492c14aac642..b8ee320208d4 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1285,6 +1285,8 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	struct hrtimer_clock_base *base;
 	unsigned long flags;
 
+	if (WARN_ON_ONCE(!timer->function))
+		return;
 	/*
 	 * Check whether the HRTIMER_MODE_SOFT bit and hrtimer.is_soft
 	 * match on CONFIG_PREEMPT_RT = n. With PREEMPT_RT check the hard
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] hrtimer: check hrtimer with a NULL function

[Anna-Maria Behnsen]

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

> To prevent improper usage of hrtimers and avoid potential kernel crashes,
> this commit introduces a validation check for hrtimers with a valid function callback,
> discard the hrtimers that have a NULL callback.
>
> The `run_hrtimer` executes callbacks for every hrtimer,
> and these callbacks must not be NULL. A NULL callback can lead to a kernel crash.
> This update ensures that all hrtimers have properly initialized callbacks
> before execution.

Definitely better! You could sort it, first problem description and then
solution:

The hrtimer function callback must not be NULL. It has to be specified
by the callsite but it is not validated by hrtimer code. When a hrtimer
is queued with a NULL pointer instead of a valid function, the kernel
crashes with a null pointer dereference when trying to execute the
callback in __run_hrtimer().

Introduce a validation before queueing the hrtimer in
hrtimer_start_range_ns().

>
> Signed-off-by: Phil Chang <phil.chang@mediatek.com>

Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de>

> ---
>  kernel/time/hrtimer.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 492c14aac642..b8ee320208d4 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -1285,6 +1285,8 @@ void hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
>  	struct hrtimer_clock_base *base;
>  	unsigned long flags;
>  
> +	if (WARN_ON_ONCE(!timer->function))
> +		return;
>  	/*
>  	 * Check whether the HRTIMER_MODE_SOFT bit and hrtimer.is_soft
>  	 * match on CONFIG_PREEMPT_RT = n. With PREEMPT_RT check the hard