| Message ID | 20241114203112.57228-6-massimiliano.pellizzer@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | CVE-2024-35887 | expand |
On Thu, Nov 14, 2024 at 09:31:07PM +0100, Massimiliano Pellizzer wrote: > From: Thomas Gleixner <tglx@linutronix.de> > > Tearing down timers which have circular dependencies to other > functionality, e.g. workqueues, where the timer can schedule work and work > can arm timers, is not trivial. > > In those cases it is desired to shutdown the timer in a way which prevents > rearming of the timer. The mechanism to do so is to set timer->function to > NULL and use this as an indicator for the timer arming functions to ignore > the (re)arm request. > > Add a shutdown argument to the relevant internal functions which makes the > actual deactivation code set timer->function to NULL which in turn prevents > rearming of the timer. > > Co-developed-by: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > Tested-by: Guenter Roeck <linux@roeck-us.net> > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > Reviewed-by: Anna-Maria Behnsen <anna-maria@linutronix.de> > Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home > Link: https://lore.kernel.org/all/20221110064101.429013735@goodmis.org > Link: https://lore.kernel.org/r/20221123201625.253883224@linutronix.de > > (cherry picked from commit 0cc04e80458a822300b93f82ed861a513edde194) > CVE-2024-35887 > Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer@canonical.com> > --- > kernel/time/timer.c | 62 +++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 54 insertions(+), 8 deletions(-) > > diff --git a/kernel/time/timer.c b/kernel/time/timer.c > index 2aca1a7edf6c..a47bc6f294b3 100644 > --- a/kernel/time/timer.c > +++ b/kernel/time/timer.c > @@ -1208,12 +1208,19 @@ EXPORT_SYMBOL_GPL(add_timer_on); > /** > * __timer_delete - Internal function: Deactivate a timer > * @timer: The timer to be deactivated > + * @shutdown: If true, this indicates that the timer is about to be > + * shutdown permanently. > + * > + * If @shutdown is true then @timer->function is set to NULL under the > + * timer base lock which prevents further rearming of the time. In that > + * case any attempt to rearm @timer after this function returns will be > + * silently ignored. > * > * Return: > * * %0 - The timer was not pending > * * %1 - The timer was pending and deactivated > */ > -static int __timer_delete(struct timer_list *timer) > +static int __timer_delete(struct timer_list *timer, bool shutdown) > { > struct timer_base *base; > unsigned long flags; > @@ -1221,9 +1228,22 @@ static int __timer_delete(struct timer_list *timer) > > debug_assert_init(timer); > > - if (timer_pending(timer)) { > + /* > + * If @shutdown is set then the lock has to be taken whether the > + * timer is pending or not to protect against a concurrent rearm > + * which might hit between the lockless pending check and the lock > + * aquisition. By taking the lock it is ensured that such a newly > + * enqueued timer is dequeued and cannot end up with > + * timer->function == NULL in the expiry code. > + * > + * If timer->function is currently executed, then this makes sure > + * that the callback cannot requeue the timer. > + */ > + if (timer_pending(timer) || shutdown) { > base = lock_timer_base(timer, &flags); > ret = detach_if_pending(timer, base, true); > + if (shutdown) > + timer->function = NULL; > raw_spin_unlock_irqrestore(&base->lock, flags); > } > > @@ -1246,20 +1266,31 @@ static int __timer_delete(struct timer_list *timer) > */ > int timer_delete(struct timer_list *timer) > { > - return __timer_delete(timer); > + return __timer_delete(timer, false); > } > EXPORT_SYMBOL(timer_delete); > > /** > * __try_to_del_timer_sync - Internal function: Try to deactivate a timer > * @timer: Timer to deactivate > + * @shutdown: If true, this indicates that the timer is about to be > + * shutdown permanently. > + * > + * If @shutdown is true then @timer->function is set to NULL under the > + * timer base lock which prevents further rearming of the timer. Any > + * attempt to rearm @timer after this function returns will be silently > + * ignored. > + * > + * This function cannot guarantee that the timer cannot be rearmed > + * right after dropping the base lock if @shutdown is false. That > + * needs to be prevented by the calling code if necessary. > * > * Return: > * * %0 - The timer was not pending > * * %1 - The timer was pending and deactivated > * * %-1 - The timer callback function is running on a different CPU > */ > -static int __try_to_del_timer_sync(struct timer_list *timer) > +static int __try_to_del_timer_sync(struct timer_list *timer, bool shutdown) > { > struct timer_base *base; > unsigned long flags; > @@ -1271,6 +1302,8 @@ static int __try_to_del_timer_sync(struct timer_list *timer) > > if (base->running_timer != timer) > ret = detach_if_pending(timer, base, true); > + if (shutdown) > + timer->function = NULL; > > raw_spin_unlock_irqrestore(&base->lock, flags); > > @@ -1295,7 +1328,7 @@ static int __try_to_del_timer_sync(struct timer_list *timer) > */ > int try_to_del_timer_sync(struct timer_list *timer) > { > - return __try_to_del_timer_sync(timer); > + return __try_to_del_timer_sync(timer, false); > } > EXPORT_SYMBOL(try_to_del_timer_sync); > > @@ -1376,12 +1409,25 @@ static inline void del_timer_wait_running(struct timer_list *timer) { } > * __timer_delete_sync - Internal function: Deactivate a timer and wait > * for the handler to finish. > * @timer: The timer to be deactivated > + * @shutdown: If true, @timer->function will be set to NULL under the > + * timer base lock which prevents rearming of @timer > + * > + * If @shutdown is not set the timer can be rearmed later. If the timer can > + * be rearmed concurrently, i.e. after dropping the base lock then the > + * return value is meaningless. > + * > + * If @shutdown is set then @timer->function is set to NULL under timer > + * base lock which prevents rearming of the timer. Any attempt to rearm > + * a shutdown timer is silently ignored. > + * > + * If the timer should be reused after shutdown it has to be initialized > + * again. > * > * Return: > * * %0 - The timer was not pending > * * %1 - The timer was pending and deactivated > */ > -static int __timer_delete_sync(struct timer_list *timer) > +static int __timer_delete_sync(struct timer_list *timer, bool shutdown) > { > int ret; > > @@ -1411,7 +1457,7 @@ static int __timer_delete_sync(struct timer_list *timer) > lockdep_assert_preemption_enabled(); > > do { > - ret = __try_to_del_timer_sync(timer); > + ret = __try_to_del_timer_sync(timer, shutdown); > > if (unlikely(ret < 0)) { > del_timer_wait_running(timer); > @@ -1463,7 +1509,7 @@ static int __timer_delete_sync(struct timer_list *timer) > */ > int timer_delete_sync(struct timer_list *timer) > { > - return __timer_delete_sync(timer); > + return __timer_delete_sync(timer, false); > } > EXPORT_SYMBOL(timer_delete_sync); > > -- > 2.43.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team It seems to me that this commit implicitly relies on another missing upstream commit, which takes care of rearming side: d02e382cef06 ("timers: Silently ignore timers with a NULL function") What do you think?
diff --git a/kernel/time/timer.c b/kernel/time/timer.c index 2aca1a7edf6c..a47bc6f294b3 100644 --- a/kernel/time/timer.c +++ b/kernel/time/timer.c @@ -1208,12 +1208,19 @@ EXPORT_SYMBOL_GPL(add_timer_on); /** * __timer_delete - Internal function: Deactivate a timer * @timer: The timer to be deactivated + * @shutdown: If true, this indicates that the timer is about to be + * shutdown permanently. + * + * If @shutdown is true then @timer->function is set to NULL under the + * timer base lock which prevents further rearming of the time. In that + * case any attempt to rearm @timer after this function returns will be + * silently ignored. * * Return: * * %0 - The timer was not pending * * %1 - The timer was pending and deactivated */ -static int __timer_delete(struct timer_list *timer) +static int __timer_delete(struct timer_list *timer, bool shutdown) { struct timer_base *base; unsigned long flags; @@ -1221,9 +1228,22 @@ static int __timer_delete(struct timer_list *timer) debug_assert_init(timer); - if (timer_pending(timer)) { + /* + * If @shutdown is set then the lock has to be taken whether the + * timer is pending or not to protect against a concurrent rearm + * which might hit between the lockless pending check and the lock + * aquisition. By taking the lock it is ensured that such a newly + * enqueued timer is dequeued and cannot end up with + * timer->function == NULL in the expiry code. + * + * If timer->function is currently executed, then this makes sure + * that the callback cannot requeue the timer. + */ + if (timer_pending(timer) || shutdown) { base = lock_timer_base(timer, &flags); ret = detach_if_pending(timer, base, true); + if (shutdown) + timer->function = NULL; raw_spin_unlock_irqrestore(&base->lock, flags); } @@ -1246,20 +1266,31 @@ static int __timer_delete(struct timer_list *timer) */ int timer_delete(struct timer_list *timer) { - return __timer_delete(timer); + return __timer_delete(timer, false); } EXPORT_SYMBOL(timer_delete); /** * __try_to_del_timer_sync - Internal function: Try to deactivate a timer * @timer: Timer to deactivate + * @shutdown: If true, this indicates that the timer is about to be + * shutdown permanently. + * + * If @shutdown is true then @timer->function is set to NULL under the + * timer base lock which prevents further rearming of the timer. Any + * attempt to rearm @timer after this function returns will be silently + * ignored. + * + * This function cannot guarantee that the timer cannot be rearmed + * right after dropping the base lock if @shutdown is false. That + * needs to be prevented by the calling code if necessary. * * Return: * * %0 - The timer was not pending * * %1 - The timer was pending and deactivated * * %-1 - The timer callback function is running on a different CPU */ -static int __try_to_del_timer_sync(struct timer_list *timer) +static int __try_to_del_timer_sync(struct timer_list *timer, bool shutdown) { struct timer_base *base; unsigned long flags; @@ -1271,6 +1302,8 @@ static int __try_to_del_timer_sync(struct timer_list *timer) if (base->running_timer != timer) ret = detach_if_pending(timer, base, true); + if (shutdown) + timer->function = NULL; raw_spin_unlock_irqrestore(&base->lock, flags); @@ -1295,7 +1328,7 @@ static int __try_to_del_timer_sync(struct timer_list *timer) */ int try_to_del_timer_sync(struct timer_list *timer) { - return __try_to_del_timer_sync(timer); + return __try_to_del_timer_sync(timer, false); } EXPORT_SYMBOL(try_to_del_timer_sync); @@ -1376,12 +1409,25 @@ static inline void del_timer_wait_running(struct timer_list *timer) { } * __timer_delete_sync - Internal function: Deactivate a timer and wait * for the handler to finish. * @timer: The timer to be deactivated + * @shutdown: If true, @timer->function will be set to NULL under the + * timer base lock which prevents rearming of @timer + * + * If @shutdown is not set the timer can be rearmed later. If the timer can + * be rearmed concurrently, i.e. after dropping the base lock then the + * return value is meaningless. + * + * If @shutdown is set then @timer->function is set to NULL under timer + * base lock which prevents rearming of the timer. Any attempt to rearm + * a shutdown timer is silently ignored. + * + * If the timer should be reused after shutdown it has to be initialized + * again. * * Return: * * %0 - The timer was not pending * * %1 - The timer was pending and deactivated */ -static int __timer_delete_sync(struct timer_list *timer) +static int __timer_delete_sync(struct timer_list *timer, bool shutdown) { int ret; @@ -1411,7 +1457,7 @@ static int __timer_delete_sync(struct timer_list *timer) lockdep_assert_preemption_enabled(); do { - ret = __try_to_del_timer_sync(timer); + ret = __try_to_del_timer_sync(timer, shutdown); if (unlikely(ret < 0)) { del_timer_wait_running(timer); @@ -1463,7 +1509,7 @@ static int __timer_delete_sync(struct timer_list *timer) */ int timer_delete_sync(struct timer_list *timer) { - return __timer_delete_sync(timer); + return __timer_delete_sync(timer, false); } EXPORT_SYMBOL(timer_delete_sync);