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

[Bruno Meneguele <bmeneg@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]

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

-- 
bmeneg 
PGP Key: http://bmeneg.com/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]