This issue was found with syzkaller.
UBSAN: Undefined behaviour in kernel/time/hrtimer.c:612:10
signed integer overflow:
9223372036854775807 - -51224496 cannot be represented in type
'long long int'
To fix this issue, add and use a function to check for overflows when
substracting two ktime_t values. This is largly the solution already once
submitted once as RFC. See Link below.
Link: https://lore.kernel.org/lkml/[email protected]/
Signed-off-by: Simone Weiss <[email protected]>
---
include/linux/ktime.h | 7 +++++++
kernel/time/hrtimer.c | 22 +++++++++++++++++++---
2 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/include/linux/ktime.h b/include/linux/ktime.h
index 73f20deb497d..ed90f53741f4 100644
--- a/include/linux/ktime.h
+++ b/include/linux/ktime.h
@@ -55,6 +55,12 @@ static inline ktime_t ktime_set(const s64 secs, const unsigned long nsecs)
*/
#define ktime_add_unsafe(lhs, rhs) ((u64) (lhs) + (rhs))
+/*
+ * Same as ktime_sub(), but avoids undefined behaviour on overflow; however,
+ * this means that you must check the result for overflow yourself.
+ */
+#define ktime_sub_unsafe(lhs, rhs) ((u64) (lhs) - (rhs))
+
/*
* Add a ktime_t variable and a scalar nanosecond value.
* res = kt + nsval:
@@ -197,6 +203,7 @@ static inline ktime_t ktime_sub_ms(const ktime_t kt, const u64 msec)
}
extern ktime_t ktime_add_safe(const ktime_t lhs, const ktime_t rhs);
+extern ktime_t ktime_sub_safe(const ktime_t lhs, const ktime_t rhs);
/**
* ktime_to_timespec64_cond - convert a ktime_t variable to timespec64
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 760793998cdd..cd0534bb60a8 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -195,7 +195,7 @@ hrtimer_check_target(struct hrtimer *timer, struct hrtimer_clock_base *new_base)
{
ktime_t expires;
- expires = ktime_sub(hrtimer_get_expires(timer), new_base->offset);
+ expires = ktime_sub_safe(hrtimer_get_expires(timer), new_base->offset);
return expires < new_base->cpu_base->expires_next;
}
@@ -342,6 +342,22 @@ ktime_t ktime_add_safe(const ktime_t lhs, const ktime_t rhs)
EXPORT_SYMBOL_GPL(ktime_add_safe);
+/*
+ * Sub two ktime values and do a safety check for overflow:
+ */
+ktime_t ktime_sub_safe(const ktime_t lhs, const ktime_t rhs)
+{
+ ktime_t res = ktime_sub_unsafe(lhs, rhs);
+
+ if (lhs > 0 && rhs < 0 && res < 0)
+ res = ktime_set(KTIME_SEC_MAX, 0);
+ else if (lhs < 0 && rhs > 0 && res > 0)
+ res = ktime_set(-KTIME_SEC_MAX, 0);
+
+ return res;
+}
+EXPORT_SYMBOL_GPL(ktime_sub_safe);
+
#ifdef CONFIG_DEBUG_OBJECTS_TIMERS
static const struct debug_obj_descr hrtimer_debug_descr;
@@ -523,7 +539,7 @@ static ktime_t __hrtimer_next_event_base(struct hrtimer_cpu_base *cpu_base,
timer = container_of(next, struct hrtimer, node);
}
- expires = ktime_sub(hrtimer_get_expires(timer), base->offset);
+ expires = ktime_sub_safe(hrtimer_get_expires(timer), base->offset);
if (expires < expires_next) {
expires_next = expires;
@@ -811,7 +827,7 @@ static void hrtimer_reprogram(struct hrtimer *timer, bool reprogram)
{
struct hrtimer_cpu_base *cpu_base = this_cpu_ptr(&hrtimer_bases);
struct hrtimer_clock_base *base = timer->base;
- ktime_t expires = ktime_sub(hrtimer_get_expires(timer), base->offset);
+ ktime_t expires = ktime_sub_safe(hrtimer_get_expires(timer), base->offset);
WARN_ON_ONCE(hrtimer_get_expires_tv64(timer) < 0);
--
2.34.1
On Tue, Dec 19 2023 at 13:44, Simone Weiss wrote:
> UBSAN: Undefined behaviour in kernel/time/hrtimer.c:612:10
> signed integer overflow:
> 9223372036854775807 - -51224496 cannot be represented in type
> 'long long int'
Some explanation about the context and why the offset is < 0 would be
helpful.
> +/*
> + * Sub two ktime values and do a safety check for overflow:
> + */
> +ktime_t ktime_sub_safe(const ktime_t lhs, const ktime_t rhs)
> +{
> + ktime_t res = ktime_sub_unsafe(lhs, rhs);
> +
> + if (lhs > 0 && rhs < 0 && res < 0)
> + res = ktime_set(KTIME_SEC_MAX, 0);
> + else if (lhs < 0 && rhs > 0 && res > 0)
> + res = ktime_set(-KTIME_SEC_MAX, 0);
Looking at the use cases, the lhs < 0 case would be a bug because the
expiry values are strictly >= 0.
> +
> + return res;
> +}
> +EXPORT_SYMBOL_GPL(ktime_sub_safe);
That export is needed because? The only usage is in this file so this
can be static, no?
Thanks,
tglx
On Fri, 2024-01-12 at 17:14 +0100, Thomas Gleixner wrote:
> CAUTION: This email originated from outside of the Elektrobit organization. Do
> not click links or open attachments unless you recognize the sender and know
> the content is safe.
>
>
> On Tue, Dec 19 2023 at 13:44, Simone Weiss wrote:
> > UBSAN: Undefined behaviour in kernel/time/hrtimer.c:612:10
> > signed integer overflow:
> > 9223372036854775807 - -51224496 cannot be represented in type
> > 'long long int'
>
> Some explanation about the context and why the offset is < 0 would be
> helpful.
>
> > +/*
> > + * Sub two ktime values and do a safety check for overflow:
> > + */
> > +ktime_t ktime_sub_safe(const ktime_t lhs, const ktime_t rhs)
> > +{
> > + ktime_t res = ktime_sub_unsafe(lhs, rhs);
> > +
> > + if (lhs > 0 && rhs < 0 && res < 0)
> > + res = ktime_set(KTIME_SEC_MAX, 0);
> > + else if (lhs < 0 && rhs > 0 && res > 0)
> > + res = ktime_set(-KTIME_SEC_MAX, 0);
>
> Looking at the use cases, the lhs < 0 case would be a bug because the
> expiry values are strictly >= 0.
>
> > +
> > + return res;
> > +}
> > +EXPORT_SYMBOL_GPL(ktime_sub_safe);
>
> That export is needed because? The only usage is in this file so this
> can be static, no?
>
> Thanks,
>
> tglx
Hi,
after more investigation it seems to me that this issue is only present in older
kernels like 4.14.y or 4.19.y. A stack trace I obtained from fuzzing 4.14 is:
Hardware name: linux,dummy-virt (DT)
Call trace:
dump_backtrace+0x0/0x330
show_stack+0x20/0x30 arch/arm64/kernel/traps.c:156
__dump_stack lib/dump_stack.c:17 [inline]
dump_stack+0x12c/0x180 lib/dump_stack.c:58
ubsan_epilogue+0x18/0x50 lib/ubsan.c:166
handle_overflow+0xf4/0x118 lib/ubsan.c:197
__ubsan_handle_sub_overflow+0x34/0x48 lib/ubsan.c:211
hrtimer_reprogram kernel/time/hrtimer.c:612 [inline]
hrtimer_start_range_ns+0x768/0x858 kernel/time/hrtimer.c:993
hrtimer_start include/linux/hrtimer.h:377 [inline]
timerfd_setup fs/timerfd.c:205 [inline]
do_timerfd_settime fs/timerfd.c:496 [inline]
SYSC_timerfd_settime fs/timerfd.c:544 [inline]
SyS_timerfd_settime+0x4c0/0x7e0 fs/timerfd.c:535
el0_svc_naked+0x34/0x38
My present assumption is, that it is already fixed in newer kernel versions
via this commit:
6cd889d43c40b ("timerfd: Make timerfd_settime() time namespace aware").
I will check further.
Best,
Simone