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

[Mimi Zohar <zohar@linux.ibm.com>]

In addition, please update the Subject line to be less generic.

thanks,

Mimi


On Wed, 2025-05-21 at 08:54 -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.
> > > 
> > > 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@redhat.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
>