| Message ID | 20230717171428.1b229215@endymion.delvare (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] module: print module name on refcount error | expand |
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
--- 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(); }
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(-)