linux-kernel - Re: [PATCH v2] module: print module name on refcount error

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] [day] [month] [year] [list]

Message-ID: <ZLVd/dsNnxoorIGG@dhcp22.suse.cz>
Date:   Mon, 17 Jul 2023 17:27:57 +0200
From:   Michal Hocko <mhocko@...e.com>
To:     Jean Delvare <jdelvare@...e.de>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        linux-modules@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] module: print module name on refcount error

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@...e.de>
> Suggested-by: Michal Hocko <mhocko@...e.com>
> Cc: Luis Chamberlain <mcgrof@...nel.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

-- 
Michal Hocko
SUSE Labs