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/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
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
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
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
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 <[email protected]>
> ---
> 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);
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