* Re: [PATCH] module: Taint the kernel when write-protecting ro_after_init fails [not found] <20250306103712.29549-1-petr.pavlu@suse.com> @ 2025-03-06 16:57 ` Luis Chamberlain 2025-03-12 15:45 ` Vlastimil Babka 0 siblings, 1 reply; 6+ messages in thread From: Luis Chamberlain @ 2025-03-06 16:57 UTC (permalink / raw) To: Petr Pavlu Cc: Sami Tolvanen, Daniel Gomez, Kees Cook, Petr Mladek, Jani Nikula, Andrew Morton, John Ogness, Greg Kroah-Hartman, linux-mm, linux-modules, linux-kernel + linux-mm since we're adding TAINT_BAD_PAGE On Thu, Mar 06, 2025 at 11:36:55AM +0100, Petr Pavlu wrote: > In the unlikely case that setting ro_after_init data to read-only fails, it > is too late to cancel loading of the module. The loader then issues only > a warning about the situation. Given that this reduces the kernel's > protection, it was suggested to make the failure more visible by tainting > the kernel. > > Allow TAINT_BAD_PAGE to be set per-module and use it in this case. The flag > is set in similar situations and has the following description in > Documentation/admin-guide/tainted-kernels.rst: "bad page referenced or some > unexpected page flags". > > Adjust the warning that reports the failure to avoid references to internal > functions and to add information about the kernel being tainted, both to > match the style of other messages in the file. Additionally, merge the > message on a single line because checkpatch.pl recommends that for the > ability to grep for the string. > > Suggested-by: Kees Cook <kees@kernel.org> > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> > --- > I opted to use TAINT_BAD_PAGE for now because it seemed unnecessary to me > to introduce a new flag only for this specific case. However, if we end up > similarly checking set_memory_*() in the boot context, a separate flag > would be probably better. > --- > kernel/module/main.c | 7 ++++--- > kernel/panic.c | 2 +- > 2 files changed, 5 insertions(+), 4 deletions(-) > > diff --git a/kernel/module/main.c b/kernel/module/main.c > index 1fb9ad289a6f..8f424a107b92 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c > @@ -3030,10 +3030,11 @@ static noinline int do_init_module(struct module *mod) > rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); > #endif > ret = module_enable_rodata_ro_after_init(mod); > - if (ret) > - pr_warn("%s: module_enable_rodata_ro_after_init() returned %d, " > - "ro_after_init data might still be writable\n", > + if (ret) { > + pr_warn("%s: write-protecting ro_after_init data failed with %d, the data might still be writable - tainting kernel\n", > mod->name, ret); > + add_taint_module(mod, TAINT_BAD_PAGE, LOCKDEP_STILL_OK); > + } > > mod_tree_remove_init(mod); > module_arch_freeing_init(mod); > diff --git a/kernel/panic.c b/kernel/panic.c > index d8635d5cecb2..794c443bfb5c 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -497,7 +497,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = { > TAINT_FLAG(CPU_OUT_OF_SPEC, 'S', ' ', false), > TAINT_FLAG(FORCED_RMMOD, 'R', ' ', false), > TAINT_FLAG(MACHINE_CHECK, 'M', ' ', false), > - TAINT_FLAG(BAD_PAGE, 'B', ' ', false), > + TAINT_FLAG(BAD_PAGE, 'B', ' ', true), > TAINT_FLAG(USER, 'U', ' ', false), > TAINT_FLAG(DIE, 'D', ' ', false), > TAINT_FLAG(OVERRIDDEN_ACPI_TABLE, 'A', ' ', false), Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> For our needs this makes sense, however I am curious if TAINT_BAD_PAGE is too broadly generic, and also if we're going to add it, if there are other mm uses for such a thing. Luis ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] module: Taint the kernel when write-protecting ro_after_init fails 2025-03-06 16:57 ` [PATCH] module: Taint the kernel when write-protecting ro_after_init fails Luis Chamberlain @ 2025-03-12 15:45 ` Vlastimil Babka 2025-03-12 16:30 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Vlastimil Babka @ 2025-03-12 15:45 UTC (permalink / raw) To: Luis Chamberlain, Petr Pavlu Cc: Sami Tolvanen, Daniel Gomez, Kees Cook, Petr Mladek, Jani Nikula, Andrew Morton, John Ogness, Greg Kroah-Hartman, linux-mm, linux-modules, linux-kernel On 3/6/25 17:57, Luis Chamberlain wrote: > + linux-mm since we're adding TAINT_BAD_PAGE > > On Thu, Mar 06, 2025 at 11:36:55AM +0100, Petr Pavlu wrote: >> In the unlikely case that setting ro_after_init data to read-only fails, it >> is too late to cancel loading of the module. The loader then issues only >> a warning about the situation. Given that this reduces the kernel's >> protection, it was suggested to make the failure more visible by tainting >> the kernel. >> >> Allow TAINT_BAD_PAGE to be set per-module and use it in this case. The flag >> is set in similar situations and has the following description in >> Documentation/admin-guide/tainted-kernels.rst: "bad page referenced or some >> unexpected page flags". >> >> Adjust the warning that reports the failure to avoid references to internal >> functions and to add information about the kernel being tainted, both to >> match the style of other messages in the file. Additionally, merge the >> message on a single line because checkpatch.pl recommends that for the >> ability to grep for the string. >> >> Suggested-by: Kees Cook <kees@kernel.org> >> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> >> --- >> I opted to use TAINT_BAD_PAGE for now because it seemed unnecessary to me >> to introduce a new flag only for this specific case. However, if we end up >> similarly checking set_memory_*() in the boot context, a separate flag >> would be probably better. >> --- >> kernel/module/main.c | 7 ++++--- >> kernel/panic.c | 2 +- >> 2 files changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/kernel/module/main.c b/kernel/module/main.c >> index 1fb9ad289a6f..8f424a107b92 100644 >> --- a/kernel/module/main.c >> +++ b/kernel/module/main.c >> @@ -3030,10 +3030,11 @@ static noinline int do_init_module(struct module *mod) >> rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); >> #endif >> ret = module_enable_rodata_ro_after_init(mod); >> - if (ret) >> - pr_warn("%s: module_enable_rodata_ro_after_init() returned %d, " >> - "ro_after_init data might still be writable\n", >> + if (ret) { >> + pr_warn("%s: write-protecting ro_after_init data failed with %d, the data might still be writable - tainting kernel\n", >> mod->name, ret); >> + add_taint_module(mod, TAINT_BAD_PAGE, LOCKDEP_STILL_OK); >> + } >> >> mod_tree_remove_init(mod); >> module_arch_freeing_init(mod); >> diff --git a/kernel/panic.c b/kernel/panic.c >> index d8635d5cecb2..794c443bfb5c 100644 >> --- a/kernel/panic.c >> +++ b/kernel/panic.c >> @@ -497,7 +497,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = { >> TAINT_FLAG(CPU_OUT_OF_SPEC, 'S', ' ', false), >> TAINT_FLAG(FORCED_RMMOD, 'R', ' ', false), >> TAINT_FLAG(MACHINE_CHECK, 'M', ' ', false), >> - TAINT_FLAG(BAD_PAGE, 'B', ' ', false), >> + TAINT_FLAG(BAD_PAGE, 'B', ' ', true), >> TAINT_FLAG(USER, 'U', ' ', false), >> TAINT_FLAG(DIE, 'D', ' ', false), >> TAINT_FLAG(OVERRIDDEN_ACPI_TABLE, 'A', ' ', false), > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > > For our needs this makes sense, however I am curious if TAINT_BAD_PAGE > is too broadly generic, and also if we're going to add it, if there are > other mm uses for such a thing. I'm not sure BAD_PAGE is a good fit. If there was a new flag that meant "a hardening measure failed", would that have other possible uses? The semantics would be that the kernel self-protection was weakened wrt expectations, even if not yet a corruption due to attack would be detected. Some admins could opt-in to panic in such case anyway, etc. Any other hardening features where such "failure to harden" is possible and could use this too? Kees? > Luis > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] module: Taint the kernel when write-protecting ro_after_init fails 2025-03-12 15:45 ` Vlastimil Babka @ 2025-03-12 16:30 ` Kees Cook 2025-03-12 17:38 ` Luis Chamberlain 2025-03-14 16:48 ` Christophe Leroy 0 siblings, 2 replies; 6+ messages in thread From: Kees Cook @ 2025-03-12 16:30 UTC (permalink / raw) To: Vlastimil Babka Cc: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez, Petr Mladek, Jani Nikula, Andrew Morton, John Ogness, Greg Kroah-Hartman, linux-mm, linux-modules, linux-kernel On Wed, Mar 12, 2025 at 04:45:24PM +0100, Vlastimil Babka wrote: > On 3/6/25 17:57, Luis Chamberlain wrote: > > + linux-mm since we're adding TAINT_BAD_PAGE > > > > On Thu, Mar 06, 2025 at 11:36:55AM +0100, Petr Pavlu wrote: > >> In the unlikely case that setting ro_after_init data to read-only fails, it > >> is too late to cancel loading of the module. The loader then issues only > >> a warning about the situation. Given that this reduces the kernel's > >> protection, it was suggested to make the failure more visible by tainting > >> the kernel. > >> > >> Allow TAINT_BAD_PAGE to be set per-module and use it in this case. The flag > >> is set in similar situations and has the following description in > >> Documentation/admin-guide/tainted-kernels.rst: "bad page referenced or some > >> unexpected page flags". > >> > >> Adjust the warning that reports the failure to avoid references to internal > >> functions and to add information about the kernel being tainted, both to > >> match the style of other messages in the file. Additionally, merge the > >> message on a single line because checkpatch.pl recommends that for the > >> ability to grep for the string. > >> > >> Suggested-by: Kees Cook <kees@kernel.org> > >> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> > >> --- > >> I opted to use TAINT_BAD_PAGE for now because it seemed unnecessary to me > >> to introduce a new flag only for this specific case. However, if we end up > >> similarly checking set_memory_*() in the boot context, a separate flag > >> would be probably better. > >> --- > >> kernel/module/main.c | 7 ++++--- > >> kernel/panic.c | 2 +- > >> 2 files changed, 5 insertions(+), 4 deletions(-) > >> > >> diff --git a/kernel/module/main.c b/kernel/module/main.c > >> index 1fb9ad289a6f..8f424a107b92 100644 > >> --- a/kernel/module/main.c > >> +++ b/kernel/module/main.c > >> @@ -3030,10 +3030,11 @@ static noinline int do_init_module(struct module *mod) > >> rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); > >> #endif > >> ret = module_enable_rodata_ro_after_init(mod); > >> - if (ret) > >> - pr_warn("%s: module_enable_rodata_ro_after_init() returned %d, " > >> - "ro_after_init data might still be writable\n", > >> + if (ret) { > >> + pr_warn("%s: write-protecting ro_after_init data failed with %d, the data might still be writable - tainting kernel\n", > >> mod->name, ret); > >> + add_taint_module(mod, TAINT_BAD_PAGE, LOCKDEP_STILL_OK); > >> + } > >> > >> mod_tree_remove_init(mod); > >> module_arch_freeing_init(mod); > >> diff --git a/kernel/panic.c b/kernel/panic.c > >> index d8635d5cecb2..794c443bfb5c 100644 > >> --- a/kernel/panic.c > >> +++ b/kernel/panic.c > >> @@ -497,7 +497,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = { > >> TAINT_FLAG(CPU_OUT_OF_SPEC, 'S', ' ', false), > >> TAINT_FLAG(FORCED_RMMOD, 'R', ' ', false), > >> TAINT_FLAG(MACHINE_CHECK, 'M', ' ', false), > >> - TAINT_FLAG(BAD_PAGE, 'B', ' ', false), > >> + TAINT_FLAG(BAD_PAGE, 'B', ' ', true), > >> TAINT_FLAG(USER, 'U', ' ', false), > >> TAINT_FLAG(DIE, 'D', ' ', false), > >> TAINT_FLAG(OVERRIDDEN_ACPI_TABLE, 'A', ' ', false), > > > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > > > > For our needs this makes sense, however I am curious if TAINT_BAD_PAGE > > is too broadly generic, and also if we're going to add it, if there are > > other mm uses for such a thing. > > I'm not sure BAD_PAGE is a good fit. If there was a new flag that meant "a > hardening measure failed", would that have other possible uses? The > semantics would be that the kernel self-protection was weakened wrt > expectations, even if not yet a corruption due to attack would be detected. > Some admins could opt-in to panic in such case anyway, etc. Any other > hardening features where such "failure to harden" is possible and could use > this too? Kees? Yeah, it could certainly be used. The direction the hardening stuff has taken is to use WARN() (as Linus requires no direct BUG() usage), and to recommend that end users tune their warn_limit sysctl as needed. Being able to TAINT might be useful, but I don't have any places that immediately come to mind that seem appropriate for it (besides this case). Hm, well, maybe in the case of a W^X test failure? (I note that this is also a "safe memory permission" failure...) How about TAINT_WEAKENED_PROTECTION ? Or something that carries that idea? -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] module: Taint the kernel when write-protecting ro_after_init fails 2025-03-12 16:30 ` Kees Cook @ 2025-03-12 17:38 ` Luis Chamberlain 2025-03-14 16:48 ` Christophe Leroy 1 sibling, 0 replies; 6+ messages in thread From: Luis Chamberlain @ 2025-03-12 17:38 UTC (permalink / raw) To: Kees Cook Cc: Vlastimil Babka, Petr Pavlu, Sami Tolvanen, Daniel Gomez, Petr Mladek, Jani Nikula, Andrew Morton, John Ogness, Greg Kroah-Hartman, linux-mm, linux-modules, linux-kernel On Wed, Mar 12, 2025 at 09:30:28AM -0700, Kees Cook wrote: > On Wed, Mar 12, 2025 at 04:45:24PM +0100, Vlastimil Babka wrote: > > On 3/6/25 17:57, Luis Chamberlain wrote: > > > + linux-mm since we're adding TAINT_BAD_PAGE > > > > > > On Thu, Mar 06, 2025 at 11:36:55AM +0100, Petr Pavlu wrote: > > >> In the unlikely case that setting ro_after_init data to read-only fails, it > > >> is too late to cancel loading of the module. The loader then issues only > > >> a warning about the situation. Given that this reduces the kernel's > > >> protection, it was suggested to make the failure more visible by tainting > > >> the kernel. > > >> > > >> Allow TAINT_BAD_PAGE to be set per-module and use it in this case. The flag > > >> is set in similar situations and has the following description in > > >> Documentation/admin-guide/tainted-kernels.rst: "bad page referenced or some > > >> unexpected page flags". > > >> > > >> Adjust the warning that reports the failure to avoid references to internal > > >> functions and to add information about the kernel being tainted, both to > > >> match the style of other messages in the file. Additionally, merge the > > >> message on a single line because checkpatch.pl recommends that for the > > >> ability to grep for the string. > > >> > > >> Suggested-by: Kees Cook <kees@kernel.org> > > >> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> > > >> --- > > >> I opted to use TAINT_BAD_PAGE for now because it seemed unnecessary to me > > >> to introduce a new flag only for this specific case. However, if we end up > > >> similarly checking set_memory_*() in the boot context, a separate flag > > >> would be probably better. > > >> --- > > >> kernel/module/main.c | 7 ++++--- > > >> kernel/panic.c | 2 +- > > >> 2 files changed, 5 insertions(+), 4 deletions(-) > > >> > > >> diff --git a/kernel/module/main.c b/kernel/module/main.c > > >> index 1fb9ad289a6f..8f424a107b92 100644 > > >> --- a/kernel/module/main.c > > >> +++ b/kernel/module/main.c > > >> @@ -3030,10 +3030,11 @@ static noinline int do_init_module(struct module *mod) > > >> rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); > > >> #endif > > >> ret = module_enable_rodata_ro_after_init(mod); > > >> - if (ret) > > >> - pr_warn("%s: module_enable_rodata_ro_after_init() returned %d, " > > >> - "ro_after_init data might still be writable\n", > > >> + if (ret) { > > >> + pr_warn("%s: write-protecting ro_after_init data failed with %d, the data might still be writable - tainting kernel\n", > > >> mod->name, ret); > > >> + add_taint_module(mod, TAINT_BAD_PAGE, LOCKDEP_STILL_OK); > > >> + } > > >> > > >> mod_tree_remove_init(mod); > > >> module_arch_freeing_init(mod); > > >> diff --git a/kernel/panic.c b/kernel/panic.c > > >> index d8635d5cecb2..794c443bfb5c 100644 > > >> --- a/kernel/panic.c > > >> +++ b/kernel/panic.c > > >> @@ -497,7 +497,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = { > > >> TAINT_FLAG(CPU_OUT_OF_SPEC, 'S', ' ', false), > > >> TAINT_FLAG(FORCED_RMMOD, 'R', ' ', false), > > >> TAINT_FLAG(MACHINE_CHECK, 'M', ' ', false), > > >> - TAINT_FLAG(BAD_PAGE, 'B', ' ', false), > > >> + TAINT_FLAG(BAD_PAGE, 'B', ' ', true), > > >> TAINT_FLAG(USER, 'U', ' ', false), > > >> TAINT_FLAG(DIE, 'D', ' ', false), > > >> TAINT_FLAG(OVERRIDDEN_ACPI_TABLE, 'A', ' ', false), > > > > > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > > > > > > For our needs this makes sense, however I am curious if TAINT_BAD_PAGE > > > is too broadly generic, and also if we're going to add it, if there are > > > other mm uses for such a thing. > > > > I'm not sure BAD_PAGE is a good fit. If there was a new flag that meant "a > > hardening measure failed", would that have other possible uses? The > > semantics would be that the kernel self-protection was weakened wrt > > expectations, even if not yet a corruption due to attack would be detected. > > Some admins could opt-in to panic in such case anyway, etc. Any other > > hardening features where such "failure to harden" is possible and could use > > this too? Kees? > > Yeah, it could certainly be used. The direction the hardening stuff has > taken is to use WARN() (as Linus requires no direct BUG() usage), and to > recommend that end users tune their warn_limit sysctl as needed. > > Being able to TAINT might be useful, but I don't have any places that > immediately come to mind that seem appropriate for it (besides this > case). Hm, well, maybe in the case of a W^X test failure? (I note that > this is also a "safe memory permission" failure...) > > How about TAINT_WEAKENED_PROTECTION ? Or something that carries that > idea? It's different, but hw poison [0] already has policies for running into poisoned pages through MCA recovery, there are a few sysctl knobs to tune this, for example vm.memory_failure_recovery set to 0 will panic on a hw poison page. A hw poison event without a panic seems like a possible use case for such a taint TAINT_WEAKENED_PROTECTION? [0] https://github.com/torvalds/linux/blob/master/Documentation/mm/hwpoison.rst Luis ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] module: Taint the kernel when write-protecting ro_after_init fails 2025-03-12 16:30 ` Kees Cook 2025-03-12 17:38 ` Luis Chamberlain @ 2025-03-14 16:48 ` Christophe Leroy 2025-03-14 19:19 ` Kees Cook 1 sibling, 1 reply; 6+ messages in thread From: Christophe Leroy @ 2025-03-14 16:48 UTC (permalink / raw) To: Kees Cook, Vlastimil Babka Cc: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez, Petr Mladek, Jani Nikula, Andrew Morton, John Ogness, Greg Kroah-Hartman, linux-mm, linux-modules, linux-kernel Le 12/03/2025 à 17:30, Kees Cook a écrit : > On Wed, Mar 12, 2025 at 04:45:24PM +0100, Vlastimil Babka wrote: >> On 3/6/25 17:57, Luis Chamberlain wrote: >>> + linux-mm since we're adding TAINT_BAD_PAGE >>> >>> On Thu, Mar 06, 2025 at 11:36:55AM +0100, Petr Pavlu wrote: >>>> In the unlikely case that setting ro_after_init data to read-only fails, it >>>> is too late to cancel loading of the module. The loader then issues only >>>> a warning about the situation. Given that this reduces the kernel's >>>> protection, it was suggested to make the failure more visible by tainting >>>> the kernel. >>>> >>>> Allow TAINT_BAD_PAGE to be set per-module and use it in this case. The flag >>>> is set in similar situations and has the following description in >>>> Documentation/admin-guide/tainted-kernels.rst: "bad page referenced or some >>>> unexpected page flags". >>>> >>>> Adjust the warning that reports the failure to avoid references to internal >>>> functions and to add information about the kernel being tainted, both to >>>> match the style of other messages in the file. Additionally, merge the >>>> message on a single line because checkpatch.pl recommends that for the >>>> ability to grep for the string. >>>> >>>> Suggested-by: Kees Cook <kees@kernel.org> >>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> >>>> --- >>>> I opted to use TAINT_BAD_PAGE for now because it seemed unnecessary to me >>>> to introduce a new flag only for this specific case. However, if we end up >>>> similarly checking set_memory_*() in the boot context, a separate flag >>>> would be probably better. >>>> --- >>>> kernel/module/main.c | 7 ++++--- >>>> kernel/panic.c | 2 +- >>>> 2 files changed, 5 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/kernel/module/main.c b/kernel/module/main.c >>>> index 1fb9ad289a6f..8f424a107b92 100644 >>>> --- a/kernel/module/main.c >>>> +++ b/kernel/module/main.c >>>> @@ -3030,10 +3030,11 @@ static noinline int do_init_module(struct module *mod) >>>> rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); >>>> #endif >>>> ret = module_enable_rodata_ro_after_init(mod); >>>> - if (ret) >>>> - pr_warn("%s: module_enable_rodata_ro_after_init() returned %d, " >>>> - "ro_after_init data might still be writable\n", >>>> + if (ret) { >>>> + pr_warn("%s: write-protecting ro_after_init data failed with %d, the data might still be writable - tainting kernel\n", >>>> mod->name, ret); >>>> + add_taint_module(mod, TAINT_BAD_PAGE, LOCKDEP_STILL_OK); >>>> + } >>>> >>>> mod_tree_remove_init(mod); >>>> module_arch_freeing_init(mod); >>>> diff --git a/kernel/panic.c b/kernel/panic.c >>>> index d8635d5cecb2..794c443bfb5c 100644 >>>> --- a/kernel/panic.c >>>> +++ b/kernel/panic.c >>>> @@ -497,7 +497,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = { >>>> TAINT_FLAG(CPU_OUT_OF_SPEC, 'S', ' ', false), >>>> TAINT_FLAG(FORCED_RMMOD, 'R', ' ', false), >>>> TAINT_FLAG(MACHINE_CHECK, 'M', ' ', false), >>>> - TAINT_FLAG(BAD_PAGE, 'B', ' ', false), >>>> + TAINT_FLAG(BAD_PAGE, 'B', ' ', true), >>>> TAINT_FLAG(USER, 'U', ' ', false), >>>> TAINT_FLAG(DIE, 'D', ' ', false), >>>> TAINT_FLAG(OVERRIDDEN_ACPI_TABLE, 'A', ' ', false), >>> >>> Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> >>> >>> For our needs this makes sense, however I am curious if TAINT_BAD_PAGE >>> is too broadly generic, and also if we're going to add it, if there are >>> other mm uses for such a thing. >> >> I'm not sure BAD_PAGE is a good fit. If there was a new flag that meant "a >> hardening measure failed", would that have other possible uses? The >> semantics would be that the kernel self-protection was weakened wrt >> expectations, even if not yet a corruption due to attack would be detected. >> Some admins could opt-in to panic in such case anyway, etc. Any other >> hardening features where such "failure to harden" is possible and could use >> this too? Kees? > > Yeah, it could certainly be used. The direction the hardening stuff has > taken is to use WARN() (as Linus requires no direct BUG() usage), and to > recommend that end users tune their warn_limit sysctl as needed. > > Being able to TAINT might be useful, but I don't have any places that > immediately come to mind that seem appropriate for it (besides this > case). Hm, well, maybe in the case of a W^X test failure? (I note that > this is also a "safe memory permission" failure...) Can be anything that fails in function mark_readonly() ? : jump_label_init_ro(); mark_rodata_ro(); debug_checkwx(); rodata_test(); Christophe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] module: Taint the kernel when write-protecting ro_after_init fails 2025-03-14 16:48 ` Christophe Leroy @ 2025-03-14 19:19 ` Kees Cook 0 siblings, 0 replies; 6+ messages in thread From: Kees Cook @ 2025-03-14 19:19 UTC (permalink / raw) To: Christophe Leroy Cc: Vlastimil Babka, Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez, Petr Mladek, Jani Nikula, Andrew Morton, John Ogness, Greg Kroah-Hartman, linux-mm, linux-modules, linux-kernel On Fri, Mar 14, 2025 at 05:48:00PM +0100, Christophe Leroy wrote: > > > Le 12/03/2025 à 17:30, Kees Cook a écrit : > > On Wed, Mar 12, 2025 at 04:45:24PM +0100, Vlastimil Babka wrote: > > > On 3/6/25 17:57, Luis Chamberlain wrote: > > > > + linux-mm since we're adding TAINT_BAD_PAGE > > > > > > > > On Thu, Mar 06, 2025 at 11:36:55AM +0100, Petr Pavlu wrote: > > > > > In the unlikely case that setting ro_after_init data to read-only fails, it > > > > > is too late to cancel loading of the module. The loader then issues only > > > > > a warning about the situation. Given that this reduces the kernel's > > > > > protection, it was suggested to make the failure more visible by tainting > > > > > the kernel. > > > > > > > > > > Allow TAINT_BAD_PAGE to be set per-module and use it in this case. The flag > > > > > is set in similar situations and has the following description in > > > > > Documentation/admin-guide/tainted-kernels.rst: "bad page referenced or some > > > > > unexpected page flags". > > > > > > > > > > Adjust the warning that reports the failure to avoid references to internal > > > > > functions and to add information about the kernel being tainted, both to > > > > > match the style of other messages in the file. Additionally, merge the > > > > > message on a single line because checkpatch.pl recommends that for the > > > > > ability to grep for the string. > > > > > > > > > > Suggested-by: Kees Cook <kees@kernel.org> > > > > > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com> > > > > > --- > > > > > I opted to use TAINT_BAD_PAGE for now because it seemed unnecessary to me > > > > > to introduce a new flag only for this specific case. However, if we end up > > > > > similarly checking set_memory_*() in the boot context, a separate flag > > > > > would be probably better. > > > > > --- > > > > > kernel/module/main.c | 7 ++++--- > > > > > kernel/panic.c | 2 +- > > > > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/kernel/module/main.c b/kernel/module/main.c > > > > > index 1fb9ad289a6f..8f424a107b92 100644 > > > > > --- a/kernel/module/main.c > > > > > +++ b/kernel/module/main.c > > > > > @@ -3030,10 +3030,11 @@ static noinline int do_init_module(struct module *mod) > > > > > rcu_assign_pointer(mod->kallsyms, &mod->core_kallsyms); > > > > > #endif > > > > > ret = module_enable_rodata_ro_after_init(mod); > > > > > - if (ret) > > > > > - pr_warn("%s: module_enable_rodata_ro_after_init() returned %d, " > > > > > - "ro_after_init data might still be writable\n", > > > > > + if (ret) { > > > > > + pr_warn("%s: write-protecting ro_after_init data failed with %d, the data might still be writable - tainting kernel\n", > > > > > mod->name, ret); > > > > > + add_taint_module(mod, TAINT_BAD_PAGE, LOCKDEP_STILL_OK); > > > > > + } > > > > > mod_tree_remove_init(mod); > > > > > module_arch_freeing_init(mod); > > > > > diff --git a/kernel/panic.c b/kernel/panic.c > > > > > index d8635d5cecb2..794c443bfb5c 100644 > > > > > --- a/kernel/panic.c > > > > > +++ b/kernel/panic.c > > > > > @@ -497,7 +497,7 @@ const struct taint_flag taint_flags[TAINT_FLAGS_COUNT] = { > > > > > TAINT_FLAG(CPU_OUT_OF_SPEC, 'S', ' ', false), > > > > > TAINT_FLAG(FORCED_RMMOD, 'R', ' ', false), > > > > > TAINT_FLAG(MACHINE_CHECK, 'M', ' ', false), > > > > > - TAINT_FLAG(BAD_PAGE, 'B', ' ', false), > > > > > + TAINT_FLAG(BAD_PAGE, 'B', ' ', true), > > > > > TAINT_FLAG(USER, 'U', ' ', false), > > > > > TAINT_FLAG(DIE, 'D', ' ', false), > > > > > TAINT_FLAG(OVERRIDDEN_ACPI_TABLE, 'A', ' ', false), > > > > > > > > Reviewed-by: Luis Chamberlain <mcgrof@kernel.org> > > > > > > > > For our needs this makes sense, however I am curious if TAINT_BAD_PAGE > > > > is too broadly generic, and also if we're going to add it, if there are > > > > other mm uses for such a thing. > > > > > > I'm not sure BAD_PAGE is a good fit. If there was a new flag that meant "a > > > hardening measure failed", would that have other possible uses? The > > > semantics would be that the kernel self-protection was weakened wrt > > > expectations, even if not yet a corruption due to attack would be detected. > > > Some admins could opt-in to panic in such case anyway, etc. Any other > > > hardening features where such "failure to harden" is possible and could use > > > this too? Kees? > > > > Yeah, it could certainly be used. The direction the hardening stuff has > > taken is to use WARN() (as Linus requires no direct BUG() usage), and to > > recommend that end users tune their warn_limit sysctl as needed. > > > > Being able to TAINT might be useful, but I don't have any places that > > immediately come to mind that seem appropriate for it (besides this > > case). Hm, well, maybe in the case of a W^X test failure? (I note that > > this is also a "safe memory permission" failure...) > > Can be anything that fails in function mark_readonly() ? : > > jump_label_init_ro(); > mark_rodata_ro(); > debug_checkwx(); > rodata_test(); Yeah, works for me! -- Kees Cook ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-03-14 19:19 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20250306103712.29549-1-petr.pavlu@suse.com>
2025-03-06 16:57 ` [PATCH] module: Taint the kernel when write-protecting ro_after_init fails Luis Chamberlain
2025-03-12 15:45 ` Vlastimil Babka
2025-03-12 16:30 ` Kees Cook
2025-03-12 17:38 ` Luis Chamberlain
2025-03-14 16:48 ` Christophe Leroy
2025-03-14 19:19 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox