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:27:57 +0200
From:   Michal Hocko <mhocko@suse.com>
To:     Jean Delvare <jdelvare@suse.de>
Cc:     Luis Chamberlain <mcgrof@kernel.org>,
        linux-modules@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] module: print module name on refcount error
Message-ID: <ZLVd/dsNnxoorIGG@dhcp22.suse.cz>
References: <20230717171428.1b229215@endymion.delvare>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230717171428.1b229215@endymion.delvare>
Precedence: bulk

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

-- 
Michal Hocko
SUSE Labs