| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f4qj7ql2g2yhp44f6quq2ighboq57vilzepnltxgje5gg2cymv@vi5i2krnk7zf>
Date: Thu, 22 May 2025 11:14:35 +0800
From: Coiby Xu <coxu@...hat.com>
To: Baoquan He <bhe@...hat.com>, Mimi Zohar <zohar@...ux.ibm.com>
Cc: linux-integrity@...r.kernel.org, kexec@...ts.infradead.org,
linux-kernel@...r.kernel.org, pmenzel@...gen.mpg.de, ruyang@...hat.com,
chenste@...ux.microsoft.com
Subject: Re: [PATCH] ima: add a knob ima= to make IMA be able to be disabled
On Wed, May 21, 2025 at 08:54:10AM -0400, Mimi Zohar wrote:
>On Fri, 2025-05-16 at 08:22 +0800, Baoquan He wrote:
>> CC kexec list.
>>
>> On 05/16/25 at 07:39am, Baoquan He wrote:
>> > Kdump kernel doesn't need IMA functionality, and enabling IMA will cost
>> > extra memory. It would be very helpful to allow IMA to be disabled for
>> > kdump kernel.
>
>The real question is not whether kdump needs "IMA", but whether not enabling
>IMA in the kdump kernel could be abused. The comments below don't address
>that question but limit/emphasize, as much as possible, turning IMA off is
>limited to the kdump kernel.
>
>> >
>> > And Coiby also mentioned that for kdump kernel incorrect ima-policy loaded
>> > by systemd could cause kdump kernel hang, and it's possible the booting
>> > process may be stopped by a strict, albeit syntax-correct policy and users
>> > can't log into the system to fix the policy. In these cases, allowing to
>> > disable IMA is very helpful too for kdump kernel.
To clarify, what I mentioned early is that the system hangs because
systemd freezes after trying to load an incorrect policy or the booting
process may be stopped by a strict, albeit syntax-correct policy. kdump
won't be affected in these cases because the IMA policy file
(/etc/ima/ima-policy) won't be installed into the kdump initramfs by
default so there is no chance for this IMA policy file to affect kdump.
Besides, if the normal/1st system does hang because of the IMA policy,
the kdump kernel and initramfs simply won't be loaded thus no chance to
to be booted or to load an IMA policy file.
But kdump can be affected if the kernel cmdline parameter
ima_policy=appraise_tcb is configured. Because currently files in kdump
initramfs don't have security.ima. Without the reference value stored in
security.ima to prove a file's integrity, IMA will prevent accessing
this file. So in this case, IMA can also stop systemd from running.
>> >
>> > Hence add a knob ima=on|off here to allow people to disable IMA in kdump
>> > kenrel if needed.
>
>^kernel
>
>> >
>> > Signed-off-by: Baoquan He <bhe@...hat.com>
>> > ---
>> > .../admin-guide/kernel-parameters.txt | 5 +++++
>> > security/integrity/ima/ima_main.c | 22 +++++++++++++++++++
>> > 2 files changed, 27 insertions(+)
>> >
>> > diff --git a/Documentation/admin-guide/kernel-parameters.txt
>> > b/Documentation/admin-guide/kernel-parameters.txt
>> > index d9fd26b95b34..762fb6ddcc24 100644
>> > --- a/Documentation/admin-guide/kernel-parameters.txt
>> > +++ b/Documentation/admin-guide/kernel-parameters.txt
>> > @@ -2202,6 +2202,11 @@
>> > different crypto accelerators. This option can be
>> > used
>> > to achieve best performance for particular HW.
>> >
>> > + ima= [IMA] Enable or disable IMA
>> > + Format: { "off" | "on" }
>> > + Default: "on"
>> > + Note that this is only useful for kdump kernel.
>
>Instead of "useful" I would prefer something clearer like "limited".
>
>> > +
>> > init= [KNL]
>> > Format: <full_path>
>> > Run specified binary instead of /sbin/init as
>> > init
>> > diff --git a/security/integrity/ima/ima_main.c
>> > b/security/integrity/ima/ima_main.c
>> > index f3e7ac513db3..07af5c6af138 100644
>> > --- a/security/integrity/ima/ima_main.c
>> > +++ b/security/integrity/ima/ima_main.c
>> > @@ -27,6 +27,7 @@
>> > #include <linux/fs.h>
>> > #include <linux/iversion.h>
>> > #include <linux/evm.h>
>> > +#include <linux/crash_dump.h>
>> >
>> > #include "ima.h"
>> >
>> > @@ -38,11 +39,27 @@ int ima_appraise;
>> >
>> > int __ro_after_init ima_hash_algo = HASH_ALGO_SHA1;
>> > static int hash_setup_done;
>> > +static int ima_disabled;
>
>Like the ima_hash_algo variable definition above, ima_disabled should be
>defined as __ro_after_init.
>
>> >
>> > static struct notifier_block ima_lsm_policy_notifier = {
>> > .notifier_call = ima_lsm_policy_change,
>> > };
>> >
>> > +static int __init ima_setup(char *str)
>> > +{
>
>is_kdump_kernel() should also be called here, before the tests below.
>Something like:
>
>+ if (!is_kdump_kernel()) {
>+ pr_info("Warning ima setup option only permitted in kdump");
>+ return 1;
>+ }
Yes, this kind of info will be helpful to avoid users misusing
ima=off. I already saw a case where a user tried to use ima=0 to skip
loading an invalid IMA policy file in order to resolve booting failure
"systemd Freezing execution" and even filed a bug saying ima=0 doesn't
work.
>
>> > + if (strncmp(str, "off", 3) == 0)
>> > + ima_disabled = 1;
>> > + else if (strncmp(str, "on", 2) == 0)
>> > + ima_disabled = 0;
>> > + else
>> > + pr_err("Invalid ima setup option: \"%s\" , please specify
>> > ima=on|off.", str);
>> > +
>> > + return 1;
>> > +}
>> > +__setup("ima=", ima_setup);
>> > +
>> > +
>> > +
>
>Remove the extraneous blank line.
>
>> > static int __init hash_setup(char *str)
>> > {
>> > struct ima_template_desc *template_desc =
>> > ima_template_desc_current();
>> > @@ -1184,6 +1201,11 @@ static int __init init_ima(void)
>> > {
>> > int error;
>> >
>> > + if (ima_disabled && is_kdump_kernel()) {
>> > + pr_info("IMA functionality is disabled");
>> > + return 0;
>> > + }
>> > +
>
>Even with the additional call to is_kdump_kernel() in ima_setup, please keep
>the is_kdump_kernel() test here as well. Even though the code is self
>describing, please add a one line comment emphasizing disabling IMA is limited
>to kdump.
>
>> > ima_appraise_parse_cmdline();
>> > ima_init_template_list();
>> > hash_setup(CONFIG_IMA_DEFAULT_HASH);
>> > --
>> > 2.41.0
>> >
>>
>>
>
>thanks,
>
>Mimi
--
Best regards,
Coiby
Powered by blists - more mailing lists