| Message ID | 20241114203112.57228-1-massimiliano.pellizzer@canonical.com |
|---|---|
| Headers | show |
| Series | CVE-2024-35887 | expand |
On 14/11/2024 17:31, Massimiliano Pellizzer wrote: > [Impact] > > ax25: fix use-after-free bugs caused by ax25_ds_del_timer > > When the ax25 device is detaching, the ax25_dev_device_down() > calls ax25_ds_del_timer() to cleanup the slave_timer. When > the timer handler is running, the ax25_ds_del_timer() that > calls del_timer() in it will return directly. As a result, > the use-after-free bugs could happen. > > In order to mitigate bugs, when the device is detaching, use > timer_shutdown_sync() to stop the timer. > > [Fix] > > Noble: Fixed > Jammy: Cherry picked from mainline > > Focal: > - Clean cherry pick of 8fd8ad5c5dfc (mainline): cherry picked since > it provides the definition of lockdep_assert_preemption_enabled() > - Clean cherry pick of c725dafc95f1 (mainline): cherry picked since it > is a prereq for 8553b5f2774a > - Backport of bb663f0f3c39 (mainline): backported since it is a prereq > for 8553b5f2774a > - Backport of 8553b5f2774a (mainline): backported since it is a prereq > for 0cc04e80458a > - Clean cherry pick of 0cc04e80458a (mainline): cherry picked since it > is a prereq for f571faf6e443 > - Clean cherry pick of 73737a5833ac (mainline): cherry picked since it > solves a compile-time error caused by f571faf6e443 > - Backport of 6e1fc2591f11 (mainline): backported since it solves a > compile-time error caused by f571faf6e443 > - Clean cherry pick of f571faf6e443 (mainline): cherry picked since it > is a prereq for the fix commit, in particular it defines > timer_shutdown_sync() > - Cherry pick of the fix commit from mainline. > > Bionic: Work in progress > Xenial: Work in progress > > [Test Case] > > Compile and boot tested. > Since the patch set significantly modifies the "timers" subsystem I also > used kselftest with target "timers" to make sure the patch set does not > introduce any regression. > > [Where problems could occur] > > The fix affects the net/ax25, and (for focal) the core timer subsystem. > In Jammy a regression is not likely. > In Focal, since the core timer subsystem has been modified > significantly, the entire kernel could be impacted. This could lead to > widespread timer failures, causing system instability and kernel > crashes. > > [Note] > > The fix for the CVE uses the function timer_shutdown_sync(), which is > safe to use in pretty much every context. > This function is not implemented in Focal. The closest function to > timer_shutdown_sync(), in Focal, is timer_delete_sync() which has strict > requirements: > > Synchronization rules: Callers must prevent restarting of the timer, > otherwise this function is meaningless. It must not be called from > interrupt contexts unless the timer is an irqsafe one. The caller must > not hold locks which would prevent completion of the timer's callback > function. The timer's handler must not call add_timer_on(). Upon exit > the timer is not queued and the handler is not running on any CPU. > > For !irqsafe timers, the caller must not hold locks that are held in > interrupt context. > > and does not implement the shutdown logic implemented by > timer_shutdown_sync(). > For these reasons I decided to backport also patches related to timers. > > [Changes between v1 and v2] > > Removed the following superflows change in [F][PATCH 4/9]: > > - WARN_ON(in_irq() && !(timer->flags & TIMER_IRQSAFE)); > + WARN_ON(hardirq_count() && !(timer->flags & TIMER_IRQSAFE)); > > as in_irq() is defined as: > > #define in_irq() (hardirq_count()) > > Ahmed S. Darwish (1): > lockdep: Add preemption enabled/disabled assertion APIs > > Duoming Zhou (1): > ax25: fix use-after-free bugs caused by ax25_ds_del_timer > > Sebastian Andrzej Siewior (1): > timers: Don't block on ->expiry_lock for TIMER_IRQSAFE timers > > Steven Rostedt (Google) (2): > clocksource/drivers/arm_arch_timer: Do not use timer namespace for > timer_shutdown() function > clocksource/drivers/sp804: Do not use timer namespace for > timer_shutdown() function > > Thomas Gleixner (4): > timers: Rename del_timer() to timer_delete() > timers: Split [try_to_]del_timer[_sync]() to prepare for shutdown mode > timers: Add shutdown mechanism to the internal functions > timers: Provide timer_shutdown[_sync]() > As Koichiro has said, it seems like we need d02e382cef06 ("timers: Silently ignore timers with a NULL function"). In addition to that, checking the original upstream submission[1], we might want to include 82ed6f7ef58f ("timers: Replace BUG_ON()s") too. With timer_shutdown_sync setting timer->function to NULL, these two additional commits seem necessary. [1] https://lore.kernel.org/all/20221123201306.823305113@linutronix.de/ > drivers/clocksource/arm_arch_timer.c | 12 +- > drivers/clocksource/timer-sp804.c | 6 +- > include/linux/lockdep.h | 19 ++ > include/linux/timer.h | 17 +- > kernel/time/timer.c | 254 ++++++++++++++++++++++----- > lib/Kconfig.debug | 1 + > net/ax25/ax25_dev.c | 2 +- > 7 files changed, 253 insertions(+), 58 deletions(-) >
[Impact] ax25: fix use-after-free bugs caused by ax25_ds_del_timer When the ax25 device is detaching, the ax25_dev_device_down() calls ax25_ds_del_timer() to cleanup the slave_timer. When the timer handler is running, the ax25_ds_del_timer() that calls del_timer() in it will return directly. As a result, the use-after-free bugs could happen. In order to mitigate bugs, when the device is detaching, use timer_shutdown_sync() to stop the timer. [Fix] Noble: Fixed Jammy: Cherry picked from mainline Focal: - Clean cherry pick of 8fd8ad5c5dfc (mainline): cherry picked since it provides the definition of lockdep_assert_preemption_enabled() - Clean cherry pick of c725dafc95f1 (mainline): cherry picked since it is a prereq for 8553b5f2774a - Backport of bb663f0f3c39 (mainline): backported since it is a prereq for 8553b5f2774a - Backport of 8553b5f2774a (mainline): backported since it is a prereq for 0cc04e80458a - Clean cherry pick of 0cc04e80458a (mainline): cherry picked since it is a prereq for f571faf6e443 - Clean cherry pick of 73737a5833ac (mainline): cherry picked since it solves a compile-time error caused by f571faf6e443 - Backport of 6e1fc2591f11 (mainline): backported since it solves a compile-time error caused by f571faf6e443 - Clean cherry pick of f571faf6e443 (mainline): cherry picked since it is a prereq for the fix commit, in particular it defines timer_shutdown_sync() - Cherry pick of the fix commit from mainline. Bionic: Work in progress Xenial: Work in progress [Test Case] Compile and boot tested. Since the patch set significantly modifies the "timers" subsystem I also used kselftest with target "timers" to make sure the patch set does not introduce any regression. [Where problems could occur] The fix affects the net/ax25, and (for focal) the core timer subsystem. In Jammy a regression is not likely. In Focal, since the core timer subsystem has been modified significantly, the entire kernel could be impacted. This could lead to widespread timer failures, causing system instability and kernel crashes. [Note] The fix for the CVE uses the function timer_shutdown_sync(), which is safe to use in pretty much every context. This function is not implemented in Focal. The closest function to timer_shutdown_sync(), in Focal, is timer_delete_sync() which has strict requirements: Synchronization rules: Callers must prevent restarting of the timer, otherwise this function is meaningless. It must not be called from interrupt contexts unless the timer is an irqsafe one. The caller must not hold locks which would prevent completion of the timer's callback function. The timer's handler must not call add_timer_on(). Upon exit the timer is not queued and the handler is not running on any CPU. For !irqsafe timers, the caller must not hold locks that are held in interrupt context. and does not implement the shutdown logic implemented by timer_shutdown_sync(). For these reasons I decided to backport also patches related to timers. [Changes between v1 and v2] Removed the following superflows change in [F][PATCH 4/9]: - WARN_ON(in_irq() && !(timer->flags & TIMER_IRQSAFE)); + WARN_ON(hardirq_count() && !(timer->flags & TIMER_IRQSAFE)); as in_irq() is defined as: #define in_irq() (hardirq_count()) Ahmed S. Darwish (1): lockdep: Add preemption enabled/disabled assertion APIs Duoming Zhou (1): ax25: fix use-after-free bugs caused by ax25_ds_del_timer Sebastian Andrzej Siewior (1): timers: Don't block on ->expiry_lock for TIMER_IRQSAFE timers Steven Rostedt (Google) (2): clocksource/drivers/arm_arch_timer: Do not use timer namespace for timer_shutdown() function clocksource/drivers/sp804: Do not use timer namespace for timer_shutdown() function Thomas Gleixner (4): timers: Rename del_timer() to timer_delete() timers: Split [try_to_]del_timer[_sync]() to prepare for shutdown mode timers: Add shutdown mechanism to the internal functions timers: Provide timer_shutdown[_sync]() drivers/clocksource/arm_arch_timer.c | 12 +- drivers/clocksource/timer-sp804.c | 6 +- include/linux/lockdep.h | 19 ++ include/linux/timer.h | 17 +- kernel/time/timer.c | 254 ++++++++++++++++++++++----- lib/Kconfig.debug | 1 + net/ax25/ax25_dev.c | 2 +- 7 files changed, 253 insertions(+), 58 deletions(-)