Re: [syzbot] [keyrings?] [lsm?] WARNING in __mod_timer

[Hillf Danton <hdanton@sina.com>]

On Mon, 27 Feb 2023 21:33:03 +0100 Thomas Gleixner <tglx@linutronix.de>
> On Sun, Feb 26 2023 at 19:55, syzbot wrote:
> > ODEBUG: assert_init not available (active state 0) object: ffffffff8d4fcbc0 object type: timer_list hint: key_gc_timer_func+0x0/0x80 security/keys/gc.c:117
> 
> >  WARNING: CPU: 1 PID: 10646 at lib/debugobjects.c:512 debug_object_assert_init+0x1f2/0x240 lib/debugobjects.c:899
> >  debug_assert_init kernel/time/timer.c:837 [inline]
> >  __mod_timer+0x10d/0xf40 kernel/time/timer.c:1020
> >  key_reject_and_link+0x3f5/0x6e0 security/keys/key.c:610
> >  key_negate_and_link include/linux/key-type.h:187 [inline]
> >  complete_request_key security/keys/request_key.c:64 [inline]
> >  call_sbin_request_key+0xa7b/0xcd0 security/keys/request_key.c:213
> >  construct_key security/keys/request_key.c:244 [inline]
> >  construct_key_and_link security/keys/request_key.c:503 [inline]
> >  request_key_and_link+0x11e3/0x18e0 security/keys/request_key.c:637
> >  __do_sys_request_key security/keys/keyctl.c:222 [inline]
> >  __se_sys_request_key+0x271/0x3b0 security/keys/keyctl.c:167
> 
> This is odd. The timer object is statically allocated via
> DEFINE_TIMER(). That macro sets
> 
>        timer.entry.next = TIMER_ENTRY_STATIC
> 
> which is used to detect statically allocated timer objects via
> timer_is_static_object() and that checks for:
> 
>      timer.entry.pprev == NULL && timer.entry.next == TIMER_ENTRY_STATIC

List operations like hlist_add_head() and __hlist_del() make
timer_is_static_object() return false positive result.

> 
> The only function which touches key_gc_timer is
> 
>     key_reject_and_link()
>       mod_timer()
>         __mod_timer()
>           debug_assert_init()

Commit d02e382cef06 ("timers: Silently ignore timers with a NULL function")
added this assert.

>             debug_timer_assert_init()
>               debug_object_assert_init()
>                 if (!lookup_object()) {
>                    if (!check_for_static_object()) <- Invokes timer_is_static_object()
>                       WARN()


	cpu 0				cpu 2
	---				---
	mod_timer()			mod_timer()
	__mod_timer()			__mod_timer()
	...				...
	debug_object_assert_init()
	raw_spin_lock_irqsave(&db->lock, flags);
	if (!lookup_object()) {
		raw_spin_unlock_irqrestore(&db->lock, flags);

					raw_spin_lock_irqsave(&db->lock, flags);
					if (!lookup_object()) {
						raw_spin_unlock_irqrestore(&db->lock, flags);

		if (check_for_static_object())
			fine;
						if (!check_for_static_object())
							WARN;


Depending on TIMER_ENTRY_STATIC makes check_for_static_object() fragile as a
static timer is always static regardless if it is enqueued.

The fragility is another explanation.

> 
> If this is the first invocation of mod_timer(&key_gc_timer,...) then
> key_gc_timer is corrupted.
> 
> If this is not the first invocation of mod_timer(&key_gc_timer,...) then
> the debugobjects hash is corrupted.
> 
> Either way neither the timer code nor debugobjects have been changed
> since the 6.2 release and certainly are innocent here.
> 
> That smells like a nasty memory corruption issue and the two other
> syzbot reports which arrived in my filtered inbox:
> 
>  https://lore.kernel.org/all/000000000000d7894b05f5924787@google.com
>  https://lore.kernel.org/all/000000000000840dae05f5a7fb53@google.com
> 
> point to memory corruption as well.
> 
> The first one has a C reproducer. Can that be used for bisection?
> 
> Thanks,
> 
>         tglx
>