NACK: [SRU][F 0/9][J 0/1][PATCH v2] CVE-2024-35887

[Magali Lemes]

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(-)
>