Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 17 Jul 2023 17:14:28 +0200
From:   Jean Delvare <jdelvare@suse.de>
To:     Luis Chamberlain <mcgrof@kernel.org>
Cc:     Michal Hocko <mhocko@suse.com>, linux-modules@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: [PATCH v2] module: print module name on refcount error
Message-ID: <20230717171428.1b229215@endymion.delvare>
Organization: SUSE Linux
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

If module_put() triggers a refcount error, and the module data is
still readable, include the culprit module name in the warning
message, to easy further investigation of the issue.

If the module name can't be read, this means the module has already
been removed while references to it still exist. This is a
user-after-free situation, so report it as such.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Suggested-by: Michal Hocko <mhocko@suse.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
---
Hi Luis, this is a different approach to my initial proposal. We no
longer assume that struct module is still available and instead check
that the expected module name string is a valid string before printing
it.

This is safer, and lets us print a better diagnostics message: include
the module name if struct module is still there (the most likely case
IMHO, as rmmod is a relatively rare operation) else explicitly report a
use after free.

The downside is that this requires more code, but I think it's worth
it. What do you think?

 kernel/module/main.c |   31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

--- linux-6.3.orig/kernel/module/main.c
+++ linux-6.3/kernel/module/main.c
@@ -55,6 +55,7 @@
 #include <linux/dynamic_debug.h>
 #include <linux/audit.h>
 #include <linux/cfi.h>
+#include <linux/ctype.h>
 #include <uapi/linux/module.h>
 #include "internal.h"
 
@@ -850,7 +851,35 @@ void module_put(struct module *module)
 	if (module) {
 		preempt_disable();
 		ret = atomic_dec_if_positive(&module->refcnt);
-		WARN_ON(ret < 0);	/* Failed to put refcount */
+		if (ret < 0) {
+			unsigned char modname_copy[MODULE_NAME_LEN];
+			unsigned char *p, *end;
+			bool sane;
+
+			/*
+			 * Report faulty module if name is still readable.
+			 * We must be careful here as the module may have
+			 * been already freed.
+			 */
+			memcpy(modname_copy, module->name, MODULE_NAME_LEN);
+			end = memchr(modname_copy, '\0', MODULE_NAME_LEN);
+			sane = end != NULL;
+			if (sane) {
+				for (p = modname_copy; p < end; p++)
+					if (!isgraph(*p)) {
+						sane = false;
+						break;
+					}
+			}
+
+			if (sane)
+				WARN(1,
+				     KERN_WARNING "Failed to put refcount for module %s\n",
+				     modname_copy);
+			else
+				WARN(1,
+				     KERN_WARNING "Failed to put refcount, use-after-free detected\n");
+		}
 		trace_module_put(module, _RET_IP_);
 		preempt_enable();
 	}


-- 
Jean Delvare
SUSE L3 Support