diff mbox series

[v2] module: print module name on refcount error

Message ID 20230717171428.1b229215@endymion.delvare
State New
Headers
Series [v2] module: print module name on refcount error |

Commit Message

Jean Delvare July 17, 2023, 3:14 p.m. UTC 
  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(-)

Comments

Michal Hocko July 17, 2023, 3:27 p.m. UTC | #1 
On Mon 17-07-23 17:14:28, Jean Delvare wrote:
> 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?

Quite honestly, I do not think that extra ode is worth the risk. If the
module could have been removed then we are in a bigger problem and
trying to do some cosmetic checks doesn't help all that much IMHO. It is
good idea to cap the name to MODULE_NAME_LEN to be bound on a garbage
output.

> 
>  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
diff mbox series

Patch

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