linux-kernel - Re: [Bug] WARNING in static_key_disable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240306105420.6a6bea2c@gandalf.local.home>
Date: Wed, 6 Mar 2024 10:54:20 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Sam Sun <samsun1006219@...il.com>
Cc: linux-kernel@...r.kernel.org, syzkaller@...glegroups.com,
 xrivendell7@...il.com, ardb@...nel.org, jbaron@...mai.com,
 jpoimboe@...nel.org, peterz@...radead.org, linux-mm@...ck.org,
 akpm@...ux-foundation.org
Subject: Re: [Bug] WARNING in static_key_disable_cpuslocked

On Tue, 5 Mar 2024 15:54:24 +0800
Sam Sun <samsun1006219@...il.com> wrote:

> We analyzed the cause of this bug. It seems that in function
> static_key_disable_cpuslocked(), there is a small racing window
> between two atomic_read(&key->enabled) in line 228 & 229, triggering
> the WARN_ON_ONCE macro. This might cause function returned without
> actually disabling the static_key "key". I am not sure if there is any
> other potential threat here.
> 
> I searched on web and found that there was a similar bug reported by
> syzbot several years ago
> (https://groups.google.com/g/syzkaller-bugs/c/_W3lgRCwlb4/m/TqzyQcPpAQAJ).
> At that time the fix was in the net instead of kernel/jump_label.c. So
> I checked the call stack and cc this email to memory management
> maintainers. Hope there is no confusion.
> 
> If you have any questions, please contact us.
> Reported by: Yue Sun <samsun1006219@...il.com>
> Reported by: xingwei lee <xrivendell7@...il.com>

Thanks for the report. I wonder if it simply needs to add the tests in the
locking? Like the patch below.

Because I could see:

	CPU 0				CPU 1
	-----				-----

key->enabled = 0
        
 static_key_enable_cpus_locked()
   jump_label_lock();
				  static_key_disable_cpus_locked()
				      if (key->enabled != 1) {

    key->enabled = 1;
				          WARN_ON(key->enabled != 0)

Now I guess the question is, why is something trying to disable something
that is not enabled? Is the above scenario OK? Or should the users of
static_key also prevent this?

-- Steve


diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index d9c822bbffb8..f154caf2a62b 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -194,12 +194,12 @@ void static_key_enable_cpuslocked(struct static_key *key)
 	STATIC_KEY_CHECK_USE(key);
 	lockdep_assert_cpus_held();
 
+	jump_label_lock();
 	if (atomic_read(&key->enabled) > 0) {
 		WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
-		return;
+		goto unlock;
 	}
 
-	jump_label_lock();
 	if (atomic_read(&key->enabled) == 0) {
 		atomic_set(&key->enabled, -1);
 		jump_label_update(key);
@@ -208,6 +208,7 @@ void static_key_enable_cpuslocked(struct static_key *key)
 		 */
 		atomic_set_release(&key->enabled, 1);
 	}
+unlock:
 	jump_label_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_enable_cpuslocked);
@@ -225,14 +226,15 @@ void static_key_disable_cpuslocked(struct static_key *key)
 	STATIC_KEY_CHECK_USE(key);
 	lockdep_assert_cpus_held();
 
+	jump_label_lock();
 	if (atomic_read(&key->enabled) != 1) {
 		WARN_ON_ONCE(atomic_read(&key->enabled) != 0);
-		return;
+		goto unlock;
 	}
 
-	jump_label_lock();
 	if (atomic_cmpxchg(&key->enabled, 1, 0))
 		jump_label_update(key);
+unlock:
 	jump_label_unlock();
 }
 EXPORT_SYMBOL_GPL(static_key_disable_cpuslocked);