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

[Nayna <nayna@linux.vnet.ibm.com>]

Hi Michael,


On 10/15/2019 07:29 AM, Michael Ellerman wrote:
> Nayna Jain <nayna@linux.ibm.com> writes:
>> This patch adds the measurement rules to the arch specific policies on
>> trusted boot enabled systems.
>>
>> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> ---
>>   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