LinuxLists.cc - Unloaded tainted modules list with tcrypt

2022-10-10 15:41:50

Subject: Unloaded tainted modules list with tcrypt

When repeatedly using the tcrypt module (which is designed to never
load successfully), the "Unloaded tainted modules" list grows forever:

Unloaded tainted modules: tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
tcrypt():1 tcrypt():1 padlock_aes():1 padlock_aes():1
isst_if_mbox_msr():1 acpi_cpufreq():1 pcc_cpufreq():1 isst_if_mbox_msr():1
acpi_cpufreq():1 pcc_cpufreq():1 acpi_cpufreq():1 acpi_cpufreq():1
pcc_cpufreq():1 isst_if_mbox_msr():1 acpi_cpufreq():1 acpi_cpufreq():1
pcc_cpufreq():1 isst_if_mbox_msr():1 acpi_cpufreq():1 acpi_cpufreq():1
isst_if_mbox_msr():1 isst_if_mbox_msr():1
acpi_cpufreq():1 pcc_cpufreq():1 acpi_cpufreq():1 isst_if_mbox_msr():1

Some other modules like acpi_cpufreq() are repeated too.

Maybe this check after the name comparison:
mod_taint->taints & mod->taints
should be:
mod_taint->taints == mod->taints

or shouldn't be there at all?

2022-10-10 22:40:47

by Aaron Tomlin

[permalink] [raw]

Subject: Re: Unloaded tainted modules list with tcrypt

On Mon 2022-10-10 15:12 +0000, Elliott, Robert (Servers) wrote:
> When repeatedly using the tcrypt module (which is designed to never
> load successfully), the "Unloaded tainted modules" list grows forever:
>
> Unloaded tainted modules: tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 padlock_aes():1 padlock_aes():1
> isst_if_mbox_msr():1 acpi_cpufreq():1 pcc_cpufreq():1 isst_if_mbox_msr():1
> acpi_cpufreq():1 pcc_cpufreq():1 acpi_cpufreq():1 acpi_cpufreq():1
> pcc_cpufreq():1 isst_if_mbox_msr():1 acpi_cpufreq():1 acpi_cpufreq():1
> pcc_cpufreq():1 isst_if_mbox_msr():1 acpi_cpufreq():1 acpi_cpufreq():1
> isst_if_mbox_msr():1 isst_if_mbox_msr():1
> acpi_cpufreq():1 pcc_cpufreq():1 acpi_cpufreq():1 isst_if_mbox_msr():1
>
> Some other modules like acpi_cpufreq() are repeated too.
>
> Maybe this check after the name comparison:
> mod_taint->taints & mod->taints
> should be:
> mod_taint->taints == mod->taints
>
> or shouldn't be there at all?
>

Hi Elliot,

Sorry about that.

This is already addressed in linux-next:

commit a0830747f4e6810876e5f5329ce941e258e13a22
Author: Aaron Tomlin <[email protected]>
Date: Fri Oct 7 14:32:44 2022 +0100

module: tracking: Keep a record of tainted unloaded modules only

This patch ensures that no module record/or entry is added to the
unloaded_tainted_modules list if it does not carry a taint.

Reported-by: Alexey Dobriyan <[email protected]>
Fixes: 99bd9956551b ("module: Introduce module unload taint tracking")

Signed-off-by: Aaron Tomlin <[email protected]>

diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
index a139e63b6f20..26d812e07615 100644
--- a/kernel/module/tracking.c
+++ b/kernel/module/tracking.c
@@ -22,6 +22,9 @@ int try_add_tainted_module(struct module *mod)

module_assert_mutex_or_preempt();

+ if (!mod->taints)
+ goto out;
+
list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules, list,
lockdep_is_held(&module_mutex)) {
if (!strcmp(mod_taint->name, mod->name) &&

Kind regards,

--
Aaron Tomlin

2022-10-10 22:41:06

by Aaron Tomlin

[permalink] [raw]

Subject: Re: Unloaded tainted modules list with tcrypt

2022-10-11 00:34:58

by Elliott, Robert (Servers)

[permalink] [raw]

Subject: RE: Unloaded tainted modules list with tcrypt

> -----Original Message-----
> From: Elliott, Robert (Servers) <[email protected]>
> Sent: Monday, October 10, 2022 10:12 AM
> To: Aaron Tomlin <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Unloaded tainted modules list with tcrypt
>
> When repeatedly using the tcrypt module (which is designed to never
> load successfully), the "Unloaded tainted modules" list grows forever:
>
> Unloaded tainted modules: tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1 tcrypt():1
> tcrypt():1 tcrypt():1 padlock_aes():1 padlock_aes():1
> isst_if_mbox_msr():1 acpi_cpufreq():1 pcc_cpufreq():1 isst_if_mbox_msr():1
> acpi_cpufreq():1 pcc_cpufreq():1 acpi_cpufreq():1 acpi_cpufreq():1
> pcc_cpufreq():1 isst_if_mbox_msr():1 acpi_cpufreq():1 acpi_cpufreq():1
> pcc_cpufreq():1 isst_if_mbox_msr():1 acpi_cpufreq():1 acpi_cpufreq():1
> isst_if_mbox_msr():1 isst_if_mbox_msr():1
> acpi_cpufreq():1 pcc_cpufreq():1 acpi_cpufreq():1 isst_if_mbox_msr():1
>
> Some other modules like acpi_cpufreq() are repeated too.
>
> Maybe this check after the name comparison:
> mod_taint->taints & mod->taints
> should be:
> mod_taint->taints == mod->taints
>
> or shouldn't be there at all?

Changing to == seems to work well - that shows incremented counts
rather than new entries:
Unloaded tainted modules: tcrypt():40 padlock_aes():1 isst_if_mbox_msr():56 pcc_cpufreq():56 acpi_cpufreq():112

2022-10-11 16:18:21

by Aaron Tomlin

[permalink] [raw]

Subject: Re: Unloaded tainted modules list with tcrypt

On Tue 2022-10-11 00:09 +0000, Elliott, Robert (Servers) wrote:
> Changing to == seems to work well - that shows incremented counts
> rather than new entries:
> Unloaded tainted modules: tcrypt():40 padlock_aes():1 isst_if_mbox_msr():56 pcc_cpufreq():56 acpi_cpufreq():112

Hi Elliott,

Please note, any module that does not carry a taint should _not_
be on the aforementioned list. See the following which is already
in Linus' tree:

commit 47cc75aa92837a9d3f15157d6272ff285585d75d
Author: Aaron Tomlin <[email protected]>
Date: Fri Oct 7 14:38:12 2022 +0100

module: tracking: Keep a record of tainted unloaded modules only

This ensures that no module record/or entry is added to the
unloaded_tainted_modules list if it does not carry a taint.

Reported-by: Alexey Dobriyan <[email protected]>
Fixes: 99bd9956551b ("module: Introduce module unload taint tracking")
Signed-off-by: Aaron Tomlin <[email protected]>
Acked-by: Luis Chamberlain <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>

diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
index a139e63b6f20..26d812e07615 100644
--- a/kernel/module/tracking.c
+++ b/kernel/module/tracking.c
@@ -22,6 +22,9 @@ int try_add_tainted_module(struct module *mod)

module_assert_mutex_or_preempt();

+ if (!mod->taints)
+ goto out;
+
list_for_each_entry_rcu(mod_taint, &unloaded_tainted_modules, list,
lockdep_is_held(&module_mutex)) {
if (!strcmp(mod_taint->name, mod->name) &&

--
Aaron Tomlin

2022-10-11 18:03:15

by Elliott, Robert (Servers)

[permalink] [raw]

Subject: RE: Unloaded tainted modules list with tcrypt

> -----Original Message-----
> From: Aaron Tomlin <[email protected]>
> Sent: Tuesday, October 11, 2022 10:51 AM
> To: Elliott, Robert (Servers) <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: Re: Unloaded tainted modules list with tcrypt
>
> On Tue 2022-10-11 00:09 +0000, Elliott, Robert (Servers) wrote:
> > Changing to == seems to work well - that shows incremented counts
> > rather than new entries:
> > Unloaded tainted modules: tcrypt():40 padlock_aes():1 isst_if_mbox_msr():56
> pcc_cpufreq():56 acpi_cpufreq():112
>
> Hi Elliott,
>
> Please note, any module that does not carry a taint should _not_
> be on the aforementioned list. See the following which is already
> in Linus' tree:
>
> commit 47cc75aa92837a9d3f15157d6272ff285585d75d
> Author: Aaron Tomlin <[email protected]>
> Date: Fri Oct 7 14:38:12 2022 +0100
>
> module: tracking: Keep a record of tainted unloaded modules only

Thanks. With that change, the list remains blank as tcrypt and the
others are never added.

The counts of failed loads are mildly interesting (e.g., cpufreq
attempts are apparently based on the number of CPU cores present), but
there are other ways to observe that.

> This ensures that no module record/or entry is added to the
> unloaded_tainted_modules list if it does not carry a taint.
>
> Reported-by: Alexey Dobriyan <[email protected]>
> Fixes: 99bd9956551b ("module: Introduce module unload taint tracking")
> Signed-off-by: Aaron Tomlin <[email protected]>
> Acked-by: Luis Chamberlain <[email protected]>
> Signed-off-by: Linus Torvalds <[email protected]>
>
> diff --git a/kernel/module/tracking.c b/kernel/module/tracking.c
> index a139e63b6f20..26d812e07615 100644
> --- a/kernel/module/tracking.c
> +++ b/kernel/module/tracking.c
> @@ -22,6 +22,9 @@ int try_add_tainted_module(struct module *mod)
>
> module_assert_mutex_or_preempt();
>
> + if (!mod->taints)
> + goto out;
> +

One nit - since the out label just leads to a return statement,
the gotos in the function would be simpler as "return 0".