| Message ID | 20200817215233.95319-4-bmeneg@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | integrity: improve user feedback for invalid bootparams | expand |
On Mon, 2020-08-17 at 18:52 -0300, Bruno Meneguele wrote: > Instead of print to kmsg any ima_appraise= option passed by the user in case > of secure boot being enabled, first check if the state was really changed > from its original "enforce" state, otherwise don't print anything. Please reword this patch description, removing "Instead of print to kmsg". > > Signed-off-by: Bruno Meneguele <bmeneg@redhat.com> > --- > security/integrity/ima/ima_appraise.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > index 2193b51c2743..000df14f198a 100644 > --- a/security/integrity/ima/ima_appraise.c > +++ b/security/integrity/ima/ima_appraise.c > @@ -19,11 +19,7 @@ > static int __init default_appraise_setup(char *str) > { > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM > - if (arch_ima_get_secureboot()) { > - pr_info("Secure boot enabled: ignoring ima_appraise=%s boot parameter option", > - str); > - return 1; > - } > + bool sb_state = arch_ima_get_secureboot(); > > if (strncmp(str, "off", 3) == 0) > ima_appraise = 0; > @@ -35,6 +31,16 @@ static int __init default_appraise_setup(char *str) > ima_appraise = IMA_APPRAISE_ENFORCE; > else > pr_err("invalid \"%s\" appraise option", str); > + > + /* If appraisal state was changed, but secure boot is enabled, > + * reset it to enforced */ > + if (sb_state) { > + if (!is_ima_appraise_enabled()) { > + pr_info("Secure boot enabled: ignoring ima_appraise=%s option", > + str); > + ima_appraise = IMA_APPRAISE_ENFORCE; > + } > + } Instead of changing ima_appraise and then resetting it back to enforcing, how about using a temporary variable instead? Maybe something like: if (!is_ima_appraise_enabled()) pr_info( ...) else ima_appraise = temporary value > #endif > return 1; > }
On Mon, Aug 24, 2020 at 04:11:22PM -0400, Mimi Zohar wrote: > On Mon, 2020-08-17 at 18:52 -0300, Bruno Meneguele wrote: > > Instead of print to kmsg any ima_appraise= option passed by the user in case > > of secure boot being enabled, first check if the state was really changed > > from its original "enforce" state, otherwise don't print anything. > > Please reword this patch description, removing "Instead of print to > kmsg". > ack > > > > Signed-off-by: Bruno Meneguele <bmeneg@redhat.com> > > --- > > security/integrity/ima/ima_appraise.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c > > index 2193b51c2743..000df14f198a 100644 > > --- a/security/integrity/ima/ima_appraise.c > > +++ b/security/integrity/ima/ima_appraise.c > > @@ -19,11 +19,7 @@ > > static int __init default_appraise_setup(char *str) > > { > > #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM > > - if (arch_ima_get_secureboot()) { > > - pr_info("Secure boot enabled: ignoring ima_appraise=%s boot parameter option", > > - str); > > - return 1; > > - } > > + bool sb_state = arch_ima_get_secureboot(); > > > > if (strncmp(str, "off", 3) == 0) > > ima_appraise = 0; > > @@ -35,6 +31,16 @@ static int __init default_appraise_setup(char *str) > > ima_appraise = IMA_APPRAISE_ENFORCE; > > else > > pr_err("invalid \"%s\" appraise option", str); > > + > > + /* If appraisal state was changed, but secure boot is enabled, > > + * reset it to enforced */ > > + if (sb_state) { > > + if (!is_ima_appraise_enabled()) { > > + pr_info("Secure boot enabled: ignoring ima_appraise=%s option", > > + str); > > + ima_appraise = IMA_APPRAISE_ENFORCE; > > + } > > + } > > Instead of changing ima_appraise and then resetting it back to > enforcing, how about using a temporary variable instead? Maybe > something like: > > if (!is_ima_appraise_enabled()) > pr_info( ...) > else > ima_appraise = temporary value > Yes, indeed it would be nice to keep the default state unchanged. Only thing is that 'is_ima_appraise_enabled()' directly checks 'ima_appraise & IMA_APPRAISE_ENFORCE', changing to a temp var would require the if() to check it directly. I'm going to send a v2 with it changed. Thanks. > > #endif > > return 1; > > } > >
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 2193b51c2743..000df14f198a 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -19,11 +19,7 @@ static int __init default_appraise_setup(char *str) { #ifdef CONFIG_IMA_APPRAISE_BOOTPARAM - if (arch_ima_get_secureboot()) { - pr_info("Secure boot enabled: ignoring ima_appraise=%s boot parameter option", - str); - return 1; - } + bool sb_state = arch_ima_get_secureboot(); if (strncmp(str, "off", 3) == 0) ima_appraise = 0; @@ -35,6 +31,16 @@ static int __init default_appraise_setup(char *str) ima_appraise = IMA_APPRAISE_ENFORCE; else pr_err("invalid \"%s\" appraise option", str); + + /* If appraisal state was changed, but secure boot is enabled, + * reset it to enforced */ + if (sb_state) { + if (!is_ima_appraise_enabled()) { + pr_info("Secure boot enabled: ignoring ima_appraise=%s option", + str); + ima_appraise = IMA_APPRAISE_ENFORCE; + } + } #endif return 1; }
Instead of print to kmsg any ima_appraise= option passed by the user in case of secure boot being enabled, first check if the state was really changed from its original "enforce" state, otherwise don't print anything. Signed-off-by: Bruno Meneguele <bmeneg@redhat.com> --- security/integrity/ima/ima_appraise.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)