| Message ID | 20250331061611.253919-1-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] ima: add a knob to make IMA be able to be disabled | expand |
Dear Baoquan, Thank you for your patch. I’d add the knob name to the commit message summary/title, so it shows up in `git log --oneline`. Am 31.03.25 um 08:16 schrieb Baoquan He: > It doesn't make sense to run IMA functionality in kdump kernel, and that > will cost extra memory. It would be great to allow IMA to be disabled on > purpose, e.g for kdump kernel. > > Hence add a knob here to allow people to disable IMA if needed. `initcall_blacklist=…` could be used already. I prefer a dedicated parameter too though. > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > security/integrity/ima/ima_main.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 28b8b0db6f9b..5d677d1389fe 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -38,11 +38,27 @@ int ima_appraise; > > int __ro_after_init ima_hash_algo = HASH_ALGO_SHA1; > static int hash_setup_done; > +static int ima_disabled = 0; > > static struct notifier_block ima_lsm_policy_notifier = { > .notifier_call = ima_lsm_policy_change, > }; > > +static int __init ima_setup(char *str) > +{ > + 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\" ", str); I’d add the allowed strings. > + > + return 1; > +} > +__setup("ima=", ima_setup); > + > + > + > static int __init hash_setup(char *str) > { > struct ima_template_desc *template_desc = ima_template_desc_current(); > @@ -1176,6 +1192,11 @@ static int __init init_ima(void) > { > int error; > > + if (ima_disabled) { > + pr_info("IMA functionality is disabled on purpose!"); … on Linux CLI. > + return 0; > + } > + > ima_appraise_parse_cmdline(); > ima_init_template_list(); > hash_setup(CONFIG_IMA_DEFAULT_HASH); Kind regards, Paul
Hi Paul, On 03/31/25 at 08:22am, Paul Menzel wrote: > Thanks for your careful reviewing. > > Thank you for your patch. I’d add the knob name to the commit message > summary/title, so it shows up in `git log --oneline`. Sounds great, will change. > > Am 31.03.25 um 08:16 schrieb Baoquan He: > > It doesn't make sense to run IMA functionality in kdump kernel, and that > > will cost extra memory. It would be great to allow IMA to be disabled on > > purpose, e.g for kdump kernel. > > > > Hence add a knob here to allow people to disable IMA if needed. > > `initcall_blacklist=…` could be used already. I prefer a dedicated parameter > too though. Yes, adding parameter can provide an explicit functionality to the feature. While 'initcall_blacklist=' can't guarantee there won't be dependency or connection between ima and other feature, and people could add or change the connection anytime when userspace is using it but not knowing the change. > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > --- > > security/integrity/ima/ima_main.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > > index 28b8b0db6f9b..5d677d1389fe 100644 > > --- a/security/integrity/ima/ima_main.c > > +++ b/security/integrity/ima/ima_main.c > > @@ -38,11 +38,27 @@ int ima_appraise; > > int __ro_after_init ima_hash_algo = HASH_ALGO_SHA1; > > static int hash_setup_done; > > +static int ima_disabled = 0; > > static struct notifier_block ima_lsm_policy_notifier = { > > .notifier_call = ima_lsm_policy_change, > > }; > > +static int __init ima_setup(char *str) > > +{ > > + 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\" ", str); > > I’d add the allowed strings. Sounds better, will change. > > > + > > + return 1; > > +} > > +__setup("ima=", ima_setup); > > + > > + > > + > > static int __init hash_setup(char *str) > > { > > struct ima_template_desc *template_desc = ima_template_desc_current(); > > @@ -1176,6 +1192,11 @@ static int __init init_ima(void) > > { > > int error; > > + if (ima_disabled) { > > + pr_info("IMA functionality is disabled on purpose!"); > > … on Linux CLI. I may not get the suggestion in this place, could you be more specific? > > > + return 0; > > + } > > + > > ima_appraise_parse_cmdline(); > > ima_init_template_list(); > > hash_setup(CONFIG_IMA_DEFAULT_HASH); > > > Kind regards, > > Paul >
On Mon, 2025-03-31 at 14:16 +0800, Baoquan He wrote: > It doesn't make sense to run IMA functionality in kdump kernel, and that > will cost extra memory. It would be great to allow IMA to be disabled on > purpose, e.g for kdump kernel. > > Hence add a knob here to allow people to disable IMA if needed. > > Signed-off-by: Baoquan He <bhe@redhat.com> > --- > security/integrity/ima/ima_main.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 28b8b0db6f9b..5d677d1389fe 100644 > --- a/security/integrity/ima/ima_main.c > +++ b/security/integrity/ima/ima_main.c > @@ -38,11 +38,27 @@ int ima_appraise; > > int __ro_after_init ima_hash_algo = HASH_ALGO_SHA1; > static int hash_setup_done; > +static int ima_disabled = 0; > > static struct notifier_block ima_lsm_policy_notifier = { > .notifier_call = ima_lsm_policy_change, > }; > > +static int __init ima_setup(char *str) > +{ > + 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\" ", str); > + > + return 1; > +} > +__setup("ima=", ima_setup); I understand your wanting to disable IMA for Kdump, but this goes way beyond that. Please don't make it generic like this. Please refer to ima_appraise_parse_cmdline(). Mimi > + > + > + > static int __init hash_setup(char *str) > { > struct ima_template_desc *template_desc = ima_template_desc_current(); > @@ -1176,6 +1192,11 @@ static int __init init_ima(void) > { > int error; > > + if (ima_disabled) { > + pr_info("IMA functionality is disabled on purpose!"); > + return 0; > + } > + > ima_appraise_parse_cmdline(); > ima_init_template_list(); > hash_setup(CONFIG_IMA_DEFAULT_HASH);
On Mon, Mar 31, 2025 at 08:15:08AM -0400, Mimi Zohar wrote: >On Mon, 2025-03-31 at 14:16 +0800, Baoquan He wrote: >> It doesn't make sense to run IMA functionality in kdump kernel, and that >> will cost extra memory. It would be great to allow IMA to be disabled on >> purpose, e.g for kdump kernel. >> >> Hence add a knob here to allow people to disable IMA if needed. >> >> Signed-off-by: Baoquan He <bhe@redhat.com> >> --- >> security/integrity/ima/ima_main.c | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c >> index 28b8b0db6f9b..5d677d1389fe 100644 >> --- a/security/integrity/ima/ima_main.c >> +++ b/security/integrity/ima/ima_main.c >> @@ -38,11 +38,27 @@ int ima_appraise; >> >> int __ro_after_init ima_hash_algo = HASH_ALGO_SHA1; >> static int hash_setup_done; >> +static int ima_disabled = 0; >> >> static struct notifier_block ima_lsm_policy_notifier = { >> .notifier_call = ima_lsm_policy_change, >> }; >> >> +static int __init ima_setup(char *str) >> +{ >> + 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\" ", str); >> + >> + return 1; >> +} >> +__setup("ima=", ima_setup); > >I understand your wanting to disable IMA for Kdump, but this goes way beyond >that. Please don't make it generic like this. > >Please refer to ima_appraise_parse_cmdline(). Hi Mimi, To save memory for kdump, it seems init_ima has been to be skipped thus ima=off is necessary (ima_appraise=off won't serve the purpose). Or do you have any specific concerns in mind? > >Mimi > >> + >> + >> + >> static int __init hash_setup(char *str) >> { >> struct ima_template_desc *template_desc = ima_template_desc_current(); >> @@ -1176,6 +1192,11 @@ static int __init init_ima(void) >> { >> int error; >> >> + if (ima_disabled) { >> + pr_info("IMA functionality is disabled on purpose!"); >> + return 0; >> + } >> + >> ima_appraise_parse_cmdline(); >> ima_init_template_list(); >> hash_setup(CONFIG_IMA_DEFAULT_HASH); > >
On Wed, 2 Apr 2025 at 09:41, Coiby Xu <coxu@redhat.com> wrote: > > On Mon, Mar 31, 2025 at 08:15:08AM -0400, Mimi Zohar wrote: > >On Mon, 2025-03-31 at 14:16 +0800, Baoquan He wrote: > >> It doesn't make sense to run IMA functionality in kdump kernel, and that > >> will cost extra memory. It would be great to allow IMA to be disabled on > >> purpose, e.g for kdump kernel. > >> > >> Hence add a knob here to allow people to disable IMA if needed. > >> > >> Signed-off-by: Baoquan He <bhe@redhat.com> > >> --- > >> security/integrity/ima/ima_main.c | 21 +++++++++++++++++++++ > >> 1 file changed, 21 insertions(+) > >> > >> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > >> index 28b8b0db6f9b..5d677d1389fe 100644 > >> --- a/security/integrity/ima/ima_main.c > >> +++ b/security/integrity/ima/ima_main.c > >> @@ -38,11 +38,27 @@ int ima_appraise; > >> > >> int __ro_after_init ima_hash_algo = HASH_ALGO_SHA1; > >> static int hash_setup_done; > >> +static int ima_disabled = 0; > >> > >> static struct notifier_block ima_lsm_policy_notifier = { > >> .notifier_call = ima_lsm_policy_change, > >> }; > >> > >> +static int __init ima_setup(char *str) > >> +{ > >> + 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\" ", str); > >> + > >> + return 1; > >> +} > >> +__setup("ima=", ima_setup); > > > >I understand your wanting to disable IMA for Kdump, but this goes way beyond > >that. Please don't make it generic like this. > > > >Please refer to ima_appraise_parse_cmdline(). > > Hi Mimi, > > To save memory for kdump, it seems init_ima has been to be skipped thus > ima=off is necessary (ima_appraise=off won't serve the purpose). Or do > you have any specific concerns in mind? I think as Mimi said see below logic enforces the IMA even with the cmdline disabling, see ima_appraise_parse_cmdline: if (sb_state) { if (!(appraisal_state & IMA_APPRAISE_ENFORCE)) pr_info("Secure boot enabled: ignoring ima_appraise=%s option", str); } else { ima_appraise = appraisal_state; } > > > > >Mimi > > > >> + > >> + > >> + > >> static int __init hash_setup(char *str) > >> { > >> struct ima_template_desc *template_desc = ima_template_desc_current(); > >> @@ -1176,6 +1192,11 @@ static int __init init_ima(void) > >> { > >> int error; > >> > >> + if (ima_disabled) { > >> + pr_info("IMA functionality is disabled on purpose!"); > >> + return 0; > >> + } > >> + > >> ima_appraise_parse_cmdline(); > >> ima_init_template_list(); > >> hash_setup(CONFIG_IMA_DEFAULT_HASH); > > > > > > -- > Best regards, > Coiby > >
On Wed, 2025-04-02 at 09:47 +0800, RuiRui Yang wrote: > On Wed, 2 Apr 2025 at 09:41, Coiby Xu <coxu@redhat.com> wrote: > > > > On Mon, Mar 31, 2025 at 08:15:08AM -0400, Mimi Zohar wrote: > > > On Mon, 2025-03-31 at 14:16 +0800, Baoquan He wrote: > > > > It doesn't make sense to run IMA functionality in kdump kernel, and that > > > > will cost extra memory. It would be great to allow IMA to be disabled on > > > > purpose, e.g for kdump kernel. > > > > > > > > Hence add a knob here to allow people to disable IMA if needed. > > > > > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > > > --- > > > > security/integrity/ima/ima_main.c | 21 +++++++++++++++++++++ > > > > 1 file changed, 21 insertions(+) > > > > > > > > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > > > > index 28b8b0db6f9b..5d677d1389fe 100644 > > > > --- a/security/integrity/ima/ima_main.c > > > > +++ b/security/integrity/ima/ima_main.c > > > > @@ -38,11 +38,27 @@ int ima_appraise; > > > > > > > > int __ro_after_init ima_hash_algo = HASH_ALGO_SHA1; > > > > static int hash_setup_done; > > > > +static int ima_disabled = 0; > > > > > > > > static struct notifier_block ima_lsm_policy_notifier = { > > > > .notifier_call = ima_lsm_policy_change, > > > > }; > > > > > > > > +static int __init ima_setup(char *str) > > > > +{ > > > > + 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\" ", str); > > > > + > > > > + return 1; > > > > +} > > > > +__setup("ima=", ima_setup); > > > > > > I understand your wanting to disable IMA for Kdump, but this goes way beyond > > > that. Please don't make it generic like this. > > > > > > Please refer to ima_appraise_parse_cmdline(). > > > > Hi Mimi, > > > > To save memory for kdump, it seems init_ima has been to be skipped thus > > ima=off is necessary (ima_appraise=off won't serve the purpose). Or do > > you have any specific concerns in mind? > > I think as Mimi said see below logic enforces the IMA even with the > cmdline disabling, see ima_appraise_parse_cmdline: > if (sb_state) { > if (!(appraisal_state & IMA_APPRAISE_ENFORCE)) > pr_info("Secure boot enabled: ignoring > ima_appraise=%s option", > str); > } else { > ima_appraise = appraisal_state; > } Thanks, RuiRui. Coiby, would disabling just IMA-measurement, as opposed to IMA-appraisal, save sufficient memory for kdump? > > > > > > > + > > > > + > > > > + > > > > static int __init hash_setup(char *str) > > > > { > > > > struct ima_template_desc *template_desc = ima_template_desc_current(); > > > > @@ -1176,6 +1192,11 @@ static int __init init_ima(void) > > > > { > > > > int error; > > > > > > > > + if (ima_disabled) { > > > > + pr_info("IMA functionality is disabled on purpose!"); > > > > + return 0; > > > > + } > > > > + > > > > ima_appraise_parse_cmdline(); > > > > ima_init_template_list(); > > > > hash_setup(CONFIG_IMA_DEFAULT_HASH); > > > > > > > > > > -- > > Best regards, > > Coiby > > > > > >
On Tue, Apr 01, 2025 at 11:30:09PM -0400, Mimi Zohar wrote: >On Wed, 2025-04-02 at 09:47 +0800, RuiRui Yang wrote: [...] >> > > that. Please don't make it generic like this. >> > > >> > > Please refer to ima_appraise_parse_cmdline(). >> > >> > Hi Mimi, >> > >> > To save memory for kdump, it seems init_ima has been to be skipped thus >> > ima=off is necessary (ima_appraise=off won't serve the purpose). Or do >> > you have any specific concerns in mind? >> >> I think as Mimi said see below logic enforces the IMA even with the >> cmdline disabling, see ima_appraise_parse_cmdline: >> if (sb_state) { >> if (!(appraisal_state & IMA_APPRAISE_ENFORCE)) >> pr_info("Secure boot enabled: ignoring >> ima_appraise=%s option", >> str); >> } else { >> ima_appraise = appraisal_state; >> } Thanks for pointing me to the above code! Note with the whole IMA disabled as done by this patch, the above code will not run so IMA (appraisal) won't be enforced. > >Thanks, RuiRui. > Mimi, so do I understand it correctly that your want IMA-appraisal to be always enabled as long as secure boot is enabled even if users choose to disable IMA? I wonder what security issue will it bring if this promise gets broken considering other LSMs can SELinux can be disabled when secure boot is enabled? >Coiby, would disabling just IMA-measurement, as opposed to IMA-appraisal, save >sufficient memory for kdump? For disabling just IMA-measurement, do you mean not enabling any measure rules? The more memory reserved for the kdump kernel, the less memory can be used by the 1st kernel. So from the perfective of kdump, we try to make the memory footprint as smaller as possible. Baoquan, do you have any statistics about the memory overhead of IMA?
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 28b8b0db6f9b..5d677d1389fe 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -38,11 +38,27 @@ int ima_appraise; int __ro_after_init ima_hash_algo = HASH_ALGO_SHA1; static int hash_setup_done; +static int ima_disabled = 0; static struct notifier_block ima_lsm_policy_notifier = { .notifier_call = ima_lsm_policy_change, }; +static int __init ima_setup(char *str) +{ + 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\" ", str); + + return 1; +} +__setup("ima=", ima_setup); + + + static int __init hash_setup(char *str) { struct ima_template_desc *template_desc = ima_template_desc_current(); @@ -1176,6 +1192,11 @@ static int __init init_ima(void) { int error; + if (ima_disabled) { + pr_info("IMA functionality is disabled on purpose!"); + return 0; + } + ima_appraise_parse_cmdline(); ima_init_template_list(); hash_setup(CONFIG_IMA_DEFAULT_HASH);
It doesn't make sense to run IMA functionality in kdump kernel, and that will cost extra memory. It would be great to allow IMA to be disabled on purpose, e.g for kdump kernel. Hence add a knob here to allow people to disable IMA if needed. Signed-off-by: Baoquan He <bhe@redhat.com> --- security/integrity/ima/ima_main.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)