diff mbox series

[3/4] ima: limit secure boot feedback scope for appraise

Message ID 20200817215233.95319-4-bmeneg@redhat.com (mailing list archive)
State New
Headers show
Series integrity: improve user feedback for invalid bootparams | expand

Commit Message

Bruno Meneguele Aug. 17, 2020, 9:52 p.m. UTC
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(-)

Comments

Mimi Zohar Aug. 24, 2020, 8:11 p.m. UTC | #1
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;
>  }
Bruno Meneguele Sept. 4, 2020, 5:47 p.m. UTC | #2
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 mbox series

Patch

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;
 }