| 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: <c0f1df02160138d0782cb897eda844287b3d7792.camel@linux.ibm.com>
Date: Thu, 22 May 2025 07:08:04 -0400
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Baoquan He <bhe@...hat.com>
Cc: linux-integrity@...r.kernel.org, kexec@...ts.infradead.org,
linux-kernel@...r.kernel.org, pmenzel@...gen.mpg.de, coxu@...hat.com,
ruyang@...hat.com, chenste@...ux.microsoft.com
Subject: Re: [PATCH] ima: add a knob ima= to make IMA be able to be disabled
On Thu, 2025-05-22 at 11:24 +0800, Baoquan He wrote:
> On 05/21/25 at 08:54am, 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.
>
> Thanks a lot for careufl reviewing and great suggestions.
>
> >
> > 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.
>
> Are you suggesting removing below paragraph from patch log because they
> are redundant? I can remove it in v2 if yes.
"The comments below" was referring to my comments on the patch, not the next
paragraph. "don't address that question" refers to whether the kdump kernel
could be abused.
We're trying to close integrity gaps, not add new ones. Verifying the UKI's
signature addresses the integrity of the initramfs. What about the integrity of
the kdump initramfs (or for that matter the kexec initramfs)? If the kdump
initramfs was signed, IMA would be able to verify it before the kexec.
As for the next paragraph, based on Coiby's response, please remove it.
thanks,
Mimi
>
> >
> > > >
> > > > 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.
> > > >
> > > > Hence add a knob ima=on|off here to allow people to disable IMA in kdump
> > > > kenrel if needed.
> >
> > ^kernel
>
> Will change.
>
> >
> > > >
> > > > 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".
>
> Makes sense, will change.
>
> >
> > > > +
> > > > 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.
>
> Will add __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;
> > + }
>
> Sure, will change as suggested.
>
> >
> > > > + 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.
>
> sure.
>
> >
> > > > 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.
>
> OK, will keep code here as this v1 is and add one line of comment at
> above.
>
> Thanks again.
>
> >
> > > > ima_appraise_parse_cmdline();
> > > > ima_init_template_list();
> > > > hash_setup(CONFIG_IMA_DEFAULT_HASH);
> > > > --
> > > > 2.41.0
> > > >
> > >
> > >
Powered by blists - more mailing lists