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@xxxxxxx>
> Suggested-by: Michal Hocko <mhocko@xxxxxxxx>
> Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> ---
> 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
