linux-kernel - Re: [PATCH] ima: add a knob ima= to make IMA be able to be disabled

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1d2848fe45dbf58707cecf16c4fc46b179e24415.camel@linux.ibm.com>
Date: Wed, 21 May 2025 08:54:10 -0400
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Baoquan He <bhe@...hat.com>, linux-integrity@...r.kernel.org,
        kexec@...ts.infradead.org
Cc: 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 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.
> > 
> > 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;
+       }

> > +	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