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.
>
> > >
> > > 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 <b...@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".
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
> > >
> >
> >
>
> thanks,
>
> Mimi
>
