* [PATCH v4 0/2] module: Introduce module unload taint tracking @ 2022-04-25 9:08 Aaron Tomlin 2022-04-25 9:08 ` [PATCH v4 1/2] module: Make module_flags_taint() accept a module's taints bitmap directly Aaron Tomlin ` (2 more replies) 0 siblings, 3 replies; 9+ messages in thread From: Aaron Tomlin @ 2022-04-25 9:08 UTC (permalink / raw) To: mcgrof Cc: cl, pmladek, mbenes, christophe.leroy, akpm, linux-kernel, linux-modules, atomlin, ghalat, oleksandr, neelx Hi Luis, This iteration is still based on the latest mcgrof/modules-next branch. I have decided still to use RCU even though no entry is ever removed from the unloaded tainted modules list. That being said, if I understand correctly, it is not safe in some instances to use 'module_mutex' in print_modules(). So instead we disable preemption to ensure list traversal with concurrent list manipulation e.g. list_add_rcu(), is safe too. Changes since v3 [1] - Fixed kernel build error reported by kernel test robot i.e. moved '#endif' outside 'if (!list_empty(&unloaded_tainted_modules))' statement in the context of print_modules() - Used strncmp() instead of memcmp() (Oleksandr Natalenko) - Removed the additional strlen() (Christoph Lameter) Changes since v2 [2] - Dropped RFC from subject - Removed the newline i.e. "\n" in printk() - Always include the tainted module's unload count - Unconditionally display each unloaded tainted module Please let me know your thoughts. [1]: https://lore.kernel.org/all/20220420115257.3498300-1-atomlin@redhat.com/ [2]: https://lore.kernel.org/all/20220419150334.3395019-1-atomlin@redhat.com/ Aaron Tomlin (2): module: Make module_flags_taint() accept a module's taints bitmap directly module: Introduce module unload taint tracking init/Kconfig | 11 +++++++ kernel/module/main.c | 73 +++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 80 insertions(+), 4 deletions(-) base-commit: eeaec7801c421e17edda6e45a32d4a5596b633da -- 2.34.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4 1/2] module: Make module_flags_taint() accept a module's taints bitmap directly 2022-04-25 9:08 [PATCH v4 0/2] module: Introduce module unload taint tracking Aaron Tomlin @ 2022-04-25 9:08 ` Aaron Tomlin 2022-04-25 9:08 ` [PATCH v4 2/2] module: Introduce module unload taint tracking Aaron Tomlin 2022-04-25 23:31 ` [PATCH v4 0/2] " Luis Chamberlain 2 siblings, 0 replies; 9+ messages in thread From: Aaron Tomlin @ 2022-04-25 9:08 UTC (permalink / raw) To: mcgrof Cc: cl, pmladek, mbenes, christophe.leroy, akpm, linux-kernel, linux-modules, atomlin, ghalat, oleksandr, neelx No functional change. The purpose of this patch is to modify module_flags_taint() to accept a module's taints bitmap as a parameter and modifies all users accordingly. This is in preparation for module unload taint tracking support. Signed-off-by: Aaron Tomlin <atomlin@redhat.com> --- kernel/module/main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/kernel/module/main.c b/kernel/module/main.c index 05a42d8fcd7a..ea78cec316dd 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -890,13 +890,13 @@ static inline int module_unload_init(struct module *mod) } #endif /* CONFIG_MODULE_UNLOAD */ -static size_t module_flags_taint(struct module *mod, char *buf) +static size_t module_flags_taint(unsigned long taints, char *buf) { size_t l = 0; int i; for (i = 0; i < TAINT_FLAGS_COUNT; i++) { - if (taint_flags[i].module && test_bit(i, &mod->taints)) + if (taint_flags[i].module && test_bit(i, &taints)) buf[l++] = taint_flags[i].c_true; } @@ -974,7 +974,7 @@ static ssize_t show_taint(struct module_attribute *mattr, { size_t l; - l = module_flags_taint(mk->mod, buffer); + l = module_flags_taint(mk->mod->taints, buffer); buffer[l++] = '\n'; return l; } @@ -2993,7 +2993,7 @@ char *module_flags(struct module *mod, char *buf) mod->state == MODULE_STATE_GOING || mod->state == MODULE_STATE_COMING) { buf[bx++] = '('; - bx += module_flags_taint(mod, buf + bx); + bx += module_flags_taint(mod->taints, buf + bx); /* Show a - for module-is-being-unloaded */ if (mod->state == MODULE_STATE_GOING) buf[bx++] = '-'; -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v4 2/2] module: Introduce module unload taint tracking 2022-04-25 9:08 [PATCH v4 0/2] module: Introduce module unload taint tracking Aaron Tomlin 2022-04-25 9:08 ` [PATCH v4 1/2] module: Make module_flags_taint() accept a module's taints bitmap directly Aaron Tomlin @ 2022-04-25 9:08 ` Aaron Tomlin 2022-05-02 11:07 ` Christophe Leroy 2022-04-25 23:31 ` [PATCH v4 0/2] " Luis Chamberlain 2 siblings, 1 reply; 9+ messages in thread From: Aaron Tomlin @ 2022-04-25 9:08 UTC (permalink / raw) To: mcgrof Cc: cl, pmladek, mbenes, christophe.leroy, akpm, linux-kernel, linux-modules, atomlin, ghalat, oleksandr, neelx 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@redhat.com> --- 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..35686e63b32f 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 (!strncmp(mod_taint->name, mod->name, len) && + 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); -- 2.34.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v4 2/2] module: Introduce module unload taint tracking 2022-04-25 9:08 ` [PATCH v4 2/2] module: Introduce module unload taint tracking Aaron Tomlin @ 2022-05-02 11:07 ` Christophe Leroy 2022-05-02 18:45 ` Aaron Tomlin 0 siblings, 1 reply; 9+ messages in thread From: Christophe Leroy @ 2022-05-02 11:07 UTC (permalink / raw) To: Aaron Tomlin, mcgrof Cc: cl, pmladek, mbenes, akpm, linux-kernel, linux-modules, atomlin, ghalat, oleksandr, neelx Le 25/04/2022 à 11:08, Aaron Tomlin a écrit : > 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@redhat.com> > --- > 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..35686e63b32f 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); Why do you need that strlen() at all, can't you just use strcmp() ? With strncmp() what happens if for instance mod_taint->name is "dead" and mod->name is "deadbeef" ? > + > + if (!strncmp(mod_taint->name, mod->name, len) && > + 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 2/2] module: Introduce module unload taint tracking 2022-05-02 11:07 ` Christophe Leroy @ 2022-05-02 18:45 ` Aaron Tomlin 0 siblings, 0 replies; 9+ messages in thread From: Aaron Tomlin @ 2022-05-02 18:45 UTC (permalink / raw) To: Christophe Leroy Cc: mcgrof, cl, pmladek, mbenes, akpm, linux-kernel, linux-modules, atomlin, ghalat, oleksandr, neelx On Mon 2022-05-02 11:07 +0000, Christophe Leroy wrote: > > @@ -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); > > Why do you need that strlen() at all, can't you just use strcmp() ? > With strncmp() what happens if for instance mod_taint->name is "dead" > and mod->name is "deadbeef" ? Hi Christophe, Thanks for your feedback. I see that. Furthermore, the length of a module's name is fixed. Hence strcmp() should be fine. Kind regards, -- Aaron Tomlin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 0/2] module: Introduce module unload taint tracking 2022-04-25 9:08 [PATCH v4 0/2] module: Introduce module unload taint tracking Aaron Tomlin 2022-04-25 9:08 ` [PATCH v4 1/2] module: Make module_flags_taint() accept a module's taints bitmap directly Aaron Tomlin 2022-04-25 9:08 ` [PATCH v4 2/2] module: Introduce module unload taint tracking Aaron Tomlin @ 2022-04-25 23:31 ` Luis Chamberlain 2022-04-26 8:39 ` Aaron Tomlin 2 siblings, 1 reply; 9+ messages in thread From: Luis Chamberlain @ 2022-04-25 23:31 UTC (permalink / raw) To: Aaron Tomlin Cc: cl, pmladek, mbenes, christophe.leroy, akpm, linux-kernel, linux-modules, atomlin, ghalat, oleksandr, neelx On Mon, Apr 25, 2022 at 10:08:39AM +0100, Aaron Tomlin wrote: > Hi Luis, > > This iteration is still based on the latest mcgrof/modules-next branch. > > I have decided still to use RCU even though no entry is ever removed from > the unloaded tainted modules list. That being said, if I understand > correctly, it is not safe in some instances to use 'module_mutex' in > print_modules(). So instead we disable preemption to ensure list traversal > with concurrent list manipulation e.g. list_add_rcu(), is safe too. > > Changes since v3 [1] > - Fixed kernel build error reported by kernel test robot i.e. moved > '#endif' outside 'if (!list_empty(&unloaded_tainted_modules))' > statement in the context of print_modules() > - Used strncmp() instead of memcmp() > (Oleksandr Natalenko) > - Removed the additional strlen() > (Christoph Lameter) > > Changes since v2 [2] > - Dropped RFC from subject > - Removed the newline i.e. "\n" in printk() > - Always include the tainted module's unload count > - Unconditionally display each unloaded tainted module > > Please let me know your thoughts. This all looks good except with all the work you did to remove #ifdef hell, it gets me wondering why not just use a new file for this? What does that look like? Luis ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 0/2] module: Introduce module unload taint tracking 2022-04-25 23:31 ` [PATCH v4 0/2] " Luis Chamberlain @ 2022-04-26 8:39 ` Aaron Tomlin 2022-04-26 16:22 ` Luis Chamberlain 0 siblings, 1 reply; 9+ messages in thread From: Aaron Tomlin @ 2022-04-26 8:39 UTC (permalink / raw) To: Luis Chamberlain, k Cc: cl, pmladek, mbenes, christophe.leroy, akpm, linux-kernel, linux-modules, atomlin, ghalat, oleksandr, neelx On Mon 2022-04-25 16:31 -0700, Luis Chamberlain wrote: > This all looks good except with all the work you did to remove > #ifdef hell, it gets me wondering why not just use a new file for this? > > What does that look like? Hi Luis, I thought about it. It is indeed possible. Yet, I do not think it is worth it, for such a small change; albeit, what do you prefer? Kind regards, -- Aaron Tomlin ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 0/2] module: Introduce module unload taint tracking 2022-04-26 8:39 ` Aaron Tomlin @ 2022-04-26 16:22 ` Luis Chamberlain 2022-04-27 9:07 ` Aaron Tomlin 0 siblings, 1 reply; 9+ messages in thread From: Luis Chamberlain @ 2022-04-26 16:22 UTC (permalink / raw) To: Aaron Tomlin Cc: k, cl, pmladek, mbenes, christophe.leroy, akpm, linux-kernel, linux-modules, atomlin, ghalat, oleksandr, neelx On Tue, Apr 26, 2022 at 09:39:30AM +0100, Aaron Tomlin wrote: > On Mon 2022-04-25 16:31 -0700, Luis Chamberlain wrote: > > This all looks good except with all the work you did to remove > > #ifdef hell, it gets me wondering why not just use a new file for this? > > > > What does that look like? > > Hi Luis, > > I thought about it. It is indeed possible. Yet, I do not think it is worth > it, for such a small change; albeit, what do you prefer? I'd rather see the effort than not, given all the effort to already split things. I think it keeps things pretty tidy and it can scale / and its easier to review. Luis ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4 0/2] module: Introduce module unload taint tracking 2022-04-26 16:22 ` Luis Chamberlain @ 2022-04-27 9:07 ` Aaron Tomlin 0 siblings, 0 replies; 9+ messages in thread From: Aaron Tomlin @ 2022-04-27 9:07 UTC (permalink / raw) To: Luis Chamberlain Cc: k, cl, pmladek, mbenes, christophe.leroy, akpm, linux-kernel, linux-modules, atomlin, ghalat, oleksandr, neelx On Tue 2022-04-26 09:22 -0700, Luis Chamberlain wrote: > I'd rather see the effort than not, given all the effort to already split things. > I think it keeps things pretty tidy and it can scale / and its easier to review. Fair enough. I'll create another iteration of the series. -- Aaron Tomlin ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-02 18:45 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-25 9:08 [PATCH v4 0/2] module: Introduce module unload taint tracking Aaron Tomlin 2022-04-25 9:08 ` [PATCH v4 1/2] module: Make module_flags_taint() accept a module's taints bitmap directly Aaron Tomlin 2022-04-25 9:08 ` [PATCH v4 2/2] module: Introduce module unload taint tracking Aaron Tomlin 2022-05-02 11:07 ` Christophe Leroy 2022-05-02 18:45 ` Aaron Tomlin 2022-04-25 23:31 ` [PATCH v4 0/2] " Luis Chamberlain 2022-04-26 8:39 ` Aaron Tomlin 2022-04-26 16:22 ` Luis Chamberlain 2022-04-27 9:07 ` Aaron Tomlin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.