Re: [PATCH v7 4/8] powerpc/ima: add measurement rules to ima arch specific policy

[Nayna]

Hi Michael,


On 10/15/2019 07:29 AM, Michael Ellerman wrote:
> Nayna Jain <nayna@xxxxxxxxxxxxx> writes:
> > This patch adds the measurement rules to the arch specific policies on
> > trusted boot enabled systems.
> >
> > Signed-off-by: Nayna Jain <nayna@xxxxxxxxxxxxx>
> > Reviewed-by: Mimi Zohar <zohar@xxxxxxxxxxxxx>
> > ---
> >   arch/powerpc/kernel/ima_arch.c | 45 +++++++++++++++++++++++++++++++---
> >   1 file changed, 42 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> > index c22d82965eb4..88bfe4a1a9a5 100644
> > --- a/arch/powerpc/kernel/ima_arch.c
> > +++ b/arch/powerpc/kernel/ima_arch.c
> > @@ -12,8 +12,19 @@ bool arch_ima_get_secureboot(void)
> >   	return is_powerpc_os_secureboot_enabled();
> >   }
> >   
> > -/* Defines IMA appraise rules for secureboot */
> > +/*
> > + * The "arch_rules" contains both the securebot and trustedboot rules for adding
> > + * the kexec kernel image and kernel modules file hashes to the IMA measurement
> > + * list and verifying the file signatures against known good values.
> > + *
> > + * The "appraise_type=imasig|modsig" option allows the good signature to be
> > + * stored as an xattr or as an appended signature. The "template=ima-modsig"
> > + * option includes the appended signature, when available, in the IMA
> > + * measurement list.
> > + */
> >   static const char *const arch_rules[] = {
> > +	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
> > +	"measure func=MODULE_CHECK template=ima-modsig",
> >   	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> >   #if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
> >   	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> > @@ -22,12 +33,40 @@ static const char *const arch_rules[] = {
> >   };
> >   
> >   /*
> > - * Returns the relevant IMA arch policies based on the system secureboot state.
> > + * The "measure_rules" are enabled only on "trustedboot" enabled systems.
> > + * These rules add the kexec kernel image and kernel modules file hashes to
> > + * the IMA measurement list.
> > + */
> > +static const char *const measure_rules[] = {
> > +	"measure func=KEXEC_KERNEL_CHECK",
> > +	"measure func=MODULE_CHECK",
> Why do these ones not have "template=ima-modsig" on the end?

ima-modsig template is applicable only when IMA "collects" the appended 
signatures. IMA can then include it in the measurement list.

> > +	NULL
> > +};
> > +
> > +/*
> > + * Returns the relevant IMA arch policies based on the system secureboot
> > + * and trustedboot state.
> >    */
> >   const char *const *arch_get_ima_policy(void)
> >   {
> > -	if (is_powerpc_os_secureboot_enabled())
> > +	const char *const *rules;
> > +	int offset = 0;
> > +
> > +	for (rules = arch_rules; *rules != NULL; rules++) {
> > +		if (strncmp(*rules, "appraise", 8) == 0)
> > +			break;
> > +		offset++;
> > +	}
> This seems like kind of a hack, doesn't it? :)
>
> What we really want is three sets of rules isn't it? But some of them
> are shared between the different sets. But they just have to be flat
> arrays of strings.
>
> I think it would probably be cleaner to just use a #define for the
> shared part of the rules, eg something like:
>
> #ifdef CONFIG_MODULE_SIG_FORCE
> #define APPRAISE_MODULE
> #else
> #define APPRAISE_MODULE \
> 	"appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
> #endif
>
> #define APPRAISE_KERNEL \
> 	"appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig"
>
> #define MEASURE_KERNEL \
> 	"measure func=KEXEC_KERNEL_CHECK"
>
> #define MEASURE_MODULE \
> 	"measure func=MODULE_CHECK"
>
> #define APPEND_TEMPLATE_IMA_MODSIG		\
> 	" template=ima-modsig"
>
> static const char *const secure_and_trusted_rules[] = {
> 	MEASURE_KERNEL APPEND_TEMPLATE_IMA_MODSIG,
> 	MEASURE_MODULE APPEND_TEMPLATE_IMA_MODSIG,
> 	APPRAISE_KERNEL,
> 	APPRAISE_MODULE
> 	NULL
> };
>
> static const char *const secure_rules[] = {
> 	APPRAISE_KERNEL,
> 	APPRAISE_MODULE
> 	NULL
> };
>
> static const char *const trusted_rules[] = {
> 	MEASURE_KERNEL,
> 	MEASURE_MODULE,
> 	NULL
> };

Yes, I agree it is sort of a hack to walk through the rules to find the 
start of the appraise policy. While trying your suggestion, I realized 
that defining three arrays, with some rule duplication, can fix the hack 
without #defines. This also improves readability of the rules. I have 
just now posted the new version with the changes. Please let me know if 
this looks ok.

Thanks & Regards,
ÂÂÂÂ - Nayna