Hello.
Thanks for this submission. Please see one comment inline.
On středa 20. dubna 2022 13:52:57 CEST Aaron Tomlin wrote:
> Currently, only the initial module that tainted the kernel is
> recorded e.g. when an out-of-tree module is loaded.
>
> The purpose of this patch is to allow the kernel to maintain a record of
> each unloaded module that taints the kernel. So, in addition to
> displaying a list of linked modules (see print_modules()) e.g. in the
> event of a detected bad page, unloaded modules that carried a taint/or
> taints are displayed too. A tainted module unload count is maintained.
>
> The number of tracked modules is not fixed. This feature is disabled by
> default.
>
> Signed-off-by: Aaron Tomlin <atomlin@xxxxxxxxxx>
> ---
> init/Kconfig | 11 ++++++++
> kernel/module/main.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 76 insertions(+)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index ddcbefe535e9..6b30210f787d 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2118,6 +2118,17 @@ config MODULE_FORCE_UNLOAD
> rmmod). This is mainly for kernel developers and desperate users.
> If unsure, say N.
>
> +config MODULE_UNLOAD_TAINT_TRACKING
> + bool "Tainted module unload tracking"
> + depends on MODULE_UNLOAD
> + default n
> + help
> + This option allows you to maintain a record of each unloaded
> + module that tainted the kernel. In addition to displaying a
> + list of linked (or loaded) modules e.g. on detection of a bad
> + page (see bad_page()), the aforementioned details are also
> + shown. If unsure, say N.
> +
> config MODVERSIONS
> bool "Module versioning support"
> help
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index ea78cec316dd..d7cc64172dd1 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -68,6 +68,16 @@
> */
> DEFINE_MUTEX(module_mutex);
> LIST_HEAD(modules);
> +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
> +static LIST_HEAD(unloaded_tainted_modules);
> +
> +struct mod_unload_taint {
> + struct list_head list;
> + char name[MODULE_NAME_LEN];
> + unsigned long taints;
> + u64 count;
> +};
> +#endif
>
> /* Work queue for freeing init sections in success case */
> static void do_free_init(struct work_struct *w);
> @@ -150,6 +160,41 @@ int unregister_module_notifier(struct notifier_block *nb)
> }
> EXPORT_SYMBOL(unregister_module_notifier);
>
> +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
> +static int try_add_tainted_module(struct module *mod)
> +{
> + struct mod_unload_taint *mod_taint;
> +
> + module_assert_mutex_or_preempt();
> +
> + list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules, list,
> + lockdep_is_held(&module_mutex)) {
> + size_t len = strlen(mod_taint->name);
> +
> + if (len == strlen(mod->name) && !memcmp(mod_taint->name, mod->name, len) &&
Here, two strings are compared, so I'd expect to see `strncmp()` instead of `memcmp()`.
> + mod_taint->taints & mod->taints) {
> + mod_taint->count++;
> + goto out;
> + }
> + }
> +
> + mod_taint = kmalloc(sizeof(*mod_taint), GFP_KERNEL);
> + if (unlikely(!mod_taint))
> + return -ENOMEM;
> + strscpy(mod_taint->name, mod->name, MODULE_NAME_LEN);
> + mod_taint->taints = mod->taints;
> + list_add_rcu(&mod_taint->list, &unloaded_tainted_modules);
> + mod_taint->count = 1;
> +out:
> + return 0;
> +}
> +#else /* MODULE_UNLOAD_TAINT_TRACKING */
> +static int try_add_tainted_module(struct module *mod)
> +{
> + return 0;
> +}
> +#endif
> +
> /*
> * We require a truly strong try_module_get(): 0 means success.
> * Otherwise an error is returned due to ongoing or failed
> @@ -1201,6 +1246,9 @@ static void free_module(struct module *mod)
> module_bug_cleanup(mod);
> /* Wait for RCU-sched synchronizing before releasing mod->list and buglist. */
> synchronize_rcu();
> + if (try_add_tainted_module(mod))
> + pr_err("%s: adding tainted module to the unloaded tainted modules list failed.\n",
> + mod->name);
> mutex_unlock(&module_mutex);
>
> /* Clean up CFI for the module. */
> @@ -3126,6 +3174,9 @@ struct module *__module_text_address(unsigned long addr)
> void print_modules(void)
> {
> struct module *mod;
> +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
> + struct mod_unload_taint *mod_taint;
> +#endif
> char buf[MODULE_FLAGS_BUF_SIZE];
>
> printk(KERN_DEFAULT "Modules linked in:");
> @@ -3136,6 +3187,20 @@ void print_modules(void)
> continue;
> pr_cont(" %s%s", mod->name, module_flags(mod, buf));
> }
> +#ifdef CONFIG_MODULE_UNLOAD_TAINT_TRACKING
> + if (!list_empty(&unloaded_tainted_modules)) {
> + printk(KERN_DEFAULT "Unloaded tainted modules:");
> + list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules,
> + list) {
> + size_t l;
> +
> + l = module_flags_taint(mod_taint->taints, buf);
> + buf[l++] = '\0';
> + pr_cont(" %s(%s):%llu", mod_taint->name, buf,
> + mod_taint->count);
> + }
> +#endif
> + }
> preempt_enable();
> if (last_unloaded_module[0])
> pr_cont(" [last unloaded: %s]", last_unloaded_module);
>
--
Oleksandr Natalenko (post-factum)
